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

feat: add option to only detect browser locale on root path #799

Merged
merged 1 commit into from
Sep 9, 2020

Conversation

rchl
Copy link
Collaborator

@rchl rchl commented Jul 20, 2020

Don't attempt to detect browser's language when user (or crawler) goes
directly to a page with a locale prefix. This is to avoid redirecting
user that potentially wanted to load that specific page rather than the
one matching browser's language.

Resolves #455
Resolves #761

TODO

@rchl rchl force-pushed the fix/detect-locale-only-root branch 2 times, most recently from 58efa34 to 197cc89 Compare July 20, 2020 21:56
@divine
Copy link

divine commented Jul 21, 2020

Locale cookie isn't changed - that's definitely needed.

Visit example.com -> redirected to /en (based on accepted-language)

After that visit examples.com/fr -> cookie not changed to 'fr', still kept 'en'.

Visit example.com again -> redirected to /en instead of /fr.

--
'detectOnlyOnRoot' might be better name I think ?

Otherwise looks good!

@rchl
Copy link
Collaborator Author

rchl commented Jul 21, 2020

Locale cookie isn't changed - that's definitely needed.

Visit example.com -> redirected to /en (based on accepted-language)

After that visit examples.com/fr -> cookie not changed to 'fr', still kept 'en'.

Visit example.com again -> redirected to /en instead of /fr.

I'm not convinced.
You are suggesting that opening /fr should update user's detected locale. Why is that?

I would say that stored locale is something more of a user's choice and should remain at the original value even if user somehow ends-up loading page from another locale.

In sites that I work with the locale is detected and store initially on the first visit and then the user can change it through a language selector. I'm not sure it should change on just visiting a page from another locale.

@divine
Copy link

divine commented Jul 21, 2020

You are suggesting that opening /fr should update user's detected locale. Why is that?

First time user came from search engines to the page ending with /fr (based on localized keywords that he have typed).

That user definitely wanted fr as language and his default locale cookie should set to that so when he gets back directly to the root page this should be redirected to fr (lets say he closed page and came back after 5 minutes).

It doesn't make sense to redirect again based on other factors when he definitely visited that page once before.

I would say that stored locale is something more of a user's choice and should remain at the original value even if user somehow ends-up loading page from another locale.

It's an user's choice that isn't kept at all which should be.

In sites that I work with the locale is detected and store initially on the first visit and then the user can change it through a language selector. I'm not sure it should change on just visiting a page from another locale.

His first visit is from search engines directly to fr page. Does that makes sense now?

Also, any other i18n are just doing what I wrote and honors user choice on page visit.

Thanks!

@rchl
Copy link
Collaborator Author

rchl commented Jul 22, 2020

That user definitely wanted fr as language and his default locale cookie should set to that so when he gets back directly to the root page this should be redirected to fr (lets say he closed page and came back after 5 minutes).

It doesn't make sense to redirect again based on other factors when he definitely visited that page once before.

That's the case when user visits the page for the first time and it makes sense what you are saying.

I was more thinking about user that has already visited the page and then clicked a link to different locale (accidentally or was sent a link or something). Then I would consider changing his choice too invasive.

I would say that stored locale is something more of a user's choice and should remain at the original value even if user somehow ends-up loading page from another locale.

It's an user's choice that isn't kept at all which should be.

Isn't kept at all? I think it is currently at least. Excluding the initial navigation that sets the cookie based on browser locale, from there I think it doesn't change unless changed explicitly.

Also, any other i18n are just doing what I wrote and honors user choice on page visit.

Do you have any references to other solutions that do that?

@divine
Copy link

divine commented Jul 22, 2020

(accidentally or was sent a link or something). Then I would consider changing his choice too invasive.

It's not too invasive, see how Google handles this:

Visit https://support.google.com/webmasters/?hl=fr (or Russian for example https://support.google.com/webmasters/?hl=ru)

Close the window and visit https://support.google.com/webmasters/

Language was set to French by only visiting that url.

Nobody mistakenly visits links, even if it happens user could change language later on - it's less annoying or invasive (it's not saved in database or not changing some config files 😄) .

Isn't kept at all? I think it is currently at least. Excluding the initial navigation that sets the cookie based on browser locale, from there I think it doesn't change unless changed explicitly.

Currently this behavior is a temporary fix for crawlers to see page content without redirection, however coming back user will be annoyed to be redirected to en locale when he definitely was browsing fr before.

Do you have any references to other solutions that do that?

Yes, kindly check this library probably it also answers some of the questions regarding prefix_except_default

Thanks!

@rchl
Copy link
Collaborator Author

rchl commented Jul 22, 2020

Well, that's quite convincing but it would be quite a major behavior change for existing users so I'm not sure I can just enable that behavior by default. And tying it to the detectOnlyOnRoot wouldn't make sense, semantically.

@divine
Copy link

divine commented Jul 22, 2020

Well, that's quite convincing but it would be quite a major behavior change for existing users so I'm not sure I can just enable that behavior by default. And tying it to the detectOnlyOnRoot wouldn't make sense, semantically.

Maybe add a configuration option? I know that you've previously said that won't add some new configurations, however as you could see this definitely needed and everyone could just configure i18n as they want.

Thanks!

@divine
Copy link

divine commented Jul 30, 2020

@rchl any update on this please?

@rchl
Copy link
Collaborator Author

rchl commented Jul 30, 2020

Sorry but I don't have much time and the time I have I prefer spending somewhere else right now.

I'm not entirely happy about this change and it also needs more testing (in form of tests, for now).

I'll get to it for sure and hopefully sooner than later.

I can release it as beta if you want to try it.

@divine
Copy link

divine commented Jul 30, 2020

I can understand this is a major change, however a lot of people have requested this feature as well not sure why everyone went silent about this now.

I've tested it, works perfectly except cookie not changing correctly.

I would like to contribute but it's a bit hard with so many options included and still trying to figure out where to put that cookie change (use already written function or just write new one or refactor it at all).

Releasing as beta might help to test it for everyone who needs this feature.

Thank you again!

@divine
Copy link

divine commented Aug 13, 2020

@rchl sorry to bother you again, have you had a chance to find a time to think about this again? Like what might take you happy about this change? This is needed for prefixed strategies to be used primarily for seo optimization. I think that all users will agree for a change, because this is a number one reason to be in search engines, otherwise no reason to make site with different locales when site isnt visible in search engines at all.

Anyways, I've tried to use this branch merged with the master, however it doesn't care about stored cookie at all and always redirects to default locale. Probably this started after that fix on initial load redirection #805

Thanks!

@rchl
Copy link
Collaborator Author

rchl commented Aug 14, 2020

Sorry for delaying this but this change feels like it needs a lot of uninterrupted attention from me so that I have the whole picture clear and manage to cover all cases.

It could be that it's simpler case than I think of it and I'm just over complicating it...

It's just hard to get into it. Especially as this last week was busy for me (not easy with a kid that had to say home).

@koendeschacht
Copy link

koendeschacht commented Aug 21, 2020

We also ran into this issue with a new site we are developing. The proposed option would solve our issue, but I'm wondering, should the locale in the url not always take precedence?

In my view, if the user entered a url with a certain locale, he probably wanted to view the page in that locale?

I understand that there is also the case where I follow a link with a certain locale, but I might want to see the page in a different locale. But I feel that, if the site owner would want the second use case to take precedence over the first, they would have chosen a no_prefix strategy?

@rchl
Copy link
Collaborator Author

rchl commented Aug 21, 2020

There are various strategies. Speccing behavior for each would help me progress here:

  • no_prefix: routes won't be prefixed
  • prefix_except_default: add locale prefix for every locale except default
  • prefix: add locale prefix for every locale
  • prefix_and_default: add locale prefix for every locale and default

Expected (?) behavior with default detectBrowserLanguage settings:

  • no_prefix: always redirect
  • prefix_except_default: only redirect from the default (unprefixed) route
  • prefix: never redirect?
  • prefix_and_default: only redirect from the default unprefixed route (not from the default prefixed rule)?

(Behavior above applies when the i18n_redirected cookie is missing, of course. Once it's set, we don't attempt to redirect.)

Does everyone agree with that?

I'm not certain about the prefix_and_default case. Whether we should redirect from both unprefixed and prefixed default locale. Same for prefix, should we never redirect or redirect from default (prefixed) locale?

@koendeschacht
Copy link

Thanks for the quick follow!

These rules look sane to me. I'm wondering however, are you suggesting to drop the onlyRedirectFromRoot option, or is the behaviour that you describe the behaviour only when onlyRedirectFromRoot is not set?

@rchl
Copy link
Collaborator Author

rchl commented Aug 21, 2020

That's the behavior with onlyRedirectFromRoot (or as I called it in this PR, detectOnlyOnRoot).
I will make this new behavior opt-in to avoid breaking changes and then change the default in next major version if it's looking good.

@divine
Copy link

divine commented Aug 21, 2020

Behavior above applies when the i18n_redirected cookie is missing, of course. Once it's set, we don't attempt to redirect.

@rchl just one kindly question, would it change cookie on visiting localized url ? #799 (comment)

I'm not certain about the prefix_and_default case. Whether we should redirect from both unprefixed and prefixed default locale. Same for prefix, should we never redirect or redirect from default (prefixed) locale?

Prefixed routes never should redirect, locale already here it doesn't make sense to try to detect anything (even cookie on that prefixed url)!

Regarding prefix_and_default it should work similarly as prefix_except_default doing redirection only on unprefixed locale url.

Sorry, if I misunderstood it correctly, however what we're looking is indexed site links in search engines and honoring site locale by visiting prefixed url without giving zero information (cookie, browser language and etc).

Thanks!

@rchl
Copy link
Collaborator Author

rchl commented Aug 21, 2020

@rchl just one kindly question, would it change cookie on visiting localized url ? #799 (comment)

Yes, makes sense to update the cookie when not redirecting.

Prefixed routes never should redirect, locale already here it doesn't make sense to try to detect anything (even cookie on that prefixed url)!

Regarding prefix_and_default it should work similarly as prefix_except_default doing redirection only on unprefixed locale url.

I think we agree here then.

Sorry, if I misunderstood it correctly, however what we're looking is indexed site links in search engines and honoring site locale by visiting prefixed url without giving zero information (cookie, browser language and etc).

Yep.

@rchl rchl force-pushed the fix/detect-locale-only-root branch from 197cc89 to 7d6b4e0 Compare September 1, 2020 21:25
@divine
Copy link

divine commented Sep 5, 2020

@rchl sorry to ask, is this almost ready or something is left?

Thanks!

@rchl
Copy link
Collaborator Author

rchl commented Sep 5, 2020

Feeling pretty sick this whole week. Planning to have a final look at the code as soon as possible. Wanted to go through https://github.com/mcamara/laravel-localization again as I remember I saw some interesting stuff while reading it but forgot what those were.

@divine
Copy link

divine commented Sep 7, 2020

Feeling pretty sick this whole week.

Wishing you well. Looking forward to see when you'll feel better!

Thanks!

…ocale

Don't attempt to detect browser's language when user (or crawler) goes
directly to a page with a locale prefix if that locale is not the default
one. This is to avoid redirecting user that potentially wanted to load
that specific page rather than the one matching browser's language.

Resolves #455
Resolves #761
@rchl rchl force-pushed the fix/detect-locale-only-root branch from 7d6b4e0 to 05dcc8f Compare September 9, 2020 18:46
@rchl
Copy link
Collaborator Author

rchl commented Sep 9, 2020

OK, I think I'm ready to let it go.

Two potential concerns:

@divine
Copy link

divine commented Sep 9, 2020

@rchl you're right, however there is no elegant solution for prefix_and_default , maybe adding a note about that it's really hard to give correct language or do some redirection which in fact affects SEO dramatically.

Search engines dislikes whenever route redirection/content changes and it makes them sick. Basically for SEO optimized website important thing is static content URLs that always kept same, if it changes duplicates are created and pages will be removed after that.

Hopefully I understood your concerns well.

Thanks!

@rchl rchl merged commit 7bdb227 into master Sep 9, 2020
@rchl rchl deleted the fix/detect-locale-only-root branch September 9, 2020 20:57
@rchl
Copy link
Collaborator Author

rchl commented Sep 9, 2020

Will release tomorrow morning so that I'm available if something breaks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants