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

Stop moving session language ahead of reqLangs #1721

Merged
merged 3 commits into from Apr 9, 2021
Merged

Conversation

pbrisbin
Copy link
Member

@pbrisbin pbrisbin commented Apr 8, 2021

Yesod.Core.Handler.languages checks first for a language set in the user's
session, prepending that value to YesodRequest{reqLangs}, so it is respected
above all else if present.

For context, reqLangs itself also includes the session, but just later in
line:

langs' = catMaybes [ lookup langKey gets -- Query _LANG
                   , lookup langKey cookies     -- Cookie _LANG
                   , lookupText langKey session -- Session _LANG
                   ] ++ langs                    -- Accept-Language(s)

In #1720, it was raised that allowing the session (something implicitly present
for any request) to override a query parameter (something explicitly given on
that request) is surprising.

We decided (without knowing what order reqLangs was doing) that query, cookie,
session, accept was best and languages should be changed to do that.
Conveniently, this just makes languages equivalent to reqLangs, so that is
what this patch does.


I'll do this in a fast-follow:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)
  • Send a message to the mailing list about the order change

Yesod.Core.Handler.languages checks first for a language set in the
user's session, prepending that value to YesodRequest{reqLangs}, so it
is respected above all else if present.

For context, reqLangs itself also includes the session, but just later
in line:

    langs' = catMaybes [ lookup langKey gets -- Query _LANG
                       , lookup langKey cookies     -- Cookie _LANG
                       , lookupText langKey session -- Session _LANG
                       ] ++ langs                    -- Accept-Language(s)

In #1720, it was raised that allowing the session (something implicitly
present for any request) to override a query parameter (something
explicitly given on that request) is surprising.

We decided (without knowing what order reqLangs was doing) that query,
cookie, session, accept was best and languages should be changed to do
that. Conveniently, this just makes languages equivalent to reqLangs, so
that is what this patch does.
@pbrisbin pbrisbin requested a review from snoyberg April 8, 2021 13:42
@snoyberg
Copy link
Member

snoyberg commented Apr 8, 2021

Can you also add a comment in the Haddocks that the behavior of the function changed, and in which version?

@pbrisbin
Copy link
Member Author

pbrisbin commented Apr 8, 2021

Before I fire off the yesodweb message, are you good with 1.6.18.9 or do you want to do 1.6.19.0?

@snoyberg
Copy link
Member

snoyberg commented Apr 8, 2021

Let's do 1.6.19.0

@pbrisbin
Copy link
Member Author

pbrisbin commented Apr 8, 2021

This is all set, just waiting on the Approve :)

@snoyberg snoyberg merged commit 21bfad3 into master Apr 9, 2021
@snoyberg
Copy link
Member

snoyberg commented Apr 9, 2021

Thanks!

@snoyberg snoyberg deleted the pb/reorder-languages branch April 9, 2021 03:05
@snoyberg
Copy link
Member

snoyberg commented Apr 9, 2021

And released to Hackage

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

2 participants