wp-rocket
wp-rocket copied to clipboard
wp-media
3.16 - Preload when enabling and updating WP Rocket
Context
As a user, I want WP Rocket to identify internal links on the homepage so that those pages can then be pre-optimized for LCP/ATF.
Scope a solution
Create a new Engine\Media\AboveTheFold\Warmup space
add a Controller class, with a dependency on the Jobs\Manager class
- add a method
fetch_linksthat will send a request to the homepage, get the HTML, find the first 10 internal links, and send them to the jobs manager - the number of links is modifiable with a new filter
rocket_atf_warmup_links_number - the method will bail-out if the filter to disable the ATF optimization feature is disabled
- add tests
add a Subscriber class, with a dependency on the Controller created above
- The subscriber will listen to the following actions:
wp_rocket_upgrade,permalink_update,switch_theme - For each one, a callback method will be used to do the fetch links
- The callback for
wp_rocket_upgradeshould only trigger when upgrading from a version lower than 3.16 - add tests
add WP_Rocket\Engine\Media\AboveTheFold\Activation\Activation & WP_Rocket\Engine\Media\AboveTheFold\Activation\ServiceProvider
- The Activation class will have a dependency on the controller
- add callback on this action:
rocket_after_activation - the callback method will call the fetch_links() method from the controller
Acceptance criteria
template to be refined with QA
- Update the plugin to 3.16 from a previous version. Check the DB against the expected result.
- Perform a fresh install. Check the DB against the expected result.
Effort estimation:
Effort M
preload le homepage and extract internal links
Not sure what this means and how it will be achieved 🤔 How do we identify the internal links from the homepage? As mentioned here, we'll need a filter on the number of links.
I updated some of the grooming but it's not complete yet, we need to figure out how to update the existing job manager to send the found links to the SaaS, with a yet-to-be-defined query string to identify them.
Maybe @jeawhanlee can help, since he implemented the refactor of this part.
add a method fetch_links that will send a request to the homepage, get the HTML, find the first 10 internal links, and send them to the jobs manager
Total newbie question here: don't we have sitemaps available that we could re-use here instead of requesting the homepage?
Sitemaps parsing would increase the complexity even more, and I'm not sure it would provide us more relevant URLs. We can usually consider that the first links we find on a website are the one that will be the most used, which might not be the case in the sitemap.
@Tabrisrp For this issue, can you clarify something please: does this implementation rely on a DB table?
It seems to me that the suggested solution is that, on update/activation/install, we trigger the fetch_link, and we can instantly send those "jobs" to the SaaS, right? Since there is no "get result" part, we don't need to store anything. Is that correct? I am asking because in the EPIC card, it is still mentioned that, when fetching homepage links, we should add them to the DB: here But I don't think this is relevant anymore: entries in the DB are to be created when injecting the beacon script, not when sending jobs to the SaaS. Correct?
(cc @piotrbak )
@mostafa-hisham, for the new WP Rocket version to be able to communicate properly with the SaaS, is it correct that the following changes should be made?
- When sending a RUCSS job, in the request payload,
config["optimization_list"]=['rucss']must be set. - When sending a LCP/ATF job, in the payload request,
config["optimization_list"]=[]must be set.
For backward compatibility, if config["optimization_list"] is not set, it will default to ['rucss'] on the SaaS side, but we should not rely on this mechanism for the new version.
If this is correct, then this change should be tackled as part of this issue as well.
To whoever picks up this issue, it could make sense to divide it in 3 successive PRs:
- First one to manage the fetch_link method on updatE/activation/install but without sending the results to the SaaS yet -> AC would be to check that on update/activation/install, we find the correct links from the homepage. Instead of sending them to the SaaS yet, they could be outputted as logs for testing purposes.
- Second one would be to replace the log by actually sending to the SaaS.
- Third one would be to change the request to the SaaS for RUCSS by adding
config["optimization_list"]=['rucss']to the payload.
@Tabrisrp For this issue, can you clarify something please: does this implementation rely on a DB table?
It doesn't need it
It seems to me that the suggested solution is that, on update/activation/install, we trigger the fetch_link, and we can instantly send those "jobs" to the SaaS, right? Since there is no "get result" part, we don't need to store anything. Is that correct? I am asking because in the EPIC card, it is still mentioned that, when fetching homepage links, we should add them to the DB: But I don't think this is relevant anymore: entries in the DB are to be created when injecting the beacon script, not when sending jobs to the SaaS. Correct?
It'd mean that we should also fetch the homepage in all scenarios when image data is cleared (using button, etc.)
Yes, but in any case we should do this I think. Otherwise if we store the links somewhere, they would become outdated, no?
@Tabrisrp @Miraeld I think in that case we are missing a trigger in the current grooming: the fetch_link process should be triggered as well when clicking the "Clear Image Data" button.
@MathieuLamiot It needs to be triggered in all cases when the data is cleared. Maybe we should not bound it with the specific triggers, rather the action of clearing the data?
We have to trigger the fetch_link on some triggers like wp_rocket_upgrade, rocket_after_activation, as those cases won't be covered if we just trigger with the data deletion.
For the fetch_link on delete data, I think we could to do it in truncate_atf method. This is already a callback of switch_theme, permalink_structure_changed, rocket_domain_options_changed 🤔
Maybe we could just use this method for everything: delete rows if they exist and start the fetch_link process ; that would be the only method to declare as a callback.
Brought back to In Progress as there is a new AC: Bail out from this process when RUCSS is enabled. (Discussed here)
@jeawhanlee I just remembered there was already a dedicated issue for this accetpance criteria Bail out from this process when RUCSS is enabled..
It is here: https://github.com/wp-media/wp-rocket/issues/6435
Up to you, either you add it into this PR, and we close 6435, either you manage both separately. In any case, could be nice to check you're aligned with the grooming there :)
@MathieuLamiot The grooming there is fine, however, Since we are already touching that same module in this PR and both share the same test case, we can handle it in this PR and close #6435 .
The only known issue to this day is ready to be merged: https://github.com/wp-media/wp-rocket/pull/6517 So let's merge it before running the above test plan. So that we only have to validate the whole thing once.
@jeawhanlee, we now fetch 10 links including homepage. According to requirements doc (and confirmed with @piotrbak), we should have 10 links apart from homepage, so 11 links in total. Can you see this through? Thank you.
@jeawhanlee, one more thing. it seems the homepage is not recognised as such, maybe because of query string. I believe this is also something in connection to this PR, if you can look this up. 🙏
@piotrbak, @MathieuLamiot, using filter rocket_above_the_fold_optimization with value -ve we get fatal error:
Stack trace:
#0 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): {closure}()
#1 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(205): WP_Hook->apply_filters()
#2 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Context/Context.php(26): apply_filters()
#3 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Saas/Admin/Notices.php(199): WP_Rocket\Engine\Media\AboveTheFold\Context\Context->is_allowed()
#4 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Saas/Admin/Subscriber.php(162): WP_Rocket\Engine\Saas\Admin\Notices->add_localize_script_data()
#5 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): WP_Rocket\Engine\Saas\Admin\Subscriber->add_localize_script_data()
#6 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(205): WP_Hook->apply_filters()
#7 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/admin/ui/enqueue.php(27): apply_filters()
#8 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): rocket_add_admin_css_js()
#9 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters()
#10 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(517): WP_Hook->do_action()
#11 /var/www/new.rocketlabsqa.ovh/htdocs/wp-admin/admin-header.php(125): do_action()
#12 /var/www/new.rocketlabsqa.ovh/htdocs/wp-admin/admin.php(239): require_once('...')
#13 /var/www/new.rocketlabsqa.ovh/htdocs/wp-admin/options-general.php(10): require_once('...')
#14 {main}
thrown in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/code-snippets/php/snippet-ops.php(582) : eval()'d code on line 2
Do we want to fix this? We're safe with other wrong values. Seems just this one remains unhandled.