Follow-up to #2153937: Default language setting is hard to find

Problem/Motivation

Current implementation does not validate that new default language exists, this leads to notice and wrong assumption for user that settings saved.

Notice: Undefined index: af in Drupal\language\LanguageListBuilder->submitForm() (line 150 of core/modules/language/src/LanguageListBuilder.php).
Drupal\language\LanguageListBuilder->submitForm(Array, Object)

Steps to reproduce (admin/config/regional/language):
1) add second language
2) open the same page in second tab and delete it
3) on first tab select deleted language and try to submit form

Also reviewing a code:
Config factory should be properly injected.
Also form redirect should use entity url.

Proposed resolution

Add validation to form.
cover validation with tests.
Clean-up code.

Remaining tasks

Review. Test. Commit.

User interface changes

no

Comments

andypost’s picture

Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new3.95 KB

Something like that

andypost’s picture

StatusFileSize
new807 bytes
new4.14 KB

seems reset only needed when default language changed

The last submitted patch, 1: 2420239-1.patch, failed testing.

andypost queued 1: 2420239-1.patch for re-testing.

andypost’s picture

StatusFileSize
new1.36 KB
new4.73 KB

a bit more cleanup

gábor hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs tests

First of all needs issue summary update on why is this a bug. No bugs are mentioned in the summary. If this is a bug, then a test is possible to write which reproduces the bug and proves this solves it.

  1. +++ b/core/modules/language/src/LanguageListBuilder.php
    @@ -36,13 +37,21 @@ class LanguageListBuilder extends DraggableListBuilder {
    +   * Stores the configuration factory.
    

    The configuration factory. -- is what we use elsewhere for short.

  2. +++ b/core/modules/language/src/LanguageListBuilder.php
    @@ -104,6 +116,10 @@ public function buildRow(EntityInterface $entity) {
    +    if ($entity->id() == $this->languageManager->getDefaultLanguage()->getId()) {
    

    Nice simplification.

andypost’s picture

Title: Default language setting should be saved only when changed » Default language setting form needs validation
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
StatusFileSize
new36.64 KB
new5.02 KB
new1.5 KB

Fixed IS and title.

New patch shows the bug, but no idea how to test that except manually (simpletest always replaces retrieved content)
Seems only manual testing applicable

Status: Needs review » Needs work

The last submitted patch, 7: 2420239-langlist-7.patch, failed testing.

andypost queued 7: 2420239-langlist-7.patch for re-testing.

gábor hojtsy’s picture

Ok, makes sense. Needs tests then :) Thanks!

Anonymous’s picture

StatusFileSize
new905 bytes

Like @andypost mentioned, it's hard to test with simpletest because handleForm() unsets values that are not available on the form but are posted anyway.

In the attached patch I try it anyway to demonstrate what I am trying to do. Any suggestion on how to improve this? Or is a totally different approach needed here?

gábor hojtsy’s picture

The test would fail if there was a notice, no? At least testbase has some code to augment that? Not sure where that is turned on for tests.

Anonymous’s picture

The test fails because I try to post "nl" and the handleForm() method unsets that value, making the test fail to set the default language to "nl". If I could work around that, I'd be able to finish the test. I'll try to find a test that has a similar problem.

Anonymous’s picture

Anonymous’s picture

Status: Needs work » Needs review

The last submitted patch, 11: default_language-2420239-11.test-fail.patch, failed testing.

The last submitted patch, 14: default_language_setting-2420239-14.test-fail.patch, failed testing.

gábor hojtsy’s picture

Status: Needs review » Needs work

Looks good! Some minor notes:

  1. +++ b/core/modules/language/src/LanguageListBuilder.php
    @@ -129,17 +137,25 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      $form_state->setErrorByName('site_default_language', $this->t('Selected language no longer exists.'));
    

    Should be "Selected default language no longer exists." IMHO. We never know if someone alters the form to select a language for some other reason, so the error needs to be more specific.

  2. +++ b/core/modules/language/src/Tests/LanguageListTest.php
    @@ -179,6 +179,13 @@ function testLanguageList() {
    +    $this->assertRaw('Selected language no longer exists.', 'A validation message is shown when setting an unexisting language as default.');
    

    Should be assertText(t()). You can remove the second string, the feedback should be fine based on the assertion.

Anonymous’s picture

Anonymous’s picture

Status: Needs work » Needs review
gábor hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

The last submitted patch, 19: default_language-2420239-19.test-fail.patch, failed testing.

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 2af2fb4 and pushed to 8.0.x. Thanks!

  • alexpott committed 2af2fb4 on 8.0.x
    Issue #2420239 by andypost, pjonckiere: Default language setting form...
gábor hojtsy’s picture

Yay, thanks all!

andypost’s picture

Issue tags: -Needs tests

Status: Fixed » Closed (fixed)

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