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

make sure force language is reflected in html lang attribute #9435

Merged
merged 2 commits into from
May 23, 2018

Conversation

georgehrke
Copy link
Member

Steps to reproduce:

  • set force_language in config.php (e.g. to de)
  • everything that's translated in javascript will still be english, because force language is not properly reflected in the lang attribute of the html element.

before:
before

after:
after

Ref: https://help.nextcloud.com/t/spracheinstellung-im-kalender/30969/16

@@ -130,6 +130,13 @@ public function findLanguage($app = null) {
return $this->requestLanguage;
}

$forceLang = $this->config->getSystemValue('force_language', false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this not be at the top if the language is forced?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and i guess the block from get can be removed then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this not be at the top if the language is forced?

@rullzer No, because the statement above caches the selected language. $this->requestLanguage is ever only set inside findLanguage() and never from the outside. If we put the forceLang statement on top, it would check on every call if the language exists, which requires fs access and should thereby be avoided.

and i guess the block from get can be removed then?

@nickvergessen No, because both get() and findLanguage() are public methods and can be used individually without each other. get() should still respect forceLang even if an app requests a specific language. Let's say forceLang = de, but an app requests get(dav, en), it should return a german L10N object.

@codecov
Copy link

codecov bot commented May 9, 2018

Codecov Report

Merging #9435 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master   #9435      +/-   ##
===========================================
+ Coverage      51.7%   51.7%   +<.01%     
- Complexity    25760   25762       +2     
===========================================
  Files          1644    1644              
  Lines         96578   96582       +4     
  Branches       1394    1394              
===========================================
+ Hits          49935   49939       +4     
  Misses        46643   46643
Impacted Files Coverage Δ Complexity Δ
lib/private/L10N/Factory.php 73.05% <100%> (+0.66%) 74 <0> (+2) ⬆️
lib/private/Files/Cache/Propagator.php 94.93% <0%> (-1.27%) 16% <0%> (ø)
apps/files_trashbin/lib/Trashbin.php 72.7% <0%> (+0.24%) 136% <0%> (ø) ⬇️

@georgehrke georgehrke force-pushed the bugfix/noid/fix_force_language_html_attr branch from 17337b7 to 2a21471 Compare May 22, 2018 17:30
@MorrisJobke MorrisJobke added this to the Nextcloud 14 milestone May 23, 2018
@MorrisJobke
Copy link
Member

MorrisJobke commented May 23, 2018

Doesn't work for me:

  • de is my default in the browser
  • nl is set as forced language
  • browser still shows German parts:

bildschirmfoto 2018-05-23 um 20 55 20

@rullzer
Copy link
Member

rullzer commented May 23, 2018

@georgehrke I pushed a fix.

Basically if force_language is set we always set it at the beginning of this function. (cheap call anyway).

Else if the first call is to an app that doesn't have the force language the requestLanguage will be set to the userLanguage (if that does exist) and it will not work.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works now 👍 Also the code makes sense

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 23, 2018
@georgehrke
Copy link
Member Author

@rullzer makes sense :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants