Problem/Motivation
Found this while redoing videos for a session. Even if you pick an RTL language, the installer will not be RTL until the database is set up. This is a regression of #1974040: When installing in an RTL language, it should be RTL from profile selection onwards which was already there before.
Proposed resolution
1. Fix it again.
2. Now write tests so it will not break again.
Remaining tasks
User interface changes
Installer will be RTL from the second step, after picking an RTL language.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#21 | interdiff.txt | 3.48 KB | sun |
#21 | drupal8.installer-rtl.21.patch | 4.81 KB | sun |
#19 | screenshot_with_2240007-17-installer-rtl.png | 322.85 KB | MarkusDBX |
#19 | screenshot_without_patch.png | 327.3 KB | MarkusDBX |
#17 | interdiff.txt | 2.21 KB | mr.baileys |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedtagging and adding contributor task documents to the issue summary
in case someone wants to use a brute force way of finding which commit broke this:
webchick's how to post http://webchick.net/node/99
Comment #2
YesCT CreditAttribution: YesCT commentedsorry, without specifics, this isn't novice yet. updating issue summary remaining tasks.
Comment #3
YesCT CreditAttribution: YesCT commentedalso tagging front end so our front end focus board has this. (http://www.drupal8multilingual.org/issues/frontend)
Comment #4
mr.baileysUsing git bisect shows that this got broken when the monster patch from #1862202: Objectify the language system landed.
I'll write tests and attempt to fix the issue over the next couple of days.
Comment #5
YesCT CreditAttribution: YesCT commentedthanks, adding that as the parent of this issue.
Comment #6
sunProper issue title.
Comment #7
sunI'm also ambivalently surprised that your bisect yielded the language objectification issue. I could have sworn that I'm guilty ;-)
Comment #8
mr.baileysI initially also thought #2213357: Use a proper kernel in early installer was the cause, but I double-checked and it was broken prior to that commit. Can you find solace in the fact that if it wouldn't have been broken, your change would probably have broken it? ;)
Here's a test for this (expecting it to fail), now trying to fix the actual issue...
Comment #10
mr.baileysSo I'm able to "fix" this by adding something like:
to InstallerServiceProvider::register(). However, this seems to not be right approach, since it adds a dependency on a global (the selected language). I also tinkered with a "InstallerDefaultLanguage" class and moved the global there, but it still feels icky.
(removed frontend tag since this is a backend issue)
Comment #11
mr.baileysPrior to appying the patch:
After applying the patch:
(unassigning myself since I will be AFK for a week or so, don't want to hold up progress on this issue)
Comment #12
Gábor Hojtsy@sun for his installer experience and @plach for his language manager experience would be great reviewers on this one :)
Comment #13
YesCT CreditAttribution: YesCT commentedI wonder if we should have the opposite test also, that the installer stays in ltr, with a ltr language.
--
this is adding docs like @file for the file and docs on methods. see interdiff.
Comment #14
Gábor HojtsyThe existing installer tests can be extended to ensure the installer keeps in LTR if appropriate. Eg. our German test and/or our default English test.
Comment #15
mr.baileysHere's a new patch with LTR language direction assertion in the German translation installer test.
Comment #16
Gábor HojtsyI see how you use this global value to pass on this information. However $conf['language_default'] is not anymore used for anything, the site default language is now directly stored on system.site's langcode.
So IMHO it would be better to use something else to avoid confusion. Is the global value the best way to pass on this info?
Comment #17
mr.baileysYes, the global is ugly, but I couldn't think of a better, cleaner way to implement this.
However, I assumed LanguageDefault was an immutable object. Turns out it isn't, so we can just do
\Drupal::service('language.default')->set()
, making this patch both cleaner and simpler.Comment #18
Gábor HojtsyLooks good from a code points of view :)
Comment #19
MarkusDBX CreditAttribution: MarkusDBX commentedReviewing this latest, 2240007-17-installer-rtl.patch
Before applying the patch:
After applying the patch
Works. It would be perfect if the translation was also correct in step 2, but it may be another issue.
Comment #20
MarkusDBX CreditAttribution: MarkusDBX commentedI tested the latest patch 2240007-17, it works fine. The translation in step two still needs work, but that may be another issue.
Comment #21
sunSorry for not getting back to this earlier.
Patch looks good to me; just some minor cleanups.
(Note that I have renames=copies enabled; attached patch still creates the new test; git merely detected that the new file is a copy.)
Comment #22
webchickNice catch!
LOL :)
Committed and pushed to 8.x. Thanks!
Comment #24
Gábor HojtsyYay, thanks!