Skip to content
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

Do not add URLs to the wpr_rocket_cache table if Preload is disabled #6278

Closed
viobru opened this issue Nov 21, 2023 · 1 comment · Fixed by #6305
Closed

Do not add URLs to the wpr_rocket_cache table if Preload is disabled #6278

viobru opened this issue Nov 21, 2023 · 1 comment · Fixed by #6305
Assignees
Labels
module: preload priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@viobru
Copy link
Contributor

viobru commented Nov 21, 2023

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version ✔️
  • Used the search feature to ensure that the bug hasn’t been reported before ✔️

Describe the bug
Currently, any URL that is visited by a real user is added to the wpr_rocket_cache table and automatically assigned a “Complete” status even when the Preload is disabled. This causes the DB to grow too large in some cases.

To Reproduce
Steps to reproduce the behavior:

  1. Make sure the Preload is disabled
  2. Visit some URLs while not logged-in
  3. Check the wpr_rocket_cache table in the DB and you'll find those visited URLs added to it with a "Complete" status

Expected behavior
The table itself is not necessary for the caching process to run effectively, so no URLs should be added to it when the Preload is disabled.

Temporary fix - The snippet below will prevent all URLs from being added to the table if the preload is disabled:

// prevents adding URL to the wpr_rocket_cache table if Preload is disabled
function stop_adding_top_preload( array $regexes ) : array {

	$new_regex = $_SERVER['REQUEST_URI'];
	
	$options = get_option('wp_rocket_settings', []);

	//only if preload is disabled
	if( $options['manual_preload'] == 0) {

		// add the currently visited URL to the exclusions array
		$regexes[] = $new_regex;

	}

return $regexes;
}

add_filter( 'rocket_preload_exclude_urls', 'stop_adding_top_preload');

Screenshots
N/A

Additional context
HS ticket: https://secure.helpscout.net/conversation/2405513676/451397/
Slack thread: https://wp-media.slack.com/archives/C08N8J6VC/p1698938940269529

Acceptance Criteria (for WP Media team use only)

  1. When preload is disabled, we should not add URLs to the cache table
  2. When enabling preload, there should not be any regression with fetching the sitemaps
  3. When the preload is enabled, URLs should be added to the table without regression
@viobru viobru changed the title Do not add URLS to the wpr_rocket_cache table if Preload is disabled Do not add URLs to the wpr_rocket_cache table if Preload is disabled Nov 21, 2023
@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement module: preload priority: low Issues that can wait priority: medium Issues which are important, but no one will go out of business. and removed priority: low Issues that can wait labels Dec 3, 2023
@Miraeld Miraeld self-assigned this Dec 4, 2023
@Miraeld
Copy link
Contributor

Miraeld commented Dec 4, 2023

Reproduce the problem

To reproduce the issue, follow the described steps in the issue.

Identify the root cause

The issue is that we are not checking if the manual preload is true or false here:

public function add_preload_excluded_uri( $regexes ): array {
$preload_excluded_uri = (array) $this->options->get( 'preload_excluded_uri', [] );
if ( empty( $preload_excluded_uri ) ) {
return $regexes;
}
return array_merge( $regexes, $preload_excluded_uri );
}

Scope a solution

To solve the issue, we could add this:

$request_uri = filter_var( wp_unslash( $_SERVER['REQUEST_URI'] ), FILTER_SANITIZE_URL );

if( (bool) !get_rocket_option( 'manual_preload', true ) ) {
	// add the currently visited URL to the exclusions array
	$regexes[] = $request_uri;
}

to the mentioned function.

Effort estimation:

S

Is a refactor needed in that part of the codebase?

No

@Miraeld Miraeld assigned Miraeld and unassigned Miraeld Dec 4, 2023
Miraeld added a commit that referenced this issue Dec 5, 2023
Miraeld added a commit that referenced this issue Dec 6, 2023
@piotrbak piotrbak added this to the 3.15.7 milestone Dec 14, 2023
github-merge-queue bot pushed a commit that referenced this issue Dec 29, 2023
@remyperona remyperona mentioned this issue Jan 4, 2024
wordpressfan pushed a commit that referenced this issue Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: preload priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
3 participants