-
Notifications
You must be signed in to change notification settings - Fork 819
Backup: Prevent _load_textdomain_just_in_time PHP notices by adding admin_menu hook #42396
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
Backup: Prevent _load_textdomain_just_in_time PHP notices by adding admin_menu hook #42396
Conversation
…to prevent translation loading order issues
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Backup plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth looking at the other standalone plugins at the same time, since they all follow the same pattern?
@fgiannar good catch, thank you! It looks like the issue you reported is only replicable in that scenario - VaultPress Backup active, Akismet active, and no other standalones including Jetpack.
@jeherve, yes, good idea. In doing so, I noticed that Boost, Search and VideoPress also already have this issue - when active with only Akismet active as well (no Jetpack). I've created an issue here: #42497 I should also add that in testing I tested with all other standalones included at the same time, and the translation issue only occurred with VaultPress Backup. The disappearing menu item appears to only be an issue in the scenario mentioned above. By changing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the Akismet related fix and related comments 🚢
Fixes #42395
Proposed changes:
admin_menu
filter.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
On a self-hosted test site, with Jetpack Backup installed and active, assuming your site language is set to English:
PHP Notice: Function _load_textdomain_just_in_time was called <strong>incorrectly</strong>. Translation loading for the <code>jetpack-backup</code> domain was triggered too early.
This is also reproducible using a different site language on the current WP version, on self-hosted:
/wp-admin/options-general.php
/wp-admin/update-core.php
Then, apply this PR and follow the same steps. Those notices should be gone, and the VaultPress Backup admin menu item should still be visible.