Follow up for #2014955: Deleted bundles do not have their language configuration deleted

Problem/Motivation

If you create a new node type, block content, vocabulary or comment type you get a entry without a machine name in language.settings.yml and the setting is not saved when editing it later.

There might be more entities that need custom submit handlers added to prevent empty machine names in saved language.settings.yml. Currently done is node type, block content, vocabulary and comment type.

Steps to reproduce:

  1. Install D8
  2. Enable the Language Module
  3. Add a content type, setting the default language to "English" (not site default)
  4. View sites/default/files/config_*/staging/language.settings.yml (after exported with e.g. drush cex)
    • Expected result: it contains an entry for the new content type.
    • Actual result: it doesn't.
  5. Edit the created content type and check if the default language is set to "English"
    • Expected result: "English"
    • Actual result: "Site default"

Proposed resolution

Identity which,
or add something so that that is not necessary.

Remaining tasks

Discuss if there is a better solution than using submit hooks, which needs to be added manully.

User interface changes

No.

API changes

TBD.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kfritsche’s picture

Steps to reproduce:

1. Current/fresh installation with content translation enabled. OR Check sites/default/config_*/language.settings.yml - There shouldn't be a '' under node.
2. Create a new node bundle - test (admin/structure/types/add).
3. Check sites/default/config_*/language.settings.yml.
- There is no bundle test under node, but a '' with the default language configuration.
- There is no bundle comment_node_test under comment
4. Set test bundle to be translatable in Content language settings (admin/config/regional/content-language)
5. Check sites/default/config_*/language.settings.yml again.
- There is now the bundle test under node, but '' still exists.
- There is now the bundle comment_node_test under comment.
6. Rename the machine name (!) of the test bundle to test_new
7. Check sites/default/config_*/language.settings.yml again.
- There is now the bundle test_new under node, but '' still exists.
- There is still the bundle comment_node_test under comment and not comment_node_test_new.
8. Just save settings in Content language (admin/config/regional/content-language)
9. Check sites/default/config_*/language.settings.yml again.
- There is now the bundle test_new under node, but '' still exists.
- There is now the bundle comment_node_test under comment and comment_node_test_new.

marthinal’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
2.73 KB

I'm using the patch from #2014955: Deleted bundles do not have their language configuration deleted by Karl and Cathy. I think here the problem is that we don't have the type(bundle name) when the form is built and we need it. Maybe it is a good idea to use the form_state values in this case?

-      'bundle' => $element['#entity_information']['bundle'],
+      'bundle' =>  $form_state['values']['type'],

The behavior looks correct.

Status: Needs review » Needs work

The last submitted patch, 2: 2015615-3.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
4.62 KB

Added submit handler to node (entity type). I was debugging on a minimal installation without comment module enabled, so probably we need to take a look at the comment module too. Firstly let's take a look at this patch. We are writing the same behavior for different entities (menu, taxonomy, nodes...) maybe it is possible to do this directly for the entities and only in one place (inside the submit handler at the language module (language_configuration_element_submit())). Tests that have been failing, now are green at my localhost and checking manually language.settings.yml the bundle (node type) seems to be added as expected and removed too. The patch is different so we do not need an interdiff in this case.

marthinal’s picture

Status: Needs review » Needs work

Berdir, YesCT and I had a discussion about it.

Each time we are using the same method languageConfigurationSubmit() adding this:

$form_state['language']['language_configuration']['bundle'] = $form_state['values']['type'];

The id is different per entity (i.e. node use type, Vocabulary use vid, menu use id) and it is possible to obtain this id using the property "bundle_of". This property is not actually in all the entities so we can only use it with node, custom block, vocabulary and not with menu or comments.

So the first step is to fix this problem using the submit handler. Add a test per entity and when fixed we can start thinking about do this in a smart way using bundle_of.

marthinal’s picture

Status: Needs work » Needs review

4: 2015615-4.patch queued for re-testing.

marthinal’s picture

Attached test + patch.

So let's fix this bug and then open a new issue to remove the submit handler as explained at #5. Probably the best way is using a hook_entity_insert().

Status: Needs review » Needs work

The last submitted patch, 7: 2015615-7.patch, failed testing.

The last submitted patch, 7: 2015615-7-only-test-should-fail.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review

7: 2015615-7.patch queued for re-testing.

YesCT’s picture

I'm reviewing this now.

YesCT’s picture

  1. +++ b/core/modules/language/language.module
    @@ -245,6 +245,8 @@ function language_configuration_element_process($element, &$form_state, &$form)
    +  // @todo On bundle create $element['#entity_information']['bundle'] is empty.
    +  //   http://drupal.org/node/2015615
    

    We will not need this @todo, that is a link to this issue.

  2. *Question*
    +++ b/core/modules/language/language.module
    @@ -327,7 +329,7 @@ function language_get_default_configuration($entity_type, $bundle) {
    -  \Drupal::config('language.settings')->clear(language_get_default_configuration_settings_key($entity_type, $bundle))->save();
    +  \Drupal::config('language.settings')->clear(language_get_default_configuration_bundle_key($entity_type, $bundle))->save();
    
    $ ag "language_get_default_configuration_settings_key\(" core
    core/modules/language/language.module
    296:  \Drupal::config('language.settings')->set(language_get_default_configuration_settings_key($entity_type, $bundle), array('langcode' => $values['langcode'], 'language_show' => $values['language_show']))->save();
    313:  $configuration = \Drupal::config('language.settings')->get(language_get_default_configuration_settings_key($entity_type, $bundle));
    347:function language_get_default_configuration_settings_key($entity_type, $bundle) {
    
    core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.php
    166:          $config->set(language_get_default_configuration_settings_key($entity_type, $bundle),
    

    Why do we set() and get() with the settings_key (with '.language.default_configuration',
    but clear() uses the bundle_key instead?

  3. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,51 @@
    + * @file
    + * Definition of Drupal\language\Tests\LanguageSettingsTest.
    

    nit, @file uses Contains now.
    https://drupal.org/node/1354#file
    fixed.

  4. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,51 @@
    +use Drupal\Core\Language\Language;
    

    un-used use.
    fixed.

  5. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,51 @@
    + * Functional tests for language configuration's effect on negotiation setup.
    

    I think this was cut and paste'd from another test.

    nit: Fixed to start with third person verb.
    "Use a third person verb to start the summary of a class, interface, or method. For example: "Represents a ..." or "Provides..."."
    https://drupal.org/node/1354#classes

    actual fix: correct what this tests.

    fixed.

  6. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,51 @@
    +
    +  public static function getInfo() {
    

    getInfo() (and setUp()) have @inheritdoc now.
    https://drupal.org/node/325974
    #2007766: {@inheritdoc} in tests on setUp and getInfo

    fixed.

  7. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,51 @@
    +   * Functional test for review language settings file.
    

    nit.
    functions third person verb to say what they do.

    fixed.

  8. *needs work here*
    +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,51 @@
    +    $this->drupalPostForm('admin/structure/types/add', $edit, t('Save and manage fields'));
    +    $type_exists = (bool) entity_load('node_type', 'foo');
    +    $this->assertTrue($type_exists, 'The new content type has been created in the database.');
    +    $language_settings = \Drupal::config('language.settings')->get('node.foo');
    +    $this->assertTrue($language_settings, 'The language settings have been found.');
    

    a)

    +    $this->assertTrue($language_settings, 'The language settings have been found.');
    

    assertTrue doesn't seem to be testing what we want it to test.

    -get() should return the setting. Let's check that, instead of checking if it evaluates to true.

    b)
    I also think we should test changing the setting, and seeing if it gets the new setting.

    c)
    And also deleting it and seeing if the setting is gone from the file.

    a, b, c, not fixed yet.

  9. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,51 @@
    +  }
    +}
    

    nit, newline at end of class
    "Leave an empty line between end of method and end of class definition:"
    https://drupal.org/node/608152#indenting

    fixed.

  10. +++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.php
    @@ -206,4 +220,14 @@ public function save(array $form, array &$form_state) {
    +   * Submit handler to update the bundle for the default language configuration.
    

    I'm not sure how we should name this. Or what docs for @params are needed.

    https://drupal.org/node/1354#functions
    didn't really help.
    But this is the same as was in #1945226: Add language selector on menus.

    ok.

---
summary:
Question: 2.
Concern: 8.

---
next: I'll try it manually.

marthinal’s picture

@YesCT thanks for your help :)

2)

-  config('language.settings')->clear(language_get_default_configuration_settings_key($entity_type, $bundle))->save();
+  config('language.settings')->clear(language_get_default_configuration_bundle_key($entity_type, $bundle))->save();
-  return $entity_type . '.' . $bundle . '.language.default_configuration';
+  return $entity_type . '.' . $bundle;

We want to clear using the bundle key (foo on the test attached).

8)
a) Checking the values.

b) Changing the settings.

c) Delete the bundle and verify that the settings were removed as expected.

ultimike’s picture

Removed whitespace and added newline between function and end of class ("https://drupal.org/node/608152#indenting). I did this because YesCT harassed me.

Oh wait, I'm supposed to say, "YesCT made me to teach me how to make an interdiff".

Interdiff and patch attached.

-mike

YesCT’s picture

I'm reviewing this now.

cordoval’s picture

  1. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,108 @@
    +     $admin_user = $this->drupalCreateUser(array('access administration pages', 'administer languages', 'administer content translation', 'administer content types', 'administer node fields', 'bypass node access'));
    

    shouldn't this line be broken into a multiline array?

  2. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,108 @@
    +    $this->drupalLogin($admin_user);
    

    indentation mess up

  3. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,108 @@
    +          'language_show' => 0
    

    missing trailing comma

  4. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,108 @@
    +    $this->assertTrue($language_settings, 'The language settings have been found.');
    

    if you are using the second argument why not removing the comment above?

  5. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,108 @@
    +      'settings[node][foo][fields][title]' => TRUE
    

    missing trailing comma

  6. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSettingsTest.php
    @@ -0,0 +1,108 @@
    +        'language_show' => 1
    

    same

YesCT’s picture

working with @cordoval to find the standards links to reference those things, and make a new patch with an interdiff for those things. (since they are not core gates, we should just fix them as part of reviewing)

Gaelan’s picture

shouldn't this line be broken into a multiline array?

I searched for other instances of this pattern, and all of the other ones are one line:

gbs ~/D/drupal → ag -Q '$admin_user = $this->drupalCreateUser(array(' 2015615 ✚1
core/modules/action/lib/Drupal/action/Tests/ActionUninstallTest.php
42:    $admin_user = $this->drupalCreateUser(array('administer users'));

core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorRenderingTest.php
42:    $admin_user = $this->drupalCreateUser(array(

core/modules/aggregator/lib/Drupal/aggregator/Tests/ImportOpmlTest.php
33:    $admin_user = $this->drupalCreateUser(array('administer news feeds', 'access news feeds', 'create article content', 'administer blocks'));

core/modules/ban/lib/Drupal/ban/Tests/IpAddressBlockingTest.php
34:    $admin_user = $this->drupalCreateUser(array('ban IP addresses'));

core/modules/block/lib/Drupal/block/Tests/BlockAdminThemeTest.php
37:    $admin_user = $this->drupalCreateUser(array('administer blocks', 'administer themes'));

<trimmed>
if you are using the second argument why not removing the comment above?

@YesCT said we should keep it, because the comment explicitly mentions the filename.

I think that all of the problems in #18 are fixed now.

Gaelan’s picture

Here's a test-only version.

Gaelan’s picture

Like the last test-only version, except this one actually only contains tests.

Status: Needs review » Needs work

The last submitted patch, 20: drupal.empty-machine-name.2015615.19.test-only.FAIL_.patch, failed testing.

Gaelan’s picture

Status: Needs work » Needs review
Gaelan’s picture

Issue summary: View changes

Added steps to reproduce.

Gaelan’s picture

I think this is RTBC for nodes. If we also want to do comments, etc., within this same issue, then this Needs Work.

YesCT’s picture

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

talked to alexpott, we will do comments and others here in this issue.

---
since we are going to do the other stuff here, we need to update the motivation section of the issue summary (and other parts that might be confusing).

I need some clarification on reproducing the empty machine names.

iirc, I think we need to do comments and custom blocks, but we need to check this.

kfritsche’s picture

Reworked the old patch.

Moved the tests to the alredy existing language config tests, so we don't need to create a complete new test class.

Also removed the strange entity_node_type_update function and did it, like we do in vocabularies, which is now also used for block types and comment types.

Added the necessary submit hooks every where, as I don't have any better solution yet and this solution works and is already used in core for that matter multiple times. I just unified it and used it everywhere...

Talked with Gabor, Andre and yched about the issue, because it feels wrong for me what we are doing here with the submit hooks. They agreed but nobody had another better solution yet, as a form element can't have a own submit handler. Which makes most of the time completley sense, but something like that we would need here.
We can't use any update/create hooks, as we don't have the form values there anymore and we don't know for which entity types we would need to do that. So a better generic solution is still needed.

Also updated issue summary.

Status: Needs review » Needs work

The last submitted patch, 26: drupal.empty-machine-name.2015615.26.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Postponed (maintainer needs more info)
Issue tags: -sprint

Does this still happen at all now that #2355909: language.settings config is not scalable landed?

Gábor Hojtsy’s picture

Status: Needs work » Postponed (maintainer needs more info)
Issue tags: -sprint

Does this still happen at all now that #2355909: language.settings config is not scalable landed?

Gábor Hojtsy’s picture

Also even the tests don't apply anymore unfortunately.

penyaskito’s picture

Checked the tests added here, and I think we have the same coverage after #2355909: language.settings config is not scalable. Is there anything else we can save from here so contributors get recognized?

Gábor Hojtsy’s picture

Status: Postponed (maintainer needs more info) » Closed (duplicate)

@panyaskito: you did #2355909: language.settings config is not scalable, so if you don't see what can be saved from here, than it is none. Thanks all for working on this one, and sorry it did not make it, and the refactoring landed. :/ It is at least a better solution for Drupal overall.