:closes: Fix issue with warm up link for homepage #6742
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences🚀 Don’t miss a bit, follow what’s new on Codacy. Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more Footnotes
|
There was a problem hiding this comment.
Thank you for the PR! It helped me understanding the issue and the approach.
While this PR should solve the issue, may I suggest a different approach? Reading this PR gave me an idea, and I think it could be good to discuss it to have a "cleaner" fix I think.
Actually, the root cause of the issue is that, on fresh install, the warm_up_home is called before the options cache_mobile and do_caching_mobile_files are set! This is due to the fact that warm_up_home is triggered with the register_activation_hook (activate_plugin -> rocket_after_activation) while options are set on admin_init which occurs after.
To confirm this, I added the following logs:
function rocket_upgrader() {
// Grab some infos.
$actual_version = (string) get_rocket_option( 'version', '' );
// You can hook the upgrader to trigger any action when WP Rocket is upgraded.
// first install.
if ( ! $actual_version ) {
error_log(" MLT TEST - FIRST INSTALL");
do_action( 'wp_rocket_first_install' );
}
To spot when the options are set (in wp_rocket_first_install)
And
private function is_mobile( bool $is_warm_up = false ): bool {
error_log(" MLT TEST - " . (string) get_rocket_option( 'version', '' ));
error_log(" MLT TEST - " . (string) $this->options->get( 'cache_mobile', 'not here' ));
error_log(" MLT TEST - " . (string) $this->options->get( 'do_caching_mobile_files', 'not here' ));
if ( $is_warm_up ) {
return true;
}
return $this->options->get( 'cache_mobile', 0 ) && $this->options->get( 'do_caching_mobile_files', 0 );
}
Here is the output upon fresh install:
[26-Jun-2024 17:59:37 UTC] MLT TEST -
[26-Jun-2024 17:59:37 UTC] MLT TEST - not here
[26-Jun-2024 17:59:37 UTC] MLT TEST - not here
[26-Jun-2024 17:59:38 UTC] MLT TEST - FIRST INSTALL
This confirms the order of execution of the actions, hence the issue.
To solve the issue, ie. send desktop + mobile homepage on fresh install, and keep the existin behavior the rest of the time, I would suggest the following rewrite of inc/Engine/Media/AboveTheFold/WarmUp/Controller.php -> is_mobile
/**
* Check if separate cache for mobile is configured.
*
* @return bool
*/
private function is_mobile(): bool {
$plugin_version = (string) get_rocket_option( 'version', '' );
if ( ! $plugin_version ) { # We are warming up a fresh install. Options are not set yet.
return true;
}
return $this->options->get( 'cache_mobile', 0 ) && $this->options->get( 'do_caching_mobile_files', 0 );
}
|
Related test plan: https://wpmediaqa.testrail.io/index.php?/runs/view/864 |
|
@wp-media/qa-team The PR won't completely fix the issue, just improve it a bit (should warm up homepage sometimes on fresh installs, but not consistently). But we still expect that homepage might not be warmed_up systematically on fresh installs: the requests to the SaaS will be made and visible on Metabase, but the beacon might not be injected. (it's flaky, so not always reproducible, there is a race condition). I would say maybe we can make this a NoQA since there is no consistent change in behavior? NRT would be enough to ensure there are no warning/Errors (the code change is quite small, that should be quick). WDYT? |
|
I completed the test plan: https://wpmediaqa.testrail.io/index.php?/runs/view/864&group_by=cases:section_id&group_order=asc Two issues: |
|
The PR generally enhances the warmup with the following notes which will be handled on separate issues
|
Description
Fixes #6697
Documentation
User documentation
Explain how this code impacts users.
Technical documentation
Explain how this code works. Diagram & drawings are welcomed.
Type of change
New dependencies
List any new dependencies that are required for this change.
Risks
List possible performance & security issues or risks, and explain how they have been mitigated.
Checklists
Feature validation
Code style
Observability
Risks