wp-rocket
wp-rocket copied to clipboard
Closes #5207 on redirect loop
Description
This PR will fix the redirect loop by redirecting with wordpress when permalink settings and url do not match.
Fixes #5207
Type of change
- [x] Enhancement (non-breaking change which improves an existing functionality)
Checklist:
Please delete the options that are not relevant.
- [x] My code follows the style guidelines of this project
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] My changes generate no new warnings
@jeawhanlee Thanks for the update. Please check tabs NGINX round 2 and Apache here, we will need to handle red cells. Orange cell(s) needs to be discussed with @piotrbak to see if anything is needed there.
Notes:
- Checking on Apache multisite with subdomain is out of scope till test env is set up.
- For enforcing/remove trailing slash plugins, we already have bug on the trunk which shall be fixed before checking it with the current PR
@Mai-Saad @piotrbak From my findings on Apache, I believe single site test cases fails after first visit of the page because the page cache is no longer served via php but .htaccess is at play already so the cache files are served using the rewrite rules.
- However Clearing cache from the admin or manually deleting the gzip file in the cache folder or the entire related cache folder makes the page served again via php and the redirect is triggered once again.
according to @piotrbak and @vmanthos we don't need to handle this case if
.htacessis at work. @Mai-Saad Can you please confirm the redirections are ok when the cache is cleared from admin. 🙏
Also for failed test cases on Muti-site with sub-folder setup for both Apache & Nginx, when this PR went through QA, Multi-site with sub-folder setup were by-passed so we don't redirect. but having removed the by-pass in the recent commits, Main site works fine but sub-sites fail, I think we might need to handle this from the area of our config file. As it is, we are not loading the corresponding config file for sub-sites of Multi-sites set up in a sub-folder as discussed with @vmanthos
@piotrbak Can you please check the orange cells here in the Apache tabs if we need to handle them 🙏
@jeawhanlee You can enable Separate Cache for Mobile Devices to remove rewrite rules from htaccess
@jeawhanlee Are those red cells still failing? There are lots of them 🤔
@jeawhanlee While doing QA we found that on multisites where:
- the subsite is in a subdirectory
&& - the permalinks there don't have a trailing
/
caching/optimizations are not applied. This is because maybe_allow_wp_redirect() returns here:
https://github.com/wp-media/wp-rocket/blob/aa66a000de1602f0e1d2b1a0587c5e0d879e1276/inc/classes/Buffer/class-cache.php#L79
Logging the variables there, I got these:
permalink_structure: /blog/%postname%
permalink_last_char:
request_uri: /multisite/green/
request_uri_last_char: /
request_uri_last_char (line 719): /
In this scenario, $permalink_last_char !== $request_uri_last_char evaluates to true:
https://onlinephp.io?s=bcxBCoAgEEDRfdAdJggsCDxASavOIWIDVqY2aeDtq2XQ9j_4wxhMKAvOYXIRCbJPBNrPCAYJO0C3-lyVRR2QdmUXt0mrzii1UQQCGOsfIzwSPjHR8lX-8qVIzmkPze-jEgL-B21_Aw%2C%2C&v=8.1.12
Can you please look into this? 🙏
PS: You can check this on MEGA.
Issue is blocked by https://github.com/wp-media/wp-rocket/issues/3477.
If we won't come up with any solution to the above, we should bypass this enhancement for multisites at all.
Open now as https://github.com/wp-media/wp-rocket/issues/3477 is fixed
@vmanthos We can move this issue to QA done then, right?
We can move this issue to QA done then, right?
If you decide that the issue I've mentioned previously (orange cells here) isn't something we should fix, then yes.