CLI: object-cache.php drop-in prevents extension usage

[ernilambar]

Sparked from #518

• Issue is related with object-cache.php
• When same CLI command is run twice, second command does not give expected output due to above file.

Issue is being investigated in #533

[swissspidy]

This isn't really about running commands twice. It's about the drop-in flipping the execution order on its head, thus preventing any other plugins from hooking into PCP.

It just so happens that when you run the command and the drop-in doesn't exist yet, it will be created, but it won't affect the loading order until next time.

[ernilambar]

I am wondering what would be the fix for this. We could remove use after_invoke WP-CLI hook to delete the cache file after command is run. But that would kinda defeat the purpose of caching I guess.

[swissspidy]

Well I was hoping we can just remove this drop-in completely, not just for CLI but everywhere (see #533). If you want to execute runtime checks, the plugin would simply need to be already active. I think that's a reasonable tradeoff.

[ernilambar]

Yah. That approach is also good. It would be great if we could gather other reviews in that PR as this issue is blocking other PRs also.

[swissspidy]

cc @felixarntz @mukeshpanchal27 @joemcgill for thoughts

Here's what this is about:

We want to make it possible for the plugins team and others to create add-ons to Plugin Check that provide additional checks. #518 aimed to add a test demonstrating that this works, only it doesn't!

The problem is,object-cache.php is used as a "hook" to short-circuit regular WordPress loading order and manually execute the Plugin Check CLI command before WordPress has fully loaded. That also means before any add-on plugins have actually loaded...

This "hack" appears to be used so we can perform runtime checks for a plugin without the plugin having to be active on the site.

In my opinion, this additional complexity not only prevents such add-ons from working, it's also simply not worth the maintenance nightmare. We can simply make it a requirement for the plugin to be active in order to perform runtime checks. Then we can simplify this a lot.

That said, I do lack full context though on why this was implemented this way, so appreciate any feedback.

[felixarntz]

@swissspidy @ernilambar

The problem is,object-cache.php is used as a "hook" to short-circuit regular WordPress loading order and manually execute the Plugin Check CLI command before WordPress has fully loaded. That also means before any add-on plugins have actually loaded...

This is definitely something we need to address. However, I don't think that removing the object-cache.php workaround is a good path forward. Also note that the CLI command itself doesn't run early, it's only the additional logic in the cli.php file, and only if it's explicitly --required.

This "hack" appears to be used so we can perform runtime checks for a plugin without the plugin having to be active on the site.

It's not that simple. It's mostly to prevent the runtime checks from messing with the actual site content or functionality, by working in a separate set of database tables (similar to how e.g. the WP unit tests do). I acknowledge this is a hack and makes the code more complicated, but it's what makes runtime checks possible in a safe way.

I'm confident we can come up with a solution to the concrete problem that doesn't involve removing the runtime check foundation. Just dumping some thoughts:

• The checks themselves don't need to run early even when runtime checks are used. The only code that needs to run early is to bootstrap the environment for running runtime checks.
• So we should be able to get the list of checks at a time where plugins are always loaded, with only minor limitations. I suppose if an extension was going to add a runtime check that would be the only runtime check requested to run, we may not be able to recognize that. But in terms of actually running checks, there shouldn't be an issue, so I assume there's a problem in our code.
• Last but not least, all the early handling is only needed when any runtime checks should be run - at least only then it should be executed. So by invoking the CLI command (or AJAX) in a way that only runs static checks, there should be no issues. If there are, probably also a bug in our code we have to fix.

I'm happy to help with addressing this, but I'm not fully understanding what the problem is that we ran into in #518. Can you give some pointers to the concrete problem / code / CI run?

[swissspidy]

Also note that the CLI command itself doesn't run early, it's only the additional logic in the cli.php file, and only if it's explicitly --required.

I'm happy to help with addressing this, but I'm not fully understanding what the problem is that we ran into in #518. Can you give some pointers to the concrete problem / code / CI run?

Well the issue is how the cli.php copies the drop-in if it's missing, so the next time you run the CLI command the execution order is mixed up. This is without any extra --require

You can reproduce this by manually performing the steps from #518 (create an add-on plugin, run the CLI command).

[felixarntz]

@swissspidy Ah, makes sense. In that case, could we add cleanup logic to cli.php that removes the drop-in again after the process?