Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
locale.module
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
25 Sep 2014 at 16:05 UTC
Updated:
5 Jan 2015 at 13:34 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
gábor hojtsyThis should fix it.
Comment #2
gábor hojtsyThis is needed for our Lab. Tagging D8MIAMS.
Comment #3
sutharsan commentedCode looks good and fixes the problem.
Comment #4
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 commentedComment #10
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 commentedTests look good. Just one tiny change in the comments:
'Afrikaans' for 'African' here.
Comment #13
sutharsan commentedThanks for the review.
Only a new patch this time, no 'test only' patch for this trivial change.
Comment #14
lazysoundsystem commentedThanks. RTBC.
Comment #16
sutharsan commentedIgnore the patch in #13. New patch containing the comment in #12 is included.
Comment #18
sutharsan commentedTest failing due to removal of
url(). For now replaced by_url().Comment #19
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.