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

Fixes language switching to default language #2689

Merged

Conversation

robinboening
Copy link
Contributor

@robinboening robinboening commented Jan 15, 2024

Closes #2632

From this commit on the language id won't be stored in the session any more.

Upon every page visit the language id got stored in the session. When switching from a secondary language to the default language no language param is passed to the controller. The order how the language is found prioritized the session and the default was just a fallback. Coming from a secondary language and therefore it's id in the session, it would never load the default language, but the secondary.

Storing the language in the session was most likely a performance optimization. Removing this should not impact current behaviour.

@robinboening robinboening force-pushed the fix_switching_to_default_language branch from d47b425 to 1cd021c Compare January 15, 2024 13:03
@robinboening robinboening requested a review from tvdeyen January 15, 2024 13:04
@tvdeyen
Copy link
Member

tvdeyen commented Jan 16, 2024

@robinboening thanks. The spec failures seem to be related. Mind taking a look?

@robinboening robinboening force-pushed the fix_switching_to_default_language branch from 1cd021c to 1c2b53e Compare January 16, 2024 11:23
@robinboening
Copy link
Contributor Author

robinboening commented Jan 16, 2024

I overlooked another session check in a spec and might have forgotten to push again last night. All specs but 1 are fixed.

I am failing to see how this one failing admin spec could possibly be related.
https://github.com/AlchemyCMS/alchemy_cms/blob/main/spec/features/admin/language_tree_feature_spec.rb#L52

I could not find any reference to a language dependency in the session in the Admin Controller stack (ControllerActions -> BaseController -> Admin::BaseController -> Admin::PagesController).

Any idea how this could be related?

@tvdeyen
Copy link
Member

tvdeyen commented Jan 16, 2024

@robinboening the CurrentLanguage module is mixed into the Admin Pages controller and this expects that the Language.current is set. Maybe there is something off now?

@tvdeyen
Copy link
Member

tvdeyen commented Jan 24, 2024

@robinboening any chance to look into failing specs, so we can add this to 7.1 milestone?

@tvdeyen tvdeyen modified the milestone: 7.1 Jan 24, 2024
@tvdeyen tvdeyen force-pushed the main branch 3 times, most recently from 4ea5c7d to 19e4094 Compare February 19, 2024 14:11
@tvdeyen tvdeyen force-pushed the fix_switching_to_default_language branch from 1c2b53e to f68f5f4 Compare February 25, 2024 18:20
@tvdeyen
Copy link
Member

tvdeyen commented Feb 25, 2024

We need to still store the language in the session for admin requests, otherwise the language switch in the admin would not be persisted. This is why the spec stilled failed. I amended the fix to your commit.

@tvdeyen tvdeyen added backport-to-7.0-stable Needs to be backported to 7.0-stable backport-to-7.1-stable Needs to be backported to 7.1-stable labels Feb 25, 2024
@tvdeyen tvdeyen force-pushed the fix_switching_to_default_language branch from f68f5f4 to 6c87e35 Compare February 25, 2024 18:27
Closes AlchemyCMS#2632

From this commit on the language id won't be stored in the session any more
for frontend requests.

Upon every page visit the language id got stored in the session. When switching from a secondary language to the default language no language param is passed to the controller. The order how the language is found prioritized the session and the default was just a fallback. Coming from a secondary language and therefore it's id in the session, it would never load the default language, but the secondary.

Storing the language in the session is still done in the admin section.
@tvdeyen tvdeyen force-pushed the fix_switching_to_default_language branch from 6c87e35 to f63157e Compare February 25, 2024 18:29
@alchemycms-bot
Copy link
Collaborator

💚 All backports created successfully

Status Branch Result
7.0-stable
7.1-stable

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

tvdeyen added a commit that referenced this pull request Feb 25, 2024
[7.0-stable] Merge pull request #2689 from robinboening/fix_switching_to_default_language
tvdeyen added a commit that referenced this pull request Feb 25, 2024
[7.1-stable] Merge pull request #2689 from robinboening/fix_switching_to_default_language
@robinboening robinboening deleted the fix_switching_to_default_language branch February 25, 2024 21:25
@robinboening
Copy link
Contributor Author

Thanks for taking over and fixing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-7.0-stable Needs to be backported to 7.0-stable backport-to-7.1-stable Needs to be backported to 7.1-stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switching back to default language doesn't actually change the language
3 participants