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

The /feed/ url is cached and optimized by default #5599

Closed
4 tasks done
alfonso100 opened this issue Nov 29, 2022 · 11 comments · Fixed by #5604
Closed
4 tasks done

The /feed/ url is cached and optimized by default #5599

alfonso100 opened this issue Nov 29, 2022 · 11 comments · Fixed by #5604
Assignees
Labels
effort: [XS] < 1 day of estimated development time module: cache priority: high Issues which should be resolved as quickly as possible type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@alfonso100
Copy link
Contributor

alfonso100 commented Nov 29, 2022

Before submitting an issue please check that you’ve completed the following steps:
yes- Made sure you’re on the latest version
yes - Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug
On WP Rocket 3.12.3.2 /feed/ is cached and optimized by default. rolling back to 3.11.5 fixes the issue.
this might be affecting /wp-json/ too and probably other paths. (untested)

To Reproduce
Steps to reproduce the behavior:

  1. Install 3.12.3.2
  2. Visit the /feeds URL and see caching and optimization applied
  3. Rollback to 3.11.5 and clear the cache
  4. Visit the /feeds URL again, caching and optimization are NOT applied

Expected behavior
Feeds should not be cached unless the user opts for this. And optimizations shouldn't be applied in any case.

Screenshots
IM3zjAV

https://i.imgur.com/IM3zjAV.jpg

Additional context
ticket where /feed/ is cached https://secure.helpscout.net/conversation/2079965213/385018/
probably related, about /wp-json/: https://secure.helpscout.net/conversation/2082507658/385784?folderId=273761

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@Tabrisrp
Copy link
Contributor

related commit c0fb3c9 in 3.12.3.2

@Tabrisrp Tabrisrp added type: bug Indicates an unexpected problem or unintended behavior module: cache labels Nov 29, 2022
@Tabrisrp
Copy link
Contributor

the user_trailingslashit() should only be applied to user added patterns, instead of applying it in the get_rocket_cache_reject_uri(), to avoid adding extra slash to some of the patterns.

This could be done by using this function before saving the value to the database with the pre_update hook for example, or during sanitize of the option

@jeawhanlee jeawhanlee self-assigned this Nov 30, 2022
@NataliaDrause
Copy link
Contributor

Related: https://secure.helpscout.net/conversation/2083011318/385873?folderId=3864740

wp-json are being cached.

@alfonso100
Copy link
Contributor Author

@jeawhanlee
Copy link
Contributor

We will need to take out these lines:

$uris = array_map(
function ( $uri ) {
return user_trailingslashit( $uri );
},
$uris
);
so as to stop trailing slash being appended to non user added patterns.

Match only user added patterns to permalink structure

When settings is saved
We will hook a method to pre_update_option_{$option} that returns the updated pattern values of cache_reject_uris after matching with permalink structure in https://github.com/wp-media/wp-rocket/blob/develop/inc/Engine/Cache/Config/ConfigSubscriber.php so as for this to also reflect in the UI as per discussion with @Tabrisrp.

When Permalink structure changes
We will add a new filter to get_rocket_cache_reject_uri function on this line that will filter the uris with the permalink structure, then hook to the same method above in https://github.com/wp-media/wp-rocket/blob/develop/inc/Engine/Cache/Config/ConfigSubscriber.php that way when permalink_structure_changed hook fires and rocket_generate_config_file() is called, get_rocket_cache_reject_uri will also be called here and new filter will be applied.

We also need to update the cache_reject_uris that uses the new permalink structure to reflect in the UI , to do that we will edit this method

public function regenerate_config_file() {
to update the cache_reject_uri option to use the current permalink structure just before we call the rocket_generate_config_file() function.

@GeekPress GeekPress added needs: grooming priority: high Issues which should be resolved as quickly as possible labels Dec 1, 2022
@jeawhanlee jeawhanlee removed their assignment Dec 1, 2022
@engahmeds3ed
Copy link
Contributor

I think we need to make it simpler, I believe the first point is not needed, using the hook get_rocket_option_cache_reject_uri and apply the code on its value like the following code, this will reflect in the UI also:

add_filter( 'get_rocket_option_cache_reject_uri', function( $value ){
	if ( empty( $value ) ) {
		return $value;
	}
    
	return array_map(
		function ( $uri ) {
			return user_trailingslashit( $uri );
		},
		$value
	);
} );

what do u think?

@jeawhanlee
Copy link
Contributor

Reproduce the problem ✅

Scope a solution ✅

Regarding @engahmeds3ed comment above and discussion with @Tabrisrp to address the UI, We will need to take out these lines:

$uris = array_map(
function ( $uri ) {
return user_trailingslashit( $uri );
},
$uris
);
then update WP_Rocket\Engine\Cache\Config:: ConfigSubscriber and add a new call back that will hook to get_rocket_option_cache_reject_uri that way, non user added pattern will be preserved and changes will reflect in the UI also.

Estimate the effort ✅

[XS]

@jeawhanlee jeawhanlee added effort: [XS] < 1 day of estimated development time and removed needs: grooming labels Dec 1, 2022
@jeawhanlee jeawhanlee self-assigned this Dec 1, 2022
@Mai-Saad Mai-Saad added this to the 3.12.3.3 milestone Dec 1, 2022
@Tabrisrp
Copy link
Contributor

Tabrisrp commented Dec 1, 2022

This solution won't be reflecting the correct value in the DB when a value was first added then saved. That's why it needs to happen before saving the value, not later

@engahmeds3ed
Copy link
Contributor

After a discussion with @Tabrisrp we agreed on that using get_rocket_option_cache_reject_uri will reflect on the UI and in every place we get this option and it won't hurt to reflect that on the database also.

So in the same class WP_Rocket\Engine\Cache\Config:: ConfigSubscriber we will need to add a callback for the pre_update hook:

'pre_update_option_' . WP_ROCKET_SLUG     => [ 'change_cache_reject_uri_with_permalink', 10, 2 ],

then

public function change_cache_reject_uri_with_permalink( $value, $old_value ) {
    if ( $old_value['cache_reject_uri'] === $value['cache_reject_uri'] ) {
        return $value
    }
    return $this->match_pattern_with_permalink_structure( $value['cache_reject_uri'] );
}

what do u think @jeawhanlee ?

@webtrainingwheels
Copy link

https://secure.helpscout.net/conversation/2085793083/386604?folderId=377611
FYI, this is affecting some other scenarios as well.
The customer is trying to exclude /index.php(.*) to solve this issue, and it no longer works.

vmanthos pushed a commit that referenced this issue Dec 8, 2022
…2.3.2 (#5604)

* Remove user_trailingslashit wrapper
* Update to match with permalink structure
* Updated test fixture
* Updated class
* Added tests
* use pre_update_option filter to modify pattern before saving
* Update tests
* Updated tests
* Updated unit test
* fixed tests
* Update inc/Engine/Cache/Config/ConfigSubscriber.php
* Use options api and data classes
* Updated unit test
* Update to accept trailing slash per line with permalinks having none
* Updated tests
* Updated to exclude patterns with index.php from respecting permalink structure
* Updated tests
* Update inc/Engine/Cache/Config/ConfigSubscriber.php
* Update inc/Engine/Cache/Config/ConfigSubscriber.php
* phpcs fix
* make fixture readable
* Updated fixtures
Co-authored-by: Rémy Perona <remy@wp-rocket.me>
Co-authored-by: Ahmed Saed <eng.ahmeds3ed@gmail.com>
Tabrisrp pushed a commit that referenced this issue Dec 8, 2022
…2.3.2 (#5604)

* Remove user_trailingslashit wrapper
* Update to match with permalink structure
* Updated test fixture
* Updated class
* Added tests
* use pre_update_option filter to modify pattern before saving
* Update tests
* Updated tests
* Updated unit test
* fixed tests
* Update inc/Engine/Cache/Config/ConfigSubscriber.php
* Use options api and data classes
* Updated unit test
* Update to accept trailing slash per line with permalinks having none
* Updated tests
* Updated to exclude patterns with index.php from respecting permalink structure
* Updated tests
* Update inc/Engine/Cache/Config/ConfigSubscriber.php
* Update inc/Engine/Cache/Config/ConfigSubscriber.php
* phpcs fix
* make fixture readable
* Updated fixtures
Co-authored-by: Rémy Perona <remy@wp-rocket.me>
Co-authored-by: Ahmed Saed <eng.ahmeds3ed@gmail.com>
@worldwildweb
Copy link
Contributor

/wp.serviceworker was also affected 3.12.3.3 fixes the issue.

@GeekPress GeekPress changed the title The /feed/ url is cached and optimized by default on 3.12.3.2 The /feed/ url is cached and optimized by default Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [XS] < 1 day of estimated development time module: cache priority: high Issues which should be resolved as quickly as possible type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants