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

Added dynamic plugins_loaded hook priority. #14720

Merged
merged 3 commits into from
Feb 19, 2020

Conversation

zinigor
Copy link
Member

@zinigor zinigor commented Feb 18, 2020

Before we were using priority 1 always, but some plugins want to use the hook even earlier than that, so we are watching the filters being added on each plugin load and reassigning our own configure call to an earlier priority.

Fixes a bug with authenticated REST API calls with plugins like Give or WPMU enabled.

Changes proposed in this Pull Request:

  • Added dynamic plugins_loaded hook priority determining mechanism.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • This is a bug fix.

Testing instructions:

  • Use a snippet like this to trigger the problem:
function break_jetpack(){
	wp_get_current_user();
}
add_action( 'plugins_loaded', 'break_jetpack', 0 );
  • Make an authenticated REST API call to your site using a mobile application or CLI.
  • Observe the fatal error.
  • Use this PR, observe no fatal errors.

Proposed changelog entry for your changes:

  • Fixed a compatibility issue with several plugins on REST API calls.

Before we were using priority 1 always, but some plugins want to use the hook even earlier than that, so we are watching the filters being added on each plugin load and reassigning our own configure call to an earlier priority.
@zinigor zinigor added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Focus] Jetpack DNA labels Feb 18, 2020
@zinigor zinigor requested a review from a team as a code owner February 18, 2020 17:45
@jetpackbot
Copy link

jetpackbot commented Feb 18, 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.

Scheduled Jetpack release: March 3, 2020.
Scheduled code freeze: February 25, 2020

Generated by 🚫 dangerJS against 56fb5a3

class.jetpack.php Outdated Show resolved Hide resolved
@kraftbj kraftbj added this to the 8.2.2 milestone Feb 18, 2020
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! I manually tested this change as described in the testing instructions and verified that the PR fixes the error.

Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

It's pretty clever. Works as advertised, but definitely want more eyes on it since it is a rather big change that has a point release potential.

Copy link
Contributor

@astralbodies astralbodies left a comment

Choose a reason for hiding this comment

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

I've tested the branch on my WooCommerce test site with one of the problematic plugins activated and have confirmed that this fixes it. Switching back to stable breaks the API again.

@kraftbj kraftbj merged commit 07de289 into master Feb 19, 2020
@kraftbj kraftbj deleted the fix/early-plugins-loaded-hooks branch February 19, 2020 20:10
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Feb 19, 2020
@kraftbj
Copy link
Contributor

kraftbj commented Feb 19, 2020

Added to branch-8.2 via a6ac28a and bumped version to 8.2.2-beta1 via 4bf2c62f2 to enable beta channel folks to get this update.

kraftbj pushed a commit that referenced this pull request Feb 19, 2020
* Added dynamic plugins_loaded hook priority.

Before we were using priority 1 always, but some plugins want to use the hook even earlier than that, so we are watching the filters being added on each plugin load and reassigning our own configure call to an earlier priority.
@peterfabian
Copy link
Contributor

Does this mean that the autoloader check for plugins_loaded is no longer necessary https://github.com/Automattic/jetpack/blob/master/packages/autoloader/src/autoload.php#L144?

@zinigor
Copy link
Member Author

zinigor commented Feb 20, 2020

@peterfabian no, this check applies to other consumers as well that may be using the Autoloader incorrectly. Otherwise we may end up in a situation where a consumer unknowingly forces usage of a library before every plugin has had a chance to load their dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Jetpack DNA [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.

8 participants