Problem/Motivation
Configuration translations are not imported if Interface translation module is enables after adding a language.
Steps to reproduce:
- Install in English.
- Enable Language module.
- Add a language. Enable Locale module.
- Go to report > Translation updates and run a translation update.
- Observe: Configuration translations are not updated.
Configuration translation is imported when:
- Install in English.
- Enable Language and Locale modules at once.
- Add a language.
- Observe: The batch that runs downloads translations and updates configuration translations.
Proposed resolution
Update configuration translations as well from the localization update process.
Remaining tasks
Review. Commit.- Add tests
User interface changes
The batch will run longer.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#30 | 2344967-config-translation-update-30.patch | 3.9 KB | Gábor Hojtsy |
#30 | interdiff.txt | 1.15 KB | Gábor Hojtsy |
#28 | 2344967-config-translation-update-27.patch | 3.91 KB | Gábor Hojtsy |
#25 | interdiff.txt | 740 bytes | Gábor Hojtsy |
#25 | 2344967-config-translation-update-25.patch | 3.9 KB | Gábor Hojtsy |
Comments
Comment #1
Gábor HojtsyThis should fix it.
Comment #2
Gábor HojtsyThis is needed for our Lab. Tagging D8MIAMS.
Comment #3
Sutharsan CreditAttribution: Sutharsan commentedCode looks good and fixes the problem.
Comment #4
Désiré CreditAttribution: Désiré commentedI tested it twice (patch #1): once with a minimal, once with a standard install profile.
First I enabled language and locale module and added a language, dumped the language related config from the database.
Then I enabled only language module, added a language and then enabled locale module and imported the translations, dumped the language related config from the database.
As I experienced the language related config were the same on both cases, so I assume that the patch works well.
Comment #5
webchickThis needs tests, but I can see writing them would be difficult. OTOH it would be good to get this fixed prior to beta. Therefore, I think I'm going to let this in, but leave it needs work for tests. If folks could please follow back up with that during/after Amsterdam, that would be really awesome. This is definitely something we don't want to break a second time. :)
Committed and pushed to 8.x. Thanks!
Now back to needs work.
Comment #7
Gábor HojtsyComment #8
Gábor HojtsyComment #9
Sutharsan CreditAttribution: Sutharsan commentedComment #10
Sutharsan CreditAttribution: Sutharsan commentedThis patch adds a test that follows the steps from the issue summary to reproduce the problem.
For the 'must-fail' patch I've added the reverse of #1 patch.
Comment #12
lazysoundsystem CreditAttribution: lazysoundsystem commentedTests look good. Just one tiny change in the comments:
'Afrikaans' for 'African' here.
Comment #13
Sutharsan CreditAttribution: Sutharsan commentedThanks for the review.
Only a new patch this time, no 'test only' patch for this trivial change.
Comment #14
lazysoundsystem CreditAttribution: lazysoundsystem commentedThanks. RTBC.
Comment #16
Sutharsan CreditAttribution: Sutharsan commentedIgnore the patch in #13. New patch containing the comment in #12 is included.
Comment #18
Sutharsan CreditAttribution: Sutharsan commentedTest failing due to removal of
url()
. For now replaced by_url()
.Comment #19
Sutharsan CreditAttribution: Sutharsan commentedComment #20
Gábor HojtsyLooks good, some minor comments:
This is a bit too generic :) I would try to get the same 1 line text that is on the test method.
Would be great to get this on one line, eg. "Test localization update will change configuration translations if enabled after language." (did not check if this fits on one line in fact :D)
"now that the"
There is no code apparent in this patch that makes this file be the result of update checking, where is that located? I suspect preexisting in core.
Comment #22
Gábor Hojtsy@Sutharsan: this seemed like was so close, are you still around to fix this one?
Comment #23
Gábor HojtsyThe answer to #20/4 is this snippet from locale_translate_test which module is used in the test. The other minor text fixes I made myself. That should not disqualify me from RTBC-ing it since the changes are so minor.
Comment #25
Gábor HojtsyComment #26
Gábor HojtsyComment #28
Gábor HojtsyMinor PHP issue.
Comment #29
alexpottIs it necessary to use _url() here?
Comment #30
Gábor Hojtsy@alexpott: the problem with the update-test path/route is it would expect some arguments, namely project and version. Then it actually ignores version later on (haha). For this test, all we need is a URL that would not return much valid info, because we only work off of local stuff here (see locale_test_translate.info.yml), so this override is for the rest of the projects so they don't interfere with the test. To fix the update-test route that requires unrelated changes, so we can just use the front page here just as well.
Better?
Comment #31
alexpottI wished we had opened a follow-up for this - anyway nice to have additional test coverage for this. This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 3f28dd7 and pushed to 8.0.x. Thanks!
Comment #33
Gábor HojtsyThanks a lot!
Comment #34
Gábor HojtsyAlso restoring original title.