New Preload: fix qa feedback

[CrochetFeve0251]

Description

Please include a summary of the change and which issue is fixed/closed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #(issue number)

Type of change

Please delete options that are not relevant.

• [ ] New feature (non-breaking change which adds functionality)
• [ ] Bug fix (non-breaking change which fixes an issue)
• [ ] Enhancement (non-breaking change which improves an existing functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ ] This change requires a documentation update

Is the solution different from the one proposed during the grooming?

Please describe in this section if there is any change to the solution, and why.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• [ ] Test A
• [ ] Test B

Checklist:

Please delete the options that are not relevant.

• [ ] My code follows the style guidelines of this project
• [ ] I have performed a self-review of my own code
• [ ] I have commented my code, particularly in hard-to-understand areas
• [ ] I have made corresponding changes to the documentation
• [ ] My changes generate no new warnings
• [ ] I have added tests that prove my fix is effective or that my feature works
• [ ] New and existing unit tests pass locally with my changes
• [ ] Any dependent changes have been merged and published in downstream modules

[Mai-Saad]

@CrochetFeve0251 Thanks for the update. Kindly find the notes/Qs below existing on commit f4f67dfde:

• [x] 1- if we enable a separate mobile cache and while preload is in the background, if we visit the desktop of any page (that is not completed yet) then it will be completed while only the desktop version in cache (however, if we visited mobile version then it will be added in cache) => as per discussion, we need to trigger the other visit too. Is it doable?

• [x] 2- Will we change URLs in the database while changing permalinks or just changing its status to pending? @piotr what do you think? update seems we are handling the request itself for / case .. however, we aren't handling the case when the permalink is totally changed now (in the table we are using an old structure and requests with an old structure).at some point this may lead to having 2 different URLs in table for the same page

• [x] 3- Currently, with a fresh install, we take some time to add the home page to caching table. can it be added instantly?

• [x] 4- Sometimes, after a fresh install, preload adds nothing to cache with/without error in logs.. this needs further investigation and keeping an eye on it using different test sites if there was an error it's probably 22-Jul-2022 14:42:49 UTC] WordPress database error Table 'new_rocketlabsqa_ovh_AzP4U3TX.wpr_rocket_cache' doesn't exist for query SELECT id FROM wpr_rocket_cacheWHERElast_accessed<= date_sub(now(), interval 1 month) made by do_action_ref_array('rocket_preload_clean_rows_time_event'), WP_Hook->do_action, WP_Hook->apply_filters, WP_Rocket\Engine\Preload\Cron\Subscriber->remove_old_rows, WP_Rocket\Engine\Preload\Database\Queries\Cache->remove_all_not_accessed_rows, WP_Rocket\Engine\Preload\Database\Queries\Cache->get_old_cache and sometimes we have this as well (as per discussion, this error this is the fix we did waiting for BerlinBD to create methods to modify cache) [22-Jul-2022 18:28:12 UTC] WordPress database error Duplicate entry 'https://newer.rocketlabsqa.ovh/test' for key 'url' for query INSERT INTO wp_wpr_rocket_cache (url, status, modified, last_accessed) VALUES ('https://newer.rocketlabsqa.ovh/test', 'pending', '2022-07-22T18:28:12Z', '2022-07-22 18:28:12') made by do_action_ref_array('action_scheduler_run_queue_preload'), WP_Hook->do_action, WP_Hook->apply_filters, WP_Rocket\Engine\Common\Queue\PreloadQueueRunner->run, WP_Rocket\Engine\Common\Queue\PreloadQueueRunner->do_batch, ActionScheduler_Abstract_QueueRunner->process_action, ActionScheduler_Action->execute, do_action_ref_array('rocket_preload_job_parse_sitemap'), WP_Hook->do_action, WP_Hook->apply_filters, WP_Rocket\Engine\Preload\Frontend\Subscriber->parse_sitemap, WP_Rocket\Engine\Preload\Frontend\FetchSitemap->parse_sitemap, WP_Rocket\Engine\Preload\Database\Queries\Cache->create_or_nothing, WP_Rocket\Dependencies\Database\Query->add_item

• [x] 5- If we enabled Cookie Notice & Compliance for GDPR / CCPA plugin, click clear and preload then access any page with completed status => the cookie notice won't be displayed although it is displayed in nowprocket post. any ideas?

• [x] 6- While transitional or reader AMP => created only normal files for all except home (for home no mobile at all), and no amp for all => shouldn't we have a mobile version for home? will we create the amp folder before a real visit (it is currently created with the manual visit)

• [x] 7- Using this filter to control the number of rows to be deleted from the actions table seems not working, is it the correct filter or do we need to handle something there? (it shall remove 500 items that exist more than 1 min if we set the period filter to 60, however, only 20 items are deleted now)

add_filter( 'rocket_action_scheduler_clean_batch_size', function( $entries ){
		return 500;
		} )

used filter for the duration was

add_filter( 'rocket_action_scheduler_retention_period', function( $seconds ){
	return 60;
	} );

• [ ] 8- When:
  1. Separate cache for mobile devices is enabled.
  2. A page is not cached.
  3. There isn't an entry in the database for that URL yet. Visiting the URI with desktop or mobile will create the cache for the respective device type but not for the other one, i.e. a visit from a desktop won't preload the mobile version and vice versa.

Note: tested on https://newer.rocketlabsqa.ovh/, while for point 4 tested with https://new.rocketlabsqa.ovh/ too

• [x] 9- On kinsta site (single site, credentials on BW), noticed the following with commit 9c5392e1e

• Error + notice in debug.log with a fresh install => same with Varnish too on http://wprocket-test.odns.fr/wp-admin and on wp-engine https://wprocketyo.wpengine.com/wp-admin/

• If the user accesses the page before being completed, it will be stuck in in-progress. as per @piotrbak is_cached function won't work on self-cached servers

• we need to guard ourselves if the database table was not created for any reason

• URL with a query string in the table (it won't be cached)

• If we enable RUCSS along with preload, then we access the page that is completed in the cache, it will be missed in Kinsta headers, while it should be hit => same on Varnish

• If we switched theme while preload is enabled, then we access the page that is completed in the cache, it will be missed in Kinsta headers, while it should be hit

• [x] 10- Cache auto cleared when it shouldn't happen in the following scenario 1- Fresh install to branch 2- Have completed entries in the cache table 3- Enable analytics 4- Save settings 5- Check completed entries => it will be cleared

• [ ] 11- Mobile-specific cache won't be created for some URLs if you follow these steps:

1. Enable the separate cache file for mobile.
2. While there are pending jobs disable the mobile cache - this doesn't clear the cache.
3. Enable mobile cache once the jobs are completed.

For the jobs that were pending the mobile-specific cache won't be created.

• [ ] 12- After disabling the separate cache for mobile devices (status completed for all URLs) the cache is cleared and it takes quite some time for preload to start.

Also, the homepage is the last URL cached.

• [x] 13- With PHP 8.1 with a fresh install (activation of the plugin using wp-cli), I got the following in the debug.log:

PHP Warning:  Undefined property: wpdb::$wpr_rocket_cache in /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-includes/wp-db.php on line 740

HP Notice:  Trying to get property 'id' of non-object in /var/www/vasilis.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Preload/Database/Queries/Cache.php on line 165
https://github.com/wp-media/wp-rocket/blob/9c5392e1e4d580951f86a310306e97ecc2db2942/inc/Engine/Preload/Database/Queries/Cache.php#L165

• [x] 14- Clear used CSS of certain URL while preload is enabled, won't preload this URL ( won't change status to pending in table and recreate cache file)
• [x] 15- In a fresh install, Yoast's sitemaps aren't picked up because of the following conditional: https://github.com/wp-media/wp-rocket/blob/9c5392e1e4d580951f86a310306e97ecc2db2942/inc/ThirdParty/Plugins/SEO/Yoast.php#L51-L53
• [x] 16- When a URL is added to the Always Purge URL(s) text area (Advanced Rules tab), after purging the cache of a post, an incorrect URL containing a // is added to the DB, e.g.: https://newmulti.rocketlabsqa.ovh//aspernatur-impedit-repudiandae-impedit and that one is not preloaded.

The double // is added here: https://github.com/wp-media/wp-rocket/blob/6b0460963fc8371ee0c7c4c1fb2f93362ea5c6b3/inc/common/purge.php#L129

• [x] 17- rocket_preload_job_check_finished is added three times in some occasions:

• [x] 18- Purge this URL for a certain page, will delete the page from the cache and won't change its status in the database => status shall change in the database too

• [ ] 19- When WPML is enabled the URL of the additional language has a //, e.g.: https://vasilis.rocketlabsqa.ovh/el//wp-sitemap.xml

• [ ] 20- When Yoast is enabled alongside WPML we keep adding WordPress's sitemaps to be parsed. This is unnecessary because Yoast already contains translated URLs.

• [x] 21- When no sitemap is in place and WordPress's sitemap has been disabled we don't crawl the homepage.

• [x] 22- Homepage crawl adds external URLs to be preloaded.

• [x] 23- Homepage crawl doesn't pick relative URLs.

• [x] 24- Cache isn't preloaded after activating Remove Unused CSS for the first time

[CrochetFeve0251]

@Mai-Saad concerning point 2 when a permalink change it will create a new row in the database. Some code executes on 404 error to drop the old permalink if it is accessed.

[piotrbak]

@CrochetFeve0251 @Mai-Saad Regarding 2nd I believe that redirection to the correct version will happen (thanks Mai), not 404.

I think we have two options here:

1. Save in the db version with/without permalinks
2. Decide whether we need to add the / just before sending the request

[CrochetFeve0251]

@CrochetFeve0251 @Mai-Saad Regarding 2nd I believe that redirection to the correct version will happen (thanks Mai), not 404.

I think we have two options here:

1. Save in the db version with/without permalinks

2. Decide whether we need to add the `/` just before sending the request

The option 2. is already implemented for trailing slashes.