Add feature tests for checking plugin with addon enabled

[ernilambar]

• Add feature tests with addon enabled (which includes new category and extra checks under that category)

[ernilambar]

Note: Behat tests are passing in my local setup with following specs.

PHP: 8.2.19
Node: 20.11.1

[ernilambar]

Problem I am facing is classes on main plugin are not loading in the addon. Since WordPress not necessarily loads plugin files in our required order, how can I make sure that all classes of main plugin are available in the addon?

CC @swissspidy

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ernilambar <[email protected]>
Co-authored-by: swissspidy <[email protected]>
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: davidperezgar <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[ernilambar]

May be we should also add instructions in our docs on how to create addon for Plugin Check.

[swissspidy]

Yeah simply defining the classes like that would be error prone.

First, you'll want to use the Requires Plugins header.

Second, I would recommending moving the classes to separate files and then load them only when needed and when you know the parent classes exist. For example like so:

add_filter(
        'wp_plugin_check_checks',
        function ( array $checks ) {
          require_once './class-prohibited-text-check.php';
          require_once './class-postsperpage-check.php';

          return array_merge(
            $checks,
            array(
              'prohibited_text' => new Prohibited_Text_Check(),
              'postsperpage'    => new PostsPerPage_Check(),
            )
          );
        }
      );

[swissspidy]

May be we should also add instructions in our docs on how to create addon for Plugin Check.

Yeah @davidperezgar brought that up too. First we can start small, explaining the filters and giving a short example. We can reuse some of the code you're writing here for that.

Not really urgent though, as I doubt there is huge demand for writing those 😄

[davidperezgar]

Yes! That would be very helpful.

[ernilambar]

When I plugin check command is run, may be there needs to cleaned up. Because when same command run twice, in the second time external checks are not detected. For example, in line 463 and 477 I have kept same command but output is not as expected.

[swissspidy]

That sounds odd 🤔 Sounds like some debugging is in order

[ernilambar]

@swissspidy I tried this to reproduce this in separate setup. After first command run, those checks are not included until I delete file object-cache.php.

[swissspidy]

So it's an issue with Plugin Check / the object-cache.php drop-in and not with the test?

Want me to take a look at it to see if I can find the issue, or what's the next step?

[ernilambar]

Yes, it does not seem to be related to Behat test. I tried to debug but could not find the exact issue. May be you can give a shot to find issue quicker as you are more familiar with the architecture 😊

[swissspidy]

Well, I'm not that familiar with that part 😅

The gist is thatobject-cache.php is used as a "hook" to short-circuit regular WordPress loading order and manually execute the PCP CLI command before WordPress has fully loaded. That also means before any plugins have actually loaded...

From what I can see, this hack is used so we can perform runtime checks for a plugin without the plugin having to be active on the site. But not entirely sure.

When I activate pcp-addon and foo-plugin and remove all of this object-cache.php stuff, the runtime checks and the custom addon checks work just fine. See #533 for where I attempted this.

Related: #153

[ernilambar]

I removed before_wp_load hook and all tests are passing. Did we miss to add test for CLI_Runner or it is not needed now?

[swissspidy]

It's still needed and used by Plugin_Check_Command. Now it's just instantiated later and not during the object-cache setup. See also #533.

[davidperezgar]

Behat tests are not passing.

[ernilambar]

Behat tests are not passing.

Yes. See #518 (comment) above and related conversation. That needs to be fixed first.

[felixarntz]

@swissspidy @ernilambar I've been looking through the codebase a bit more to find potential problems, and so far I believe I found at least one, in Abstract_Check_Runner::get_checks_to_run():

• There's a check $this->initialized_early || $this->runtime_environment->can_set_up(), based on which runtime checks are being used. This makes sense for the AJAX based workflow because:
  • Checks are being retrieved as a first step (before anything is set up).
  • Therefore we need to check whether the environment is already set up or, if not, whether it can be set up.
    • The $this->initialized_early is basically whether the environment is already set up, since that can only be true if the object-cache.php drop-in is already present.
    • Even if $this->initialized_early is false at this point, it's fine as long as the environment can be set up, as in the AJAX workflow this happens in a separate request afterwards, but before any request that runs any checks.
• For the CLI based workflow, I believe this condition is wrong though. With CLI, it all runs in a single request so if the environment is not set up already (which requires the --require to place the object-cache.php drop-in early), we can't do this later, since the CLI command runs after WordPress, plugins etc have already been loaded.

In other words, I think for CLI we either need to reduce this to just $this->initialized_early, or we go with $this->initialized_early && $this->runtime_environment->can_set_up() (which shouldn't make any difference in outcome, but may be better for extra safety).

I don't know if this bug is related to the failure here, but it's worth hacking it in temporarily to try. If that's it, I think we should open a separate PR to address the bug. I think a clean implementation would be to replace the current $this->initialized_early || $this->runtime_environment->can_set_up() check with a call to an abstract method like can_use_runtime_checks(), so that it can be implemented differently between the AJAX and CLI runners.

Let me know if you have any questions. FWIW I'm thinking it might be useful to make a flowchart of how this works in the different circumstances to make it easier to follow.

[swissspidy]

FWIW I'm thinking it might be useful to make a flowchart of how this works in the different circumstances to make it easier to follow.

Yeah as you can see, neither @ernilambar nor I are really familiar with that flow as it's not really documented and hard to follow. If someone from the original implementers group can do that, this would be helpful.

tbh, I really wish all this magic wasn't needed, and that we could address #153 instead and can come up with a simpler solution for things like the GitHub Action where it's a controlled environment.

[swissspidy]

I've been looking through the codebase a bit more to find potential problems, and so far I believe I found at least one, in Abstract_Check_Runner::get_checks_to_run(): [...]

Could you perhaps make those changes against this PR yourself? That way it's easier to follow.

[swissspidy]

I just pushed some changes to make the tests easier to reason about.
Found a couple of issues along the way, like how the drop-in wasn't actually copied correctly because ABSPATH isn't defined before_wp_load, so I switched to after_wp_config_load.

Also included a runtime check in the example add-on so we can verify those work too (they don't).

We'd need to figure out how we could possibly load an add-on in CLI when doing runtime checks. Maybe the add-ons to load need to be explicitly passed as an arg or something?

[felixarntz]

@swissspidy @ernilambar Please review #597, which is about documenting all the current process flows. While doing that, I identified a number of problems (mostly related about addon checks indeed). I believe with the additional context from that documentation (potentially after a few reviews to clarify things more), we can collaborate in finding solutions to these problems.

[swissspidy]

Proposed next steps:

1. Update the PHP tests so that they pass again
   • Happens because I'm checking for the WP_CLI constant now
2. Merge the PR with some Behat tests commented out
3. Address Documentation of PCP check process flows in all scenarios #597 (comment) in follow-up PRs, re-enabling the commented out Behat tests

[felixarntz]

LGTM, just minor notes.

[felixarntz]

In principle, this should only be placed if runtime checks are run. If only static checks should be run against a plugin, this isn't needed. But I'm not sure it's reasonably possible to detect that from here.

FWIW at the moment users of WP-CLI have to know about the --require portion anyway, so because of that it's probably okay to also expect of them to only include the --require if they want to run any runtime checks. Something to improve elsewhere, as we've already discussed a few times.