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

CommentFileSizeAuthor
#36 interdiff-langcode_widget-2533964-31-36.txt2.39 KBedurenye
#36 langcode_widget-2533964-36.patch6.9 KBedurenye
#36 langcode_widget-2533964-36-test.patch5.14 KBedurenye
#31 interdiff-langcode_widget-2533964-27-31.txt8.13 KBedurenye
#31 langcode_widget-2533964-31.patch7.14 KBedurenye
#31 langcode_widget-2533964-31-test.patch5.38 KBedurenye
#27 interdiff-langcode_widget-2533964-24-27.txt7.08 KBedurenye
#27 langcode_widget-2533964-27.patch8.27 KBedurenye
#27 langcode_widget-2533964-27-test.patch4.78 KBedurenye
#24 interdiff-langcode_widget-2533964-21-24.txt3.72 KBedurenye
#24 langcode_widget-2533964-24.patch4.43 KBedurenye
#24 langcode_widget-2533964-24-test.patch2.67 KBedurenye
#21 interdiff-langcode_widget-2533964-18-21.txt3.47 KBedurenye
#21 langcode_widget-2533964-21.patch5.47 KBedurenye
#21 langcode_widget-2533964-21-test.patch3.71 KBedurenye
#18 interdiff-langcode_widget-2533964-12-18.txt1.48 KBedurenye
#18 langcode_widget-2533964-18.patch3.7 KBedurenye
#18 langcode_widget-2533964-18-test.patch1.94 KBedurenye
#12 interdiff-langcode_widget-2533964-10-12.txt724 bytesedurenye
#12 langcode_widget-2533964-12.patch3.61 KBedurenye
#12 langcode_widget-2533964-12-test.patch1.85 KBedurenye
#10 interdiff-langcode_widget-2533964-6-10.txt739 bytesedurenye
#10 langcode_widget-2533964-10.patch3.61 KBedurenye
#9 test-langcode_widget-2533964-9.patch1.85 KBedurenye
#6 langcode_widget-2533964-6.patch3.63 KBedurenye
#6 interdiff-langcode_widget-2533964-1-6.txt1.87 KBedurenye
#1 contact-message-langcode-2533964-1.patch1.76 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Correction. 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.

Berdir’s picture

Title: langcode widget from language.module and visibility settings are not applied consistently » langcode widget visibility settings from language.module are not applied consistently
Berdir’s picture

Title: langcode widget visibility settings from language.module are not applied consistently » Langcode widget visibility settings from language.module are not applied consistently
Berdir’s picture

Status: Active » Needs review
Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-content, +sprint

The 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.

edurenye’s picture

Gábor Hojtsy’s picture

Can you post the test only patch as well? Reviewing the test it looks good, only a minor thing found:

+++ b/core/modules/contact/src/Tests/ContactLanguageTest.php
@@ -0,0 +1,61 @@
+    // Ensure that contact form by default does not show the language select.

Comment is incorrect (the second time; looks like copy-paste).

Gábor Hojtsy’s picture

Status: Needs review » Needs work
edurenye’s picture

Sorry for this mistake.
Here is just the test patch.
Next comment will post final patch and interdiff.

edurenye’s picture

Status: Needs work » Needs review
FileSize
3.61 KB
739 bytes

Here is the patch and interdiff.

Gábor Hojtsy’s picture

The 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:

+++ b/core/modules/contact/src/Tests/ContactLanguageTest.php
@@ -0,0 +1,61 @@
+    // Ensure that contact form now show the language select.

show => shows

The last submitted patch, 12: langcode_widget-2533964-12-test.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Superb looks great! Thanks!

The last submitted patch, 9: test-langcode_widget-2533964-9.patch, failed testing.

penyaskito’s picture

Issue tags: -Needs tests

Tests were added, removing tag.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
+++ b/core/modules/language/language.module
@@ -463,12 +463,12 @@ function language_form_alter(&$form, FormStateInterface $form_state) {
-  if ($form_object instanceof ContentEntityFormInterface) {
+  if ($form_object instanceof ContentEntityFormInterface && $form_object->getEntity()->getEntityType()->hasKey('langcode')) {
     /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
     $entity = $form_object->getEntity();
     $entity_type = $entity->getEntityType();
     $langcode_key = $entity_type->getKey('langcode');
-    if (isset($form[$langcode_key]) && $entity_type->isTranslatable()) {
+    if (isset($form[$langcode_key])) {

Do we have any tests where an entity does not have a langcode? Tests for this should be in the language module.

edurenye’s picture

I 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.

The last submitted patch, 18: langcode_widget-2533964-18-test.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@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.

edurenye’s picture

Ok, I added a test for the empty langcode.

Gábor Hojtsy’s picture

Well, $form_object->getEntity()->getEntityType()->hasKey('langcode') is not about a specific entity not having a langcode but an entity type not supporting language.

The last submitted patch, 21: langcode_widget-2533964-21-test.patch, failed testing.

edurenye’s picture

The last submitted patch, 24: langcode_widget-2533964-24-test.patch, failed testing.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/language/src/Tests/LanguageEntityFormTest.php
    @@ -0,0 +1,78 @@
    + * Tests entity form with language module.
    + *
    + * This is to ensure that an entity form by default does not show the language
    + * select, but it does so when it's enabled from the content language settings
    + * page.
    + * In this test we test it with the particular cases of the contact messages
    + * and with node.
    ...
    +  public function testContactLanguage() {
    

    Classname, 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 ?)

  2. +++ b/core/modules/language/src/Tests/LanguageEntityFormTest.php
    @@ -0,0 +1,78 @@
    + * @group contact
    

    @group is now wrong.

  3. +++ b/core/modules/language/src/Tests/LanguageEntityFormTest.php
    @@ -0,0 +1,78 @@
    +    $edit['settings[entity_test][entity_test][settings][language][language_alterable]'] = TRUE;
    

    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...

edurenye’s picture

I 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?

Berdir’s picture

I'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.

The last submitted patch, 27: langcode_widget-2533964-27-test.patch, failed testing.

The last submitted patch, 27: langcode_widget-2533964-27-test.patch, failed testing.

edurenye’s picture

Ok, now it has real no language support, I splitted the tests and assert the right thing.

The last submitted patch, 31: langcode_widget-2533964-31-test.patch, failed testing.

Berdir’s picture

Leaving at needs work to get feedback on point 2. and 3.

  1. +++ b/core/modules/language/src/Tests/EntityTypeWithoutLanguageFormTest.php
    @@ -0,0 +1,60 @@
    +  public function testEmptyLangcode() {
    +    // Create an entity without language definition.
    +    $entity = NoLanguageEntityTest::create();
    +    $entity->save();
    

    We don't need this part, except if we actually want to edit it, see below.

  2. +++ b/core/modules/language/src/Tests/EntityTypeWithoutLanguageFormTest.php
    @@ -0,0 +1,60 @@
    +    // Assert that we can not enable language select from
    +    // content language settings page.
    +    $this->drupalGet('admin/config/regional/content-language');
    +    $this->assertNoField('entity_types[no_language_entity_test]');
    

    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.

  3. +++ b/core/modules/language/src/Tests/LanguageEntityFormTest.php
    @@ -0,0 +1,70 @@
    +/**
    + * Tests entity form with language module.
    + *
    + * This is to ensure that an entity form by default does not show the language
    + * select, but it does so when it's enabled from the content language settings
    + * page.
    + * In this test we test it with the particular cases of the contact messages.
    + *
    + * @group language
    + */
    +class LanguageEntityFormTest extends WebTestBase {
    

    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?

Gábor Hojtsy’s picture

Agreed with Berdir :)

Berdir’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Ok, setting to NW then for removing the entity create call and moving the test back.

edurenye’s picture

The last submitted patch, 36: langcode_widget-2533964-36-test.patch, failed testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me, maybe wait for Gabor to sign of too :)

Gábor Hojtsy’s picture

Looks good to me too :) Thanks a lot!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

diff --git a/core/modules/language/src/Tests/EntityTypeWithoutLanguageFormTest.php b/core/modules/language/src/Tests/EntityTypeWithoutLanguageFormTest.php
index b264d28..d17cc24 100644
--- a/core/modules/language/src/Tests/EntityTypeWithoutLanguageFormTest.php
+++ b/core/modules/language/src/Tests/EntityTypeWithoutLanguageFormTest.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\language\Tests;
 
-use Drupal\language_test\Entity\NoLanguageEntityTest;
 use Drupal\simpletest\WebTestBase;
 
 /**

Not used. Fixed on commit.

+++ b/core/modules/language/src/Tests/EntityTypeWithoutLanguageFormTest.php
diff --git a/core/modules/language/tests/language_test/src/Entity/NoLanguageEntityTest.php b/core/modules/language/tests/language_test/src/Entity/NoLanguageEntityTest.php
new file mode 100755

new file mode 100755
index 0000000..5018784

index 0000000..5018784
--- /dev/null

--- /dev/null
+++ b/core/modules/language/tests/language_test/src/Entity/NoLanguageEntityTest.php

Fixed file mode to be 644 on commit.

  • alexpott committed bd1964c on 8.0.x
    Issue #2533964 by edurenye, Berdir, Gábor Hojtsy: Langcode widget...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, superb!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.