Suggestion: add these people from the older duplicate issue to the commit message (for opening the issue, and steps to reproduce):
steelsector, matsbla, SiliconMind, vodde83
Problem/Motivation
The configuration of the language types are reset when another module is installed.
Proposed resolution
Not known yet.
Remaining tasks
Identify the cause and find a solution.
when this is committed, unpostpone #2623788: updateConfiguration() should be able to override configurable of language types when modules are installed or uninstalled
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|
Comments
Comment #2
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedHere is a test, that demonstrates the problem. First language_content is set as configurable, but after installing the field module, this is not the case anymore.
Comment #3
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedComment #4
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedIt is interesting that the test passes if I replaced the content_translation module by language_test.
Comment #5
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedSo, my guess is that the cause of this bug lies in the different implementations of hook_language_types_info_alter():
Comment #6
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedOr the way the language negotiater is updated after a module installation is not correct:
Comment #7
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedComment #9
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedComment #10
catchSince this is easily-reproduced data loss (fortunately configuration rather than content though), bumping to critical.
Comment #11
catchLooked through git blame, if it is language_modules_installed(), the last time it was changed was in 2014, and that was a straight port of previous logic which I didn't track down, so looks like the bug has been here for a long time.
Reading the code, this patch might be enough - untested.
Comment #14
catchWith the test coverage from #2.
Comment #19
alexpottComment #20
plachI'll have a look to this ASAP...
Comment #21
plachOn this
Comment #22
plachLets' try this
Comment #23
plachComment #25
plachMarked #2281211: Content language detection settings gets reverted to default as a duplicate.
Comment #26
plachComment #27
BerdirI can't say I fully understand that language configuration/purging stuff but the fix seems to make sense, the test looks good and fails as expected.
Let's get back to 0 criticals :)
Comment #28
BerdirThat's what I wanted to do...
Comment #30
catch#22 looks right to me.
Comment #31
catchFixable on commit:
alteration of
or just altering.
Comment #32
plach@Berdir:
You are not the only one: #2166879: Clean-up code managing language type configurability ;)
This is a clear case where a straight conversion from D7 was not the right move. Or at least we got it wrong :)
Comment #33
alexpottI think the test should be a part of LanguageNegotiationInfoTest
Comment #34
plachHere
Comment #36
plachBack to RTBC
Comment #39
cilefen CreditAttribution: cilefen commentedI think this was supposed to stay RTBC
Comment #42
cilefen CreditAttribution: cilefen commentedNot sure how to please the bot/issue integration.
Comment #43
YesCT CreditAttribution: YesCT commented(@cilefen maybe related to #2598438: Improve logic and/or UI around retesting patches in RTBC issues. hiding previous patches)
was reviewing the interdiff in #34 and it moves the test fine. rtbc++ for that.
but while I was at it, I fixed the nit in #31 and another nit.
I'm not sure if the following questions are enough to block this...
I also read the entire patch, and
this comment makes it seem like uninstalling could also cause the problem.
do we need a change in the hook for uninstalling modules? ah, no. because the uninstall hook is a wrapper for the install hook.
why do we do this here instead of a change in updateConfiguration()
this test uses the field module. but what is it about the field module that makes this test fail. is it that it does not try and make the language configurable?
I'm wondering if we should have a test module we use that has exactly the conditions we need instead of re-using field.
Also, I wonder if, as the comment in the fix states, uninstalling a module could have cause the same problem, if we should include that in the test also.
Comment #45
catchI think we should look at that in a follow-up, my original patch was hoping that would work already (clearly it doesn't), but it'd also be a behaviour change so something we should change in 8.1.x.
Let's add the test coverage for uninstalling a module though - that's the sort of thing that could easily get missed if this code gets refactored later.
Comment #46
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedComment #47
YesCT CreditAttribution: YesCT commentedadded note to the issue summary to
add these people from the older duplicate issue #2281211: Content language detection settings gets reverted to default to the commit message (for opening the issue, and steps to reproduce):
steelsector, matsbla, SiliconMind, vodde83
(I know we can't at this time add them to the database, but we can to the commit message)
@catch just to clarify, are we ok with using the field module in the test vs another test module? Or is that followup material also?
opened the followup @catch asked for in #45. it is postponed on this issue. #2623788: updateConfiguration() should be able to override configurable of language types when modules are installed or uninstalled it probably needs a better title and an update to the issue summary.
Comment #48
catch@YesCT I think it's probably fine to use field module in the test. It shouldn't matter which module is enabled in the test - just that any module gets enabled. However a comment explaining that would be good.
Comment #49
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedNo, it is not about the field module. There is no particular reason why I chose that.
Ok, I added the test_module, which only contains the info.yml. Already with that the test fails.
I added that in the new patch, too.
Comment #50
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedSorry, did not realized you commented already. So, we stick with #37 or #49?
Comment #51
catchNew module is fine too, just didn't want to block on it.
Comment #54
plachThat would be an API change. The PHP docs clearly state the argument should be an array of configurable language type names, so the previous code was plain wrong and the current one should be correct.
Or are you suggesting to make the parameter optional and default to that code?
The interdiff looks good to me.
Comment #55
catchYes was thinking optional parameter - but would be 8.1.x change.
Interdiff looks good to me too, so ommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!
Comment #62
catchComment #63
plachThanks, created #2623906: Make specifing the configurable language types for LanguageNegotiatorInterface::updateConfiguration() optional.
Comment #64
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedThank you very much for all your work!
Comment #65
YesCT CreditAttribution: YesCT commentedComment #66
Gábor Hojtsy