Problem/Motivation
After enabling language.module, the default contact form (and any other you create) shows the language widget. No other entity form does.
language.module makes the default widget configurable and It also has settings to control if it should be shown or not, but they have inconsistent conditions under which they are applied:
UI to configure the langcode widget: checks if the entity type has a langcode key
controlling visiblity: checks the langcode key (and if it exists in the form) and if the entity type is translatable
Now, the contact message entity defines a langcode field with a widget but not a langcode entity key. The result is that the widget is shown for each new contact form and doesn't respect the default behavior, which is not showing anything.
Proposed resolution
Add the langcode key to Message, make sure that language.module behaves consistently by always checking the langcode.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff-langcode_widget-2533964-31-36.txt | 2.39 KB | edurenye |
#36 | langcode_widget-2533964-36.patch | 6.9 KB | edurenye |
#36 | langcode_widget-2533964-36-test.patch | 5.14 KB | edurenye |
#31 | interdiff-langcode_widget-2533964-27-31.txt | 8.13 KB | edurenye |
#31 | langcode_widget-2533964-31.patch | 7.14 KB | edurenye |
Comments
Comment #1
BerdirCorrection. The base field alter hook just makes the widget configurable, it's added in Message. So it really is mostly contact.module's fault but we should still do the part about checking the langcode key and not isTranslatable(). Just because something isn't translatable doesn't mean that you can't set a language.
Manually tested this, needs automated tests.
Comment #2
BerdirComment #3
BerdirComment #4
BerdirComment #5
Gábor HojtsyThe proposed changes make sense, the language field should not depend on whether the entity type is translatable, those two are not the same thing. Does need tests indeed, but looks good as a fix.
Comment #6
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedAdded the tests.
Comment #7
Gábor HojtsyCan you post the test only patch as well? Reviewing the test it looks good, only a minor thing found:
Comment is incorrect (the second time; looks like copy-paste).
Comment #8
Gábor HojtsyComment #9
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedSorry for this mistake.
Here is just the test patch.
Next comment will post final patch and interdiff.
Comment #10
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedHere is the patch and interdiff.
Comment #11
Gábor HojtsyThe test-only patch that I mentioned is a .patch file where the fix is missing, so we can ensure that the test is testing the fixed bug (by observing it fail without the fix). Can you post that one? Also:
show => shows
Comment #12
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedDone.
Comment #14
Gábor HojtsySuperb looks great! Thanks!
Comment #16
penyaskitoTests were added, removing tag.
Comment #17
alexpottDo we have any tests where an entity does not have a langcode? Tests for this should be in the language module.
Comment #18
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedI agree, was not the best place to put the test at it's not with the only module that it happens, so I moved it to language.
About the tests without langcode, what you mean exactly? Beacuse in the EntityDefaultLanguageTest, there is a test about undefinde language, I'm not sure if is possible to have there an empty langcode and then may break.
But anyway, this maybe means a follow up, as then this other test is not related to this issue.
Comment #20
Gábor Hojtsy@edurenye: so the code changes to use
$form_object->getEntity()->getEntityType()->hasKey('langcode')
I think @alexpott meant testing the case where the entity type does not have a langcode key. While as oer EntityType::__construct(), all entity types at least have an empty langcode key, those would of course not go into this conditional now. They did before. my understanding is the needs tests tag was for that.Comment #21
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedOk, I added a test for the empty langcode.
Comment #22
Gábor HojtsyWell,
$form_object->getEntity()->getEntityType()->hasKey('langcode')
is not about a specific entity not having a langcode but an entity type not supporting language.Comment #24
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedSo then, better with entity_test, that doesn't has language support.
Comment #26
BerdirClassname, description and method is inconsistent now.
It might be a lot easier to document and understand if you keep the two scenarios in completely different tests. Keep the contact test like we had it before because that's actually testing contact.module and add a separate test (EntityTypeWithoutLanguageFormTest ?)
@group is now wrong.
If entity_test really has no language, then it makes no sense to show up here, so why does it?
One part of the answer is that entity_test *does* have a language and langcode entity key.
So we either need a new test entity type (Maybe in language_test.module, because entity_test has like 20 entity types already and they all need to create the schema every time the module is installed...
Comment #27
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedI don't understand why I should split the class, both tests are testing the language widget in the entity form.
I updated just the documentation to be more consistent, and changed entity_test for a custom no_language_entity_test.
Also I don't understand why I have to remove the line in the point 3, I'm not suposed to test that in a class with a empty langcode even if you set it, shouldn't appear the language selector?
Comment #28
BerdirI'm suggesting to split them because the test is hard to understand, you still have a testContactLanguage() method for example, but that's just one part of what you are doing.
On point 3, you should not be able to set it, because an entity type without language should not be selectable in that list. The fact that you can is yet another bug. So we should assert that there is *no* checkbox for that entity type.
Comment #31
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedOk, now it has real no language support, I splitted the tests and assert the right thing.
Comment #33
BerdirLeaving at needs work to get feedback on point 2. and 3.
We don't need this part, except if we actually want to edit it, see below.
We discussed this a bit.
Unlike contact forms, which actually had the form element there and the assertNoField() made sense on the edit form, it really doesn't here IMHO. There can not possibly be a form element, since there is not even a field that could have a widget.
There is also no actual bug for that scenario, so there is no way we can have a failing test.
We could add it, but the only thing it would allow us to do is making sure we have no notices on such a form.
This test is testing contact at least as much as language.
The real bug was in contact.module that had a weird inconsistent language definition ( a field but no language key). And language then behaved strangely as well.
I don't really care, but imho, this test would make more sense in contact.module, as it originally was.
But I'm leaving the decision to others.. Gabor?
Comment #34
Gábor HojtsyAgreed with Berdir :)
Comment #35
BerdirOk, setting to NW then for removing the entity create call and moving the test back.
Comment #36
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedDone.
Comment #38
BerdirThis looks good to me, maybe wait for Gabor to sign of too :)
Comment #39
Gábor HojtsyLooks good to me too :) Thanks a lot!
Comment #40
alexpottThis issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed bd1964c and pushed to 8.0.x. Thanks!
Not used. Fixed on commit.
Fixed file mode to be 644 on commit.
Comment #42
Gábor HojtsyYay, superb!