-
Notifications
You must be signed in to change notification settings - Fork 821
CRM: PHP notices: Translation loading for the zero-bs-crm domain was triggered too early. #3542 #44109
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
base: trunk
Are you sure you want to change the base?
Conversation
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! |
Code Coverage SummaryCoverage changed in 3 files.
Coverage check overridden by
I don't care about code coverage for this PR
|
0f11714
to
81e5f0d
Compare
@tbradsha interestingly - this also appears to fix this issue which I've improved the code slightly here |
$zbs = ZeroBSCRM::instance(); | ||
// init hook | ||
add_action( | ||
'init', |
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.
I'll note that we're changing the load order here; I'm not sure what implications this might have for extensions that register with CRM (e.g. Mail Campaigns or WooSync).
On the plus side, this sets the stage to clean up several lines in ZeroBSCRM::init_hooks()
to no longer use the init
hook.
// Pre-init Hook | ||
do_action( 'before_zerobscrm_init' ); | ||
|
||
// After all the plugins have loaded (THESE FIRE BEFORE INIT) | ||
add_action( 'plugins_loaded', array( $this, 'load_textdomain' ) ); // } Translations |
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.
Should we chop ZeroBSCRM::load_textdomain
now?
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.
we should!
return $zbs->database_server_info['db_engine_label']; | ||
|
||
// If $zbs is an object but database_server_info is not set, try to populate it. | ||
if ( is_object( $zbs ) && ! isset( $zbs->database_server_info ) ) { |
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.
Did you run into an issue with this? If $zbs
is not an object, we should probably return early.
Related, $zbs->database_server_info
will always be set if $zbs
is an object, given that it's initialized as an empty object. It's also populated as early as possible (within $zbs->verify_minimum_requirements()
).
In other words, if $zbs
is an object, the rest of the checks are unnecessary and the fallback scenario isn't possible.
|
||
// Activating the plugin directly loads the welcome wizard, so no need to move pages here. | ||
// Click the activate link for jetpack-crm directly | ||
$I->click( 'Activate', array( 'css' => 'tr[data-slug="jetpack-crm"] .activate a' ) ); |
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.
I presume this is because the $I->activatePlugin()
uses the bulk activation route? This might be irrelevant now that I've adjusted it to support bulk activation with one plugin.
global $zbs; | ||
$zbs->install(); | ||
zeroBSCRM_notifyme_createDBtable(); | ||
if ( class_exists( 'zeroBSCRM' ) && ! isset( $zbs ) ) { |
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.
Why do we need this check? We literally load the class file and we know it's not set. :^)
// Run installation if needed | ||
if ( isset( $zbs ) && is_object( $zbs ) && method_exists( $zbs, 'install' ) ) { | ||
$zbs->install(); | ||
if ( function_exists( 'zeroBSCRM_notifyme_createDBtable' ) ) { |
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.
Same thing here...if $zbs
is instantiated, that means it's run the __construct()
, which means it's run $zbs->includes()
, which means this function does exist. There's no way to get to this point and not have the function exist.
} | ||
|
||
// Run installation if needed | ||
if ( isset( $zbs ) && is_object( $zbs ) && method_exists( $zbs, 'install' ) ) { |
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.
This is overkill and not needed. :^)
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.
unless someone deletes the install function?
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.
Some comments to consider...
$this->assertWizardIsShown( $I ); | ||
} | ||
|
||
public function test_bulk_plugin_activation_skips_wizard( AcceptanceTester $I ) { |
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.
I suspect this doesn't show the wizard because technically the option already is set. So more accurately this is testing if the wizard shows on second load.
Fixes #3542
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No
Testing instructions: