-
Notifications
You must be signed in to change notification settings - Fork 228
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
Make sure that we cache only valid posts/pages urls #7236
base: develop
Are you sure you want to change the base?
Make sure that we cache only valid posts/pages urls #7236
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 preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @wordpressfan, for this PR.
Exploratory and Test Plan execution went well.
testrail-report-696.pdf
I think we need to wait a bit before merging this PR because we got some issues with taxonomy pages PR and the solution here is exactly the same. Those issues are:
but now this will happen with posts so I'd suggest having a filter to enable/disable this feature when needed (that's a product decision) |
In general, we'll need a filter here and some more testing as we know about the new limitations |
Definitely, let's go with adding a filter around this whole functionality once 3.19-prealpha is released internally 👌 |
@wordpressfan I did a refactor and added the filter if you want to have a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't approve my own PR, but seems valid from my side, just two questions.
Thanks Boss.
Description
Fixes https://github.com/wp-media/wp-rocket.me/issues/4456
This PR will make sure that we cache only
Type of change
Detailed scenario
For posts, pages, and even custom posts in some cases when we add any string inside the url, we still cache those pages because they don't return 404 error, here we make sure that those urls are not cached at all.
More details are inside the issue itself.
Technical description
Documentation
We compare the current page/post urls with their correct urls and stop caching for those not valid ones.
New dependencies
No
Risks
We have this code exactly like what we handled taxonomies so I hope we don't have a false cases that we detect the valid urls correctly.
Mandatory Checklist
Code validation
Code style
Unticked items justification
Checking tests now.
Additional Checks