wp-rocket icon indicating copy to clipboard operation
wp-rocket copied to clipboard

Closes #5207 on redirect loop

Open jeawhanlee opened this issue 3 years ago • 3 comments

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 avatar Jul 05 '22 17:07 jeawhanlee

@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 avatar Aug 16 '22 14:08 Mai-Saad

@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 .htacess is 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 avatar Sep 13 '22 17:09 jeawhanlee

@jeawhanlee You can enable Separate Cache for Mobile Devices to remove rewrite rules from htaccess

piotrbak avatar Sep 13 '22 17:09 piotrbak

@jeawhanlee Are those red cells still failing? There are lots of them 🤔

piotrbak avatar Sep 29 '22 11:09 piotrbak

@jeawhanlee While doing QA we found that on multisites where:

  1. the subsite is in a subdirectory &&
  2. 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.

vmanthos avatar Dec 02 '22 12:12 vmanthos

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.

piotrbak avatar Dec 16 '22 14:12 piotrbak

Open now as https://github.com/wp-media/wp-rocket/issues/3477 is fixed

jeawhanlee avatar Dec 20 '22 07:12 jeawhanlee

@vmanthos We can move this issue to QA done then, right?

piotrbak avatar Jan 10 '23 12:01 piotrbak

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.

vmanthos avatar Jan 10 '23 12:01 vmanthos