Fix slug for single-file plugins in both admin and CLI #747
Conversation
This can be handled by `Plugin_Context`
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
swissspidy
changed the title
|
OK the Behat test require some work when WP isn't fully loaded yet 😄 |
@swissspidy Should we add polyfill function for |
|
It's just |
swissspidy
requested review from
felixarntz,
davidperezgar and
mukeshpanchal27
| @@ -627,7 +627,7 @@ final protected function get_slug() { | |||
| private function get_check_context() { | |||
| $plugin_basename = $this->get_plugin_basename(); | |||
| $plugin_path = is_dir( $plugin_basename ) ? $plugin_basename : WP_PLUGIN_DIR . '/' . $plugin_basename; | |||
| return new Check_Context( $plugin_path, $this->get_slug() ); | |||
| return new Check_Context( $plugin_path, $this->get_slug() ? $this->get_slug() : basename( $plugin_path, '.php' ) ); | |||
There was a problem hiding this comment.
I think this should happen within Plugin_Context (which Check_Context extends).
That's where there's already logic to determine the slug by default, if none was specified. It doesn't make sense to me to have that duplicated in this class, neither here nor in the set_slug() method where it was so far.
I think it makes most sense to have get_slug() pass through the empty string that get_slug_param() may return (for AJAX always, for CLI only if no slug was explicitly specified). And then because that string is empty, Plugin_Context would define the default slug as the source of truth.
Alternatively, if there's value to have the logic here in Abstract_Check_Runner, we should remove the logic from Plugin_Context and make the constructor $slug parameter required (and throw an exception if it's empty). It just doesn't make sense to have the logic split/duplicated between two places.
There was a problem hiding this comment.
I just moved this there in 16f8e09 because otherwise it didn't work when using WP-CLI "shortinit" with --require. We don't pass enough information to Check_Context/Plugin_Context on its own to determine whether it's a single-file plugin or not.
Feel free to make edits to the PR with what you have in mind.
Aside: whenever we have to add workarounds like this I'm wondering whether we actually really want to support single-file plugins or not. They aren't allowed in the repo anyway.
There was a problem hiding this comment.
How does WP-CLI "shortinit" work? Not familiar with it... How does it prevent that logic in the constructor from working correctly?
There was a problem hiding this comment.
I meant the --require hack we use with the object-cache drop-in. Then a lot of the WP constants and functions won't be available that early. See https://github.com/WordPress/plugin-check/actions/runs/11660800826/job/32463868033?pr=747 for example.
There was a problem hiding this comment.
Thanks for clarifying. I can have a look later this week to see if I can come up with a solution for that.
Agree on this one. There are lots of workarounds for the single file plugin. |
Do not try to guess slug in
Abstract_Check_Runner. This can be handled byPlugin_Context.This way both admin and CLI will determine the same slug for the same plugin.
In
Plugin_Context, use correct slug for single-file plugins. Forhello.phpit would behello.Fixes #745