Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix slug for single-file plugins in both admin and CLI #747

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from
+4 −8
Open

Fix slug for single-file plugins in both admin and CLI #747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e3480c6
Do not try to guess slug in `Abstract_Check_Runner`
swissspidy Oct 25, 2024
48a3ed9
Use correct slug for single-file plugins
swissspidy Oct 25, 2024
7b5bde6
Don't use `plugin_dir_path`
swissspidy Nov 4, 2024
31a86e2
Merge branch 'trunk' into fix/745-slug-domain
swissspidy Nov 4, 2024
a184b20
No `trailingslashit`
swissspidy Nov 4, 2024
ce049d5
Remove errant space
swissspidy Nov 4, 2024
89ba796
Merge branch 'trunk' into fix/745-slug-domain
swissspidy Nov 6, 2024
16f8e09
Don't use in constructor
swissspidy Nov 6, 2024
f430cc2
Merge branch 'trunk' into fix/745-slug-domain
davidperezgar Nov 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions includes/Checker/Abstract_Check_Runner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' ) );
Copy link
Member

@felixarntz felixarntz Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does WP-CLI "shortinit" work? Not familiar with it... How does it prevent that logic in the constructor from working correctly?

Copy link
Member Author

@swissspidy swissspidy Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying. I can have a look later this week to see if I can come up with a solution for that.

}

/**
Expand All @@ -640,10 +640,6 @@ private function get_check_context() {
final public function set_slug( $slug ) {
if ( ! empty( $slug ) ) {
$this->slug = $slug;
} else {
$basename = $this->get_plugin_basename();

$this->slug = ( '.' === pathinfo( $basename, PATHINFO_DIRNAME ) ) ? $basename : dirname( $basename );
}
}

Expand Down
6 changes: 3 additions & 3 deletions includes/Plugin_Context.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@
if ( ! empty( $slug ) ) {
$this->slug = $slug;
} else {
$this->slug = basename( dirname( $this->main_file ) );
$this->slug = basename( dirname( $this->main_file ), '.php' );
}
}

Expand Down Expand Up @@ -128,9 +128,9 @@
*/
public function path( $relative_path = '/' ) {
if ( is_dir( $this->main_file ) ) {
return trailingslashit( $this->main_file ) . ltrim( $relative_path, '/' );
return $this->main_file . '/' . ltrim( $relative_path, '/' );

Check warning on line 131 in includes/Plugin_Context.php

View check run for this annotation

Codecov / codecov/patch

includes/Plugin_Context.php#L131 

Added line #L131 was not covered by tests
} else {
return plugin_dir_path( $this->main_file ) . ltrim( $relative_path, '/' );
return dirname( $this->main_file ) . '/' . ltrim( $relative_path, '/' );
}
}

Expand Down
Loading