Problem/Motivation

LanguageSelectWidget hardcodes the list of languages to LanguageInterface::STATE_ALL. While that's a reasonable default it should be possible to configure it to use LanguageInterface::STATE_CONFIGURABLE (i.e. without Not specified and Not applicable).

Proposed resolution

Add a include_locked setting for LanguageSelectWidget with a corresponding checkbox in the settings form. We should probably make the label a bit more explicit, such as:

Include locked languages such as Not specified and Not applicable

Depending on that setting either use LanguageInterface::STATE_CONFIGURABLE or LanguageInterface::STATE_ALL as the #languages.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

An additional setting for form displays using the language widget.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

Issue tags: +Novice

Marking Novice. If you want to tackle this and need help please just ask here and I will provide detailed instructions.

tstoeckler’s picture

cebasqueira’s picture

Assigned: Unassigned » cebasqueira
cebasqueira’s picture

Hi @tstoeckler
Please send me more information about the settings form that was cited on proposed resolution

tstoeckler’s picture

Sure.

In LanguageSelectWidget, you should add a defaultSettings() which provides an array with include_locked as key and the default value (I guess it should be TRUE in this case) as the value.

Then you should add a settingsForm() method which returns a form to configure this setting, so it should just contain a single include_locked setting which is just a checkbox.

Then in formElement() you should replace the value of the #languages key with $this->getSetting('include_locked').

Does that help?

juancasantito’s picture

I think this could be a good start for this.

juancasantito’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: languageselectwidget-2827784-7.patch, failed testing.

cebasqueira’s picture

cebasqueira’s picture

Assigned: cebasqueira » Unassigned
tstoeckler’s picture

Thanks @cebasqueira for the patch! Thanks also @juancasantito, your patch was almost identical, but had some identation problems, and since this was assigned to @cebasqueira I'm reviewing the last patch. It's best to always keep a watch on if an issue is already assigned to someone else or not.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/LanguageSelectWidget.php
@@ -27,10 +27,35 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      '#title' => $this->t('Include locked languages such as Not specified and Not applicable'),

I think we should wrap the "Not specified" and "Not applicable" in < em > tags.

Other than that, the patch looks great.

This will need some tests and an upgrade path for existing form displays, however. Removing the Novice tag for that, but do feel free to tackle that if you want. I will try to help.

cebasqueira’s picture

The new patch with "<em></em>"!

tstoeckler’s picture

That looks great, thanks! Now just needs tests and an upgrade path, as noted above.

By the way @cebasqueira, it is a best practice to always create an interdiff when you update a patch. That would be great to do, next time (no need to do it for this one, that's fine). More information on: https://www.drupal.org/documentation/git/interdiff

faline’s picture

Assigned: Unassigned » faline

Hi @tstoeckler, I will try to develop the test for this.
Could you help me with some directions?

Thank you

tstoeckler’s picture

@faline Unfortunately there are no tests at all for LanguageSelectWidget at the moment, so you would have to create a new test. Maybe you can look at OptionsWidgetsTest for inspiration. Basically we would need 2 test methods, one which just uses the default settings and asserts that the locked languages are there and then another test method which disables the include_locked setting and then asserts that the locked languages are not there.

Also putting this on the D8MI sprint board, as this is actively being worked on ATM.

faline’s picture

@tstoeckler I'm trying to create the Test but when I try to run it, I receive the error on log:

 ReflectionException: Class Drupal\language\Tests\LanguageSelectWidgetsTest does not exist in ReflectionClass->__construct() (line 328 of /private/var/www/html/dco/drupal8/core/modules/simpletest/src/TestDiscovery.php).
 

But I cannot figure out what is happen.

faline’s picture

The attached patch is just to help to find the problem with the test class

Wim Leers’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/LanguageSelectWidget.php
--- /dev/null
+++ b/core/modules/language/src/Tests/LanguageSelectWidgetTest.php

+++ b/core/modules/language/src/Tests/LanguageSelectWidgetTest.php
@@ -0,0 +1,108 @@
+class LanguageSelectWidget extends FieldTestBase {

Your classname doesn't match the filename. Hence the error output you see in #17 :)

faline’s picture

@Wim Leers I missed this! My bad! Thank you

faline’s picture

Assigned: faline » Unassigned

@tstoeckler I'm facing some problems with Drupal installation (I've already create some environments, but none works) for run some tests (included this one).
So I'm unassigned this issue so anyone else could work on it. I will continues try to run this test. If this works I will upload the correct patch.
Thank for your help and @Wim Leers.

faline’s picture

@tstoeckler I'm uploading what I did for test as we talked.

I hope this is on the right path.

tstoeckler’s picture

Status: Needs work » Needs review

Let's see what the bot thinks. Patch looks pretty good.

The last submitted patch, 22: languageselectwidget-2827784-22.patch, failed testing.

The last submitted patch, 13: languageselectwidget-2827784-13.patch, failed testing.

The last submitted patch, 18: languageselectwidget-2827784-18-TEST.patch, failed testing.

The last submitted patch, 10: languageselectwidget-2827784-10.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 22: languageselectwidget-2827784-22-testonly.patch, failed testing.

tstoeckler’s picture

Issue tags: +dcmuc16
tstoeckler’s picture

Nice work. Please don't be discouraged that I marked a big portion of the test below, the test is actually not that far from what we want, it just needs some tweaks that make a lot of lines of code.

Some ideas for improvement for the test:

  1. +++ b/core/modules/language/src/Tests/LanguageSelectWidgetTest.php
    @@ -0,0 +1,134 @@
    +namespace Drupal\language\Tests;
    ...
    +class LanguageSelectWidgetTest extends FieldTestBase {
    

    FieldTestBase is an old WebTestBase-based test. There is now a new replacement, called FieldKernelTestBase. It's very easy to miss, because there are so many different classes at so many different locations... :-/

    Looking at FieldKernelTestBase I'm not sure if we actually need it here, or we can just extend KernelTestBase directly. Then you would have to do, for example

    $this->installEntitySchema('entity_test');
    

    in setUp() but maybe not much more.

    Either way, the test will then be in the Drupal\Tests\language\Kernel and it will be located in core/modules/language/tests/src/Kernel

    Again, sorry that this is so weird...

  2. +++ b/core/modules/language/src/Tests/LanguageSelectWidgetTest.php
    @@ -0,0 +1,134 @@
    +  public static $modules = array(
    ...
    +  );
    

    Here, and elsewhere: It's not required, but it's nice to use the short array syntax ([]) for new classes.

  3. +++ b/core/modules/language/src/Tests/LanguageSelectWidgetTest.php
    @@ -0,0 +1,134 @@
    +    // Install Portuguese, Brazil language.
    +    $edit = array();
    +    $edit['predefined_langcode'] = 'pt-br';
    +
    +    $this->drupalPostForm(
    +      'admin/config/regional/language/add',
    +      $edit,
    +      t('Add language')
    +    );
    

    This is currently failing, I think, because you are not logged in as an administrator. Instead of creating it from the UI, though, we can create a language directly from code.

    Something like this (untested):

    $language = ConfigurableLanguage::createFromLangcode('pt-br');
    $language->save();
    
  4. +++ b/core/modules/language/src/Tests/LanguageSelectWidgetTest.php
    @@ -0,0 +1,134 @@
    +    $this->fieldLanguageLocked = FieldStorageConfig::create([
    +      'field_name' => 'language_locked',
    +      'entity_type' => 'entity_test',
    +      'type' => 'language_select',
    +      'languages' => LanguageInterface::STATE_ALL,
    +    ]);
    +
    +    $this->fieldLanguageLocked->save();
    +
    +    $this->fieldLanguageUnlocked = FieldStorageConfig::create([
    +      'field_name' => 'language_unlocked',
    +      'entity_type' => 'entity_test',
    +      'type' => 'language_select',
    +      'languages' => LanguageInterface::STATE_CONFIGURABLE,
    +    ]);
    +
    +    $this->fieldLanguageUnlocked->save();
    

    So if we use the entity_test entity type we can directly use the language base field that that provides and don't have to add any fields ourselves. We simply have to configure the form display to use the widget with a particular setting.

    Adapting this from similar code in OptionsWidgetsTest you could try something like the following (untested):

        entity_get_form_display('entity_test', 'entity_test', 'default')
          ->setComponent('language', [
            'type' => 'language_select',
          ])
          ->save();
    

    (That's without the setting. And then you would have to add the setting in the other test method.)

  5. +++ b/core/modules/language/src/Tests/LanguageSelectWidgetTest.php
    @@ -0,0 +1,134 @@
    +    // Create an instance of the 'single value' field.
    +    $field = FieldConfig::create([
    +      'field_storage' => $this->fieldLanguageLocked,
    +      'bundle' => 'entity_test',
    +    ]);
    +
    +    $field->save();
    ...
    +    // Create an instance of the 'single value' field.
    +    $field = FieldConfig::create([
    +      'field_storage' => $this->fieldLanguageUnlocked,
    +      'bundle' => 'entity_test',
    +    ]);
    +
    +    $field->save();
    

    Again, this should not be needed, because we can just re-use the existing langcode field.

  6. +++ b/core/modules/language/src/Tests/LanguageSelectWidgetTest.php
    @@ -0,0 +1,134 @@
    +    // Create an entity.
    +    $entity = EntityTest::create(array(
    +      'user_id' => 1,
    +      'name' => $this->randomMachineName(),
    +    ));
    +    $entity->save();
    ...
    +    // Create an entity.
    +    $entity = EntityTest::create(array(
    +      'user_id' => 1,
    +      'name' => $this->randomMachineName(),
    +    ));
    +    $entity->save();
    

    This is not necessarily wrong, but I wonder if we could just go to the add form and do the assertions there? That should work just the same, I think?

  7. +++ b/core/modules/language/src/Tests/LanguageSelectWidgetTest.php
    @@ -0,0 +1,134 @@
    +    $this->assertOptionSelected('edit-language-locked', LanguageInterface::LANGCODE_NOT_APPLICABLE);
    +    $this->assertOptionSelected('edit-language-locked', LanguageInterface::LANGCODE_NOT_SPECIFIED);
    +    $this->assertOptionSelected('edit-language-locked', 'pt-br');
    ...
    +    $this->assertNoOptionSelected('edit-language-locked', LanguageInterface::LANGCODE_NOT_APPLICABLE);
    +    $this->assertNoOptionSelected('edit-language-locked', LanguageInterface::LANGCODE_NOT_SPECIFIED);
    +    $this->assertOptionSelected('edit-language-locked', 'pt-br');
    

    I think we don't actually want to assert that the option is selected (or not), but that it exists (or not) so I think we should be using $this->assertOption() and $this->assertNoOption()

Hopefully that will allow you or someone else to move this along. Also re-marking "Novice" as - now that you've put the test in place, I think it's a Novice task to fix at least some of the issues I noted above, even if some might be a bit trickier.

tstoeckler’s picture

Issue tags: +Novice
faline’s picture

@tstoeckler thank you for point out what should change on test code!
As soon as I have time I will try to do these changes. If someone else do it first, that is no problem either.

Thanks! :)

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

maxocub’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Novice
FileSize
4.36 KB
6.71 KB

Here's a new version of the test using KernelTestBase.

maxocub’s picture

Status: Needs review » Needs work

Back to Needs work to add an upgrade path.

maxocub’s picture

Status: Needs work » Needs review
Issue tags: -Needs upgrade path
FileSize
5.34 KB
999 bytes

I'm not sure this is the best way to do the upgrade path, but that's what I've got for now.

Status: Needs review » Needs work

The last submitted patch, 36: 2827784-36.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review
FileSize
5.38 KB
741 bytes
maxocub’s picture

Here's a test for the update path.

Status: Needs review » Needs work

The last submitted patch, 39: 2827784-39.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review
FileSize
7.19 KB
2.86 KB

Changed the hook_update_N() to a hook_post_update_NAME(), as suggested by @tstoeckler on IRC.

Status: Needs review » Needs work

The last submitted patch, 41: 2827784-41.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review

CI error, back to NR.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thank you so much. Looks absolutely perfect.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/language/language.post_update.php
    @@ -0,0 +1,33 @@
    +    $content = $display_form->get('content');
    +    foreach (array_keys($content) as $element) {
    +      if (isset($content[$element]['type']) && $content[$element]['type'] == 'language_select') {
    +        $content[$element]['settings'] = ['include_locked' => TRUE];
    +        $display_form->set('content', $content);
    +        $display_form->save();
    +      }
    +    }
    

    This looks a bit odd - might we save a entity display twice?

    How about we do something like:

      foreach (EntityFormDisplay::loadMultiple() as $display_form) {
        $content = $display_form->get('content');
        $changed = FALSE;
        foreach (array_keys($content) as $element) {
          if (isset($content[$element]['type']) && $content[$element]['type'] == 'language_select') {
            $content[$element]['settings'] = ['include_locked' => TRUE];
            $changed = TRUE;
          }
        }
        if ($changed) {
          $display_form->set('content', $content);
          $display_form->save();
        }
      }
    
  2. +++ b/core/modules/language/language.post_update.php
    @@ -0,0 +1,33 @@
    +        $content[$element]['settings'] = ['include_locked' => TRUE];
    

    There's no chance that settings will have any thing in?

  3. +++ b/core/modules/language/src/Tests/Update/LanguageSelectWidgetUpdateTest.php
    @@ -0,0 +1,55 @@
    +    foreach (EntityFormDisplay::loadMultiple() as $display_form) {
    +      $content = $display_form->get('content');
    +      foreach (array_keys($content) as $element) {
    +        if (isset($content[$element]['type']) && $content[$element]['type'] == 'language_select') {
    +          $this->assertEqual($expected_settings, $content[$element]['settings']);
    +        }
    +      }
    +    }
    

    This test is prone to failure :( we can't be sure that an assert is made. Because of all the foreaches and if... Just load a specific entity form display and check before and after.

maxocub’s picture

Status: Needs work » Needs review
FileSize
6.88 KB
2.6 KB

Made the changes from #45.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, thanks for the review @alexpott and the quick turnaround @maxocub!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b67ccdc and pushed to 8.4.x. Thanks!

  • alexpott committed b67ccdc on 8.4.x
    Issue #2827784 by maxocub, faline, cebasqueira, juancasantito,...
Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks all, this looks great :)

Status: Fixed » Closed (fixed)

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

Tom Voesenek’s picture

FileSize
6.86 KB

I've made a version of the patch from #46 for 8.3.x, so it can be used there too.

maxocub’s picture