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.

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

Autoloader: Avoid a PHP warning when an empty string is passed to is_directory_plugin(). #16442

Merged
merged 2 commits into from
Jul 9, 2020

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Jul 9, 2020

Autoloader: Avoid a PHP warning when an empty string is passed to is_directory_plugin().

Example Warning: strpos(): Offset not contained in string.

Changes proposed in this Pull Request:

  • Avoids a PHP warning when an empty string is passed to is_directory_plugin().

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

I'm not really sure how this has happened, but on WordPress.org get_option( 'active_plugins' ) returns '' (an empty string) on some sites. I don't know if that's expected or not, but I suspect it only happens on sites that have never had a plugin activated on it directly, only through Network-activated plugins.

It causes get_active_plugin_paths() https://github.com/Automattic/jetpack/blob/master/packages/autoloader/src/class-plugins-handler.php#L48-L52 to have array( '' ) as it's $plugin_slugs value, which seems like it might be expected due to it casting the option result to an array..
I've included a unit test which covers at least the result of this.

An alternative patch could have been to perform an array_filter() over $plugin_slugs first in get_active_plugin_paths() but this seemed more defensive.

Proposed changelog entry for your changes:

  • N/A

…_directory_plugin()`.

Example Warning: `strpos(): Offset not contained in string`.
@jetpackbot
Copy link

jetpackbot commented Jul 9, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16442

Scheduled Jetpack release: August 4, 2020.
Scheduled code freeze: July 28, 2020

Generated by 🚫 dangerJS against 4860b7e

@dd32 dd32 requested a review from kbrown9 July 9, 2020 00:52
@kraftbj kraftbj added this to the 8.7.1 milestone Jul 9, 2020
@kraftbj kraftbj added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Bug When a feature is broken and / or not performing as intended labels Jul 9, 2020
@kraftbj
Copy link
Contributor

kraftbj commented Jul 9, 2020

Thanks for the patch. We can't duplicate this on a brand new multisite with no local plugins activated in our initial look, so might just be some odd legacy w.org stuff going on.

Copy link
Member

@kbrown9 kbrown9 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for the fix and the unit test!

@dd32
Copy link
Member Author

dd32 commented Jul 9, 2020

We can't duplicate this on a brand new multisite with no local plugins activated in our initial look, so might just be some odd legacy w.org stuff going on.

I've never known the option to not contain an array either, so I really do suspect it's some ancient effect of some kind.

I suspect it may have predated the $default_value 2nd-argument to get_option() which caused the options to be set to an empty string in the first place, potentially even a bug only in a non-released version of WP (ie. trunk) which WordPress.org ran.

@kraftbj
Copy link
Contributor

kraftbj commented Jul 9, 2020

Makes sense. Either way, 40 minutes without any warnings on w.org, so that's an approved review from me. Thanks for the assist!

@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jul 9, 2020
@kraftbj kraftbj self-assigned this Jul 9, 2020
@jeherve jeherve merged commit 2927b61 into Automattic:master Jul 9, 2020
@matticbot matticbot added [Status] Needs Changelog [Status] Needs Package Release This PR made changes to a package. Let's update that package now. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jul 9, 2020
jeherve pushed a commit that referenced this pull request Jul 9, 2020
…s passed to `is_directory_plugin()`. (#16442)

Co-authored-by: Brandon Kraft <[email protected]>
@jeherve
Copy link
Member

jeherve commented Jul 9, 2020

Cherry-picked to branch-8.7 in 9c1c811

@jeherve
Copy link
Member

jeherve commented Jul 9, 2020

Updated the release branch to a new version of Jetpack Autoloader with that fix in 461b349

@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Package Release This PR made changes to a package. Let's update that package now. labels Jul 9, 2020
pereirinha pushed a commit that referenced this pull request Jul 27, 2020
…s passed to `is_directory_plugin()`. (#16442)

Co-authored-by: Brandon Kraft <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Autoloader [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants