Implement network-specific metadata handling in commands

[spacedmonkey]

Added overrides for add, update, get, and delete metadata methods to utilize network-specific options when available. This ensures compatibility and functionality for multisite network scenarios. Fallbacks to standard metadata functions are maintained for non-network environments.

Add function exists, to ensure compatablity with 4.2 below.

Fixes #504

[spacedmonkey]

Pinging maintainers to get eyes on this @swissspidy @danielbachhuber @schlessera

[swissspidy]

Two thoughts:

1. Network meta command accessing the incorrect caches #504 mentions steps to reproduce the issue — this could be added as a Behat test.
2. Looking at https://core.trac.wordpress.org/ticket/61467, it sounds like this could just as well be fixed only in core — so why this change here? Could we just wait for the core fix?

[jonnynews]

Note that we get pinged automatically with the review request :)

No one was assigned for a code reviewers. Maybe this repo needs a codeowner file 🤔

#504 mentions steps to reproduce the issue — this could be added as a Behat test.

Before spending any time on getting tests to work, I wanted to make sure everyone was happy with the approach.

Looking at https://core.trac.wordpress.org/ticket/61467, it sounds like this could just as well be fixed only in core — so why this change here? Could we just wait for the core fix?

The core patch is not confirmed yet and not work. Also that will only fix the issue with 6.8+. WP CLI support down to 3.7. Fixing this here is to ensure BC.

[schlessera]

Yes, approach looks good, @jonnynews. If we add the tests from #504 , this will be good to go.

[swissspidy]

According to the issue description, the following test should reproduce the situation:

  Scenario: Network meta is actually network options
    Given a WP multisite install

    When I run `wp eval 'update_network_option( 1, "mykey", "123" );'`
    And I run `wp eval 'echo get_network_option( 1, "mykey" );'`
    Then STDOUT should be:
      """
      123
      """

    When I run `wp network meta update 1 mykey 456`
    Then STDOUT should be:
      """
      Success: Updated custom field 'mykey'.
      """

    When I run `wp network meta get 1 mykey`
    Then STDOUT should be:
      """
      456
      """

    When I run `wp eval 'echo get_network_option( 1, "mykey" );'`
    Then STDOUT should be:
      """
      456
      """

Is that accurate?

Because this currently passes for me on main... 🤔

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/Network_Meta_Command.php	50.00%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[jonnynews]

@swissspidy The issue is the metadata api and network option api, update different object cache keys. Without persistent object cache, you will likely not see the issue.

[swissspidy]

Ah, okay! I will try to see how we can best test an object cache setup with our test suite then

[spacedmonkey]

@swissspidy My recommendation would be use something like SQLite Object Cache, so you can have a object cache setup, without the overhead having install memcache / redis packages and running a another container. SQLite is also already used when it detects if mysql is not setup.

In general, i would recommend running the behat tests with and without object caching. I wonder if it would find any other bugs.

I would help, but I don't know your test package that well.

[swissspidy]

Cool yeah makes sense, I've implemented that now and see some unrelated test failures.

But the example test I shared above still passes on main branch, both with and without an object cache. In other words, I don't yet have a test that verifies this change.

Running with WP_CLI_TEST_DBTYPE=sqlite WP_CLI_TEST_OBJECT_CACHE=sqlite composer behat features/network-meta.feature (after doing a composer update)

[swissspidy]

Turns out, there is actually 1 failing test in extension-command that will be fixed by this change.

https://github.com/wp-cli/extension-command/blob/252674e3fb310e1a6cbb946cfb15d70c6cdabc63/features/theme.feature#L319-L394

001 Scenario: Enabling and disabling a theme # features/theme.feature:320
      Then STDOUT should contain:            # features/theme.feature:377
        $ wp network-meta get 1 allowedthemes
        array (
          'twentytwentyfive' => true,
        )
        
        cwd: /tmp/wp-cli-test-run--69c497154be549.74820243/
        run time: 0.33636903762817
        exit status: 0
        
        Output does not contain expected string:
        'moina-blog' => true
        
        --- Expected
        +++ Actual
        @@ @@
        -'moina-blog' => true
        +array (
        +  'twentytwentyfive' => true,
        +)
         (Exception)

As you can see, it uses wp network-meta.

The test passes when using this PR.

Though it also passes when replacing wp network-meta with wp site option