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

[language filter] Review HTTP redirect codes #11206

Merged

Conversation

andrepereiradasilva
Copy link
Contributor

@andrepereiradasilva andrepereiradasilva commented Jul 19, 2016

Pull Request for Issue #11165.

Summary of Changes

This PR reviews the HTTP status code of the redirects in the language filter plugins (current all are non cached 301 Moved Permanently).

After this PR the behaviour is the following:

  • /defaultlang* to /* (remove language code is set to Yes) = 301 Moved Permanently
  • /* to /defaultlang* (remove language code is set to No) = 301 Moved Permanently (not cached)
  • /* to /otherlang* (cookie language based redirect) = 302 Found

Testing Instructions

  1. Use latest staging
  2. Apply patch
  3. Test in a multilanguage site the behaviour described above by testing all combinations in global config (SEF / non SEF / etc) and language filter plugin parameters.
  4. Do a code review

More info

See discussion in #11196

@mbabker
Copy link
Contributor

mbabker commented Jul 19, 2016

What's the benefit to not caching the second scenario? Something in my gut tells me this isn't the most optimal behavior.

@ggppdk
Copy link
Contributor

ggppdk commented Jul 19, 2016

Works:

Remove default language CODE : No (language filter plugin)
code_on

Remove default language CODE : Yes (language filter plugin)
code_off

All redirections that do not involve default language get a 302 code

@ggppdk
Copy link
Contributor

ggppdk commented Jul 19, 2016

I have tested this item ✅ successfully on 38b92ae

See my above comment


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11206.

@ggppdk
Copy link
Contributor

ggppdk commented Jul 19, 2016

@mbabker

no, we can not allow any redirection to be cached, that is what we fixed recently

e.g. using language code in the URL for default language

  • user visits homepage URL with no language code (thus user does not have language cookie yet), the user is redirected to home page with language code and gets 301 code, informing browser / search engines that 2nd URL is appropriate in place of the other
  • now user switches to other language and clicks on banner with home page URL, he get redirected to homepage with code 302,

now in this last case, we tell browser to cache it then we break the language switching again, that what we recently fixed

@mbabker
Copy link
Contributor

mbabker commented Jul 19, 2016

Something still doesn't feel right but you're all the ones dealing with this behavior so I leave it to your capable hands.

@ggppdk
Copy link
Contributor

ggppdk commented Jul 19, 2016

@mbabker

Something still doesn't feel right ...

you are right that it would be nicer to allow caching of some redirects e.g. cache 301 for default language,
but we also need to support the language switching with all possible configurations ...

i just say that this PR does not deal with the topic that you discuss, other PR have dealt with the above,

  • this PR only wants to remove the 301 code for redirects that are obviously not 301, and make them 302, and make no other changes

and this last purpose is succeeded in my tests that is why i marked a successful test

@andrepereiradasilva
Copy link
Contributor Author

andrepereiradasilva commented Jul 19, 2016

What's the benefit to not caching the second scenario? Something in my gut tells me this isn't the most optimal behavior.

Scenario: you have a site with /en/ (default langauge) and /fr/

If you use a 301 cached redirect.

First time you access / you get a 301 from / to /en/ and this will be cached in the browser.
So now you changed to /fr/ and navigate a little.
Now, while you are in french you access / trought a link (home logo for instance) or direct input. Since the 301 was cached before will go to /en/ (instead of going to /fr/), i.e, it will override the language cookie behaviour.

So we can't cache it.

@mbabker
Copy link
Contributor

mbabker commented Jul 19, 2016

OK that makes sense.

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 38b92ae


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11206.

@infograf768
Copy link
Member

RTC. We now get the correct redirections afaik


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11206.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 20, 2016
@wilsonge wilsonge merged commit a3ee16c into joomla:staging Jul 20, 2016
@wilsonge wilsonge added this to the Joomla 3.6.1 milestone Jul 20, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 20, 2016
@andrepereiradasilva andrepereiradasilva deleted the improve-redirect-alternative branch July 20, 2016 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants