language.settings config is not scalable

Funny that we had some feeling to go the opposite direction. Let me explain why. This screen actually configures lots of things at once.

1. Default language value for an entity bundle.
2. Language widget visibility for an entity bundle (this normally would be on the form widget configuration).
3. Translation setting for each field on each entity bundle (this would be n times the screen visits around the field settings if not on this page).

People we have shown this screen **totally loved it**. In fact we originally implemented it in Barcelona on suggestion from webchick with design help from Bojhan when webchick was outraged that you would need to go into each field one by one to configure translatability (and if you have hundreds of bundles, then you have hundreds if not thousands of fields).

Of course none of these helps if it does not stand actual use in real life scenarios.

We *also* have this setting on the node type / entity bundle forms. The UX problem with that is you enable translatability there but then you still need to go in and set it for your fields as appropriate.

Original issue where this screen was introduced is #1810386: Create workflow to setup multilingual for entity types, bundles and fields.

Also is it a *feeling* that this causes performance issues or are there some numbers?

#2306087: Entity configuration form allows to enable translation without any translatable field is the opposite UX question of this issue that when you enable the bundle you don't have translatable fields yet, etc. (Tried to add it to related issues several times but drupal.org is not cooperating).

Splitting the config object is fine with me. In fact it would allow to ship this setting with a bundle in a module, which is not possible ATM with the current setup purely in config.

I was always wondering whether we could put that information somehow on the bundle entities using the third_party mechanism? This would probably conflict though
with the point in #8?

@dawehner: is there a generic way to do that though? Node types and vocabularies support 3rd party settings. Block types and comment types don't. Users don't even have a type config entity to store this data on. I don't think it conflicts with #8 if it would be at all possible because I think we consider shipping settings at once, not module A shipping the Article content type and module B shipping that it is translatable, but module C shipping the Page content type AND that it is translatable. (Neither scenarios are possible with the current config setup).

Yeah, also you can have bundles which are not config entities so that's not really an option.

I think we should make these language configurations little config entities. We already have config entities for entity fields, entity displays, base field overrides.... so this would not really be novel.

That also gives us the whole CRUD infrastructure for free and language settings don't have to be a one-off anymore.

Then we could even implement ThirdPartySettingsInterface in those config entities so that content_translation could store its stuff there. That would be pretty neat IMO.

Tagging

Yeah, also you can have bundles which are not config entities so that's not really an option.

Catch and I have discussed making that impossible.

But then there are entity types which are not bundleable

Given #11 #13 I think we should proceed with a new config entity to handle this.

I'm working on this one.

Catch and I have discussed making that impossible.

I don't think that is a good idea. It's the recommended way, but I don't see why we would need to limit ourself to that. Maybe some entity types have a fixed set of bundles that can't be changed or it depends on something else.

That's actually bad. I expect hundreds of bundles on most sites I will be working with and I wouldn't be surprised to see thousands.

Hundreds could be a problem, thousands is not going to happen.

Config entity overviews don't scale, they have no pager, no search, no filtering. Let's say you have 500 node types, each of them has 10 fields. That gives you 5000 fields config entities, not even starting on field storages base field overrides if you have translations. Then it will be a lot more. Good luck with that.

"Not so bad" might be correct, but we have one problem: And that is the field map bottleneck. Everything else is loaded and cached by entity_type/bundle, but exactly *because* of that, generating the field map is crazy (loop over each fieldable entity type and each bundle of that, then fech fields). But this discussion doesn't belong here :)

Getting the ball rolling...

Created ContentLanguageSettings config entity, modified schema avoiding failures (config_inspector says it is not correct yet). Form loads and saves configuration as expected.

Fixed failures by removing forgotten references to language.settings config and language_get_default_configuration_settings_key function.

Fixed test at LanguageConfigSchemaTest::testValidLanguageConfigSchema(), not sure if this is needed for config entities and should be removed.

+++ b/core/modules/language/config/schema/language.schema.yml
@@ -101,6 +101,27 @@ language.entity.*:
+    entities:
+      type: sequence
+      label: 'Entity type'
+      sequence:
+        - type: mapping
+          label: 'Bundle'
+          mapping:

The schema here seems very confusing.

I'm not clear now if we want a config entity per entity type or per entity_type/bundle, I thought the second?

If that is the case, then I would expect the config entity to look like this:

id: node.article
entity_type: node
bundle: article
default_langcode: 'en'
language_show: TRUE
third_party_settings: array()

entity_type/bundle added as separate keys, not sure if necessary, but it is done in other config entities too. (view displays for example).

default_langcode instead of langcode, makes more sense, but would probably be a bigger API change when using the functions.

and third_party_settings because why not ;)

If we want to do it by entity_type, then the last two would be below bundles.article (and not entities.article)

Obviously the API needs to be updated to use them as entities, not raw config :)

While changing all those helper functions, I would suggest to also try to move more code to the config entity. For fields, we have added a loadByName() method, so that callers don't know to know about the exact structure of the node, so you could do something like ContentLanguageSettings::loadByBundle($entity_type, $bundle)->getDefaultLangcode(). We can keep the functions around as a BC wrapper if we want to.

Agreed with @Berdir that the idea was to have this by entity+bundle combo. That lets module foo ship with node type foo AND the language settings for node type foo. If all node language settings are in one config file then that is not possible. (I don't think the other scalability problems would apply there though :).

As for the default_langcode key, if it is a key on the top level of the file it NEEDS to NOT be langcode because langcode on the top level means the language of the config file itself (any labels, etc.), so we should not co-opt the same name.

As for the third party settings, @tstoeckler said above that content translation could store settings on it, but the translatability is also stored on the entity bundle because the entity system has that capability, so not sure we have a core use case? (Contrib definitely possible though).

As for the default_langcode key, if it is a key on the top level of the file it NEEDS to NOT be langcode because langcode on the top level means the language of the config file itself (any labels, etc.), so we should not co-opt the same name.

Right, so we actually have to rename it. For the record, I'm not too happy that we force that a config entity has a langcode. There are config entities that have no translatable things in them, like entity displays, and they also don't have a label, so forcing those two things over the config_schema/base class is a bit problematic. But either way, naming it differently, to not confuse it with that.

As for the third party settings, @tstoeckler said above that content translation could store settings on it, but the translatability is also stored on the entity bundle because the entity system has that capability, so not sure we have a core use case? (Contrib definitely possible though).

Yeah, we probably don't have a core use case, but we started adding this almost everywhere, and core only has only few uses, it's a system very much made for contrib :)

@Berdir: if we would not enforce the root langcode to be the langcode of the file and use the langcode key here for some other thing, it would be bad DX (confusing) anyway... almost all core files have some kind of human readable thing, mostly labels, so people will expect langcode to refer to that

Sorry, I probably wasn't clear. I made two different arguments:

1: Yes, I agree that default_langcode is better to avoid confusions and make it clear what it actually is.

2: This entity will also automatically *have* a langcode and a label (at least when exported), but that really doesn't make sense here, so we should not enforce it? This is because it is part of the default config_entity schema.

I misunderstood, that will make migrate easier too.

Re: @Berdir #23
Entity displays are a bad example, there could totally be translatable strings in an entity display (some formatter setting, or a fielgroup label) - entity displays are highly pluggable :-)

Same if we allow third party settings in this new config entity here, hard to say what people will put in there.

Regarding hardcoded "config system housekeeping" key names, I've been thinking recently that the real problem IMO is that once 8.0 is out, it will be very hard to add new ones if the need arises in 8.1 or even in 9, because there is no way to ensure that the new key name will not conflict with one that'in use in some config entity type somewhere. That far exceeds the scope of this issue here, but that might be a real problem down the line. We have put all properties (those required/used by the config system, and those specific to the domain logic of each config entity type) in the same namespace, thus locking us in :-(

Uploading new patch, using a config entity per entity_type-bundle.
My initial plan was creating an storage, but Berdir's suggestion at #21 has been followed instead.

No interdiff, as is mostly from scratch again. If you are curious, step-by-step is at #2360245-10: #2355909 helper.

Needs general clean-up still, but I want to know if I'm on the right track. uuid and langcode are not set, I have to look at how that works yet. Adding third-party settings is still needed too if there is consensus.

Yes, going in the right direction I think.

1. +++ b/core/modules/language/language.module
   @@ -230,28 +235,7 @@ function language_get_default_configuration($entity_type, $bundle) {
   +  ContentBundleLanguageSettings::deleteByEntityTypeBundle($entity_type, $bundle);
   

   Not sure if we really want a dedicated method for that instead of doing a load + delete.

2. +++ b/core/modules/language/src/Entity/ContentBundleLanguageSettings.php
   @@ -0,0 +1,137 @@
   + * Contains \Drupal\language\Entity\ContentLanguageSettings.
   ...
   + * Defines the ContentLanguageSettings entity.
   ...
   +class ContentBundleLanguageSettings extends ConfigEntityBase {
   

   class name doesn't match. Not sure if we need bundle in here and the label. I would go with LanguageContentSettings as well, but not too sure about that.

   Also not sure about label, but no idea what would be good.

3. +++ b/core/modules/language/src/Entity/ContentBundleLanguageSettings.php
   @@ -0,0 +1,137 @@
   + *   id = "content_language_settings",
   

   entity types/plugins should be prefixed by module name, so I would use language_content_settings.

4. +++ b/core/modules/language/src/Entity/ContentBundleLanguageSettings.php
   @@ -0,0 +1,137 @@
   +
   +  public function __construct(array $values, $entity_type = 'content_language_settings') {
   +    parent::__construct($values, $entity_type);
   +  }
   

   This is not necessary.

5. +++ b/core/modules/language/src/Entity/ContentBundleLanguageSettings.php
   @@ -0,0 +1,137 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function id() {
   +    return $this->entity_type . '.' . $this->bundle;
   +  }
   

   Have a look at how field/field storage do this. the id needs to be a defined property in the schema and here, so that it is stored.

6. +++ b/core/modules/language/src/Entity/ContentBundleLanguageSettings.php
   @@ -0,0 +1,137 @@
   +  public function setLanguageShow($language_show) {
   +    $this->language_show = $language_show;
   +
   

   Not sure about the property and method names here. Currently, this is about showing the language form, but I think @yched mentioned that this should be understood in a broader way, meaning, being able to select specific a language when an entity is created. Which would also include rest for example.

7. +++ b/core/modules/language/src/Entity/ContentBundleLanguageSettings.php
   @@ -0,0 +1,137 @@
   +   * @return boolean
   

   bool, not boolean.

8. +++ b/core/modules/language/src/Entity/ContentBundleLanguageSettings.php
   @@ -0,0 +1,137 @@
   +      $config->setDefaultLangcode(LanguageInterface::LANGCODE_SITE_DEFAULT)
   +        ->setLanguageShow(false);
   

   If you want those to be defaults, you should be able to just set them as property defaults on the class.

9. +++ b/core/modules/language/src/Entity/ContentBundleLanguageSettings.php
   @@ -0,0 +1,137 @@
   +  public static function deleteByEntityTypeBundle($entity_type_id, $bundle) {
   +    \Drupal::entityManager()->getStorage('content_language_settings')->load($entity_type_id . '.' . $bundle)->delete();
   +  }
   

   So you load directly to avoid the auto creation, but this will actually fail if it doesn't exist at the moment..

10. +++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
    @@ -145,16 +147,14 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +        $config = new ContentBundleLanguageSettings(['entity_type'=> $entity_type, 'bundle' => $bundle]);
    

    Ah, that is why you added a constructor. This is now how you create an entity, Use ContentBundleLanguageSettings::create($values) instead for example.

    If the missing id gives you trouble, implement preCreate() to set it baesd on entity_type/bundle

11. +++ b/core/modules/language/src/Tests/LanguageConfigSchemaTest.php
    @@ -65,11 +65,11 @@ function testValidLanguageConfigSchema() {
    -    $config_data = \Drupal::config('language.settings')->get();
    +    $config_data = \Drupal::config('language.content_settings.menu_link_content.menu_link_content')->get();
    

    Should use the entity API for this check?

Thanks for reviewing, Berdir!

1.

Not sure if we really want a dedicated method for that instead of doing a load + delete.

Done.

2.

class name doesn't match. Not sure if we need bundle in here and the label. I would go with LanguageContentSettings as well, but not too sure about that.
Also not sure about label, but no idea what would be good.

Renamed as ContentLanguageSettings. That is the name of the form, and the title of that page, so I think is more consistent. No strong feelings anyway.

3.

entity types/plugins should be prefixed by module name, so I would use language_content_settings.

Renamed. But is inconsistent with the class name and form then. Not sure what is best :-/

4.

(Constructor) is not necessary.

Removed, didn't know about ::create().

5.

Have a look at how field/field storage do this (id()). the id needs to be a defined property in the schema and here, so that it is stored.

Added $id and filled in preSave() if isNew(), like in field and field storage config.

6.

Not sure about the property and method names here. Currently, this is about showing the language form, but I think @yched mentioned that this should be understood in a broader way, meaning, being able to select specific a language when an entity is created. Which would also include rest for example.

How about language_exposed? I will ignore this for now, as it will require lots of changes difficulting the review.

7.

bool, not boolean.

Fixed.

8.

If you want those to be defaults, you should be able to just set them as property defaults on the class.

Done that.

9.

So you load directly to avoid the auto creation, but this will actually fail if it doesn't exist at the moment..

Fixed (logic in language_clear_default_configuration() now).

10.

Ah, that is why you added a constructor. This is now how you create an entity, Use ContentBundleLanguageSettings::create($values) instead for example.

Fixed.

11.

Should use the entity API for this check?

I'm not even sure if other config entities have this kind of tests. Leaving as is for now.

Oops

1. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,161 @@
   +  /**
   +   * The id. Combination of $entity_type.$bundle.
   +   *
   +   * @var string
   +   */
   +  protected $id;
   ...
   +   * {@inheritdoc}
   +   */
   +  public function id() {
   +    return $this->entity_type . '.' . $this->bundle;
   +  }
   +
   +  /**
   +   * Sets the default language code.
   

   Is there a reason to both store the $id and have a dedicated id() method doing the same? would it not be enough to return $this->id here?

2. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,161 @@
   +   * The entity type (machine name).
   ...
   +  protected $entity_type;
   

   we call that entity type ID and $entity_type_idin a lot of places ...

3. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,161 @@
   +  /**
   +   * Overrides \Drupal\Core\Entity\Entity::preSave().
   +   *
   +   * @throws \Drupal\Core\Field\FieldException
   +   *   If the field definition is invalid.
   +   * @throws \Drupal\Core\Entity\EntityStorageException
   +   *   In case of failures at the configuration storage level.
   +   */
   

   Do we need that kind of documentation at all? This should be also part of the parent class already ...

4. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,161 @@
   +    if ($this->isNew()) {
   +      $this->id = $this->id();
   +    }
   

   Ah, I take back my previous question ...

5. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,161 @@
   +    $config = \Drupal::entityManager()->getStorage('language_content_settings')->load($entity_type_id . '.' . $bundle);
   +    if ($config == NULL) {
   +      $config = ContentLanguageSettings::create(['entity_type' => $entity_type_id, 'bundle' => $bundle]);
   +    }
   

   is there any case where the ID might be something different than we expect it to be?

Thanks for reviewing @dawehner.

1. You answered in 4. See #29.5 too.

2. I tried with entityTypeId at first and that was the name of the property in Entity base class and everything broke. If I rename to entity_type_id instead, the getter becomes getEntityTypeId() and overrides the Entity getter for the said property. So my suggestion is leaving it as is.

3. No, this is the remaining from a copypaste from FieldConfig entity, I remove that.

4. :-)

5. No, it should be always like this, but ::preSave() and ::loadByEntityTypeBundle() hide the implementation details from any other class.

Before I forget:

• Still needs thirdpartysettings. Afaik is just adding the trait and schema change.
• Still needs to solve uuid and langcode problem, they are not saved. No idea yet how to solve.
• Maybe we need an interface for the config entity?
• Rename language_show to language_exposed? Only in the config entity? Or also in the arrays used in forms?

for the entity type property, in entity displays/modes, we use targetEntityType. That is afaik the only config entity property that is camel cased, so I'm not sure about that, but maybe target_entity_type_id? Don't really care about id or not, but target prefix should imho be there.

Also on the methods in case you added methods for getting/setting that.

FYI: opened #2362317: No namespacing of "system" config types for my remark in #27. Tangentially related to "langcode / default_langcode", "entity_type / target_entity_type" discussions, but should not really derail this issue here...

Opened #2363155: content_translation.settings config is not scalable for where the third party settings would be very useful. It has the same problem as language.settings...

Rerolled, conflicted with #2316561: type hint hooks with interface: ConfigurableLanguageInterface instead of LanguageEntity/ConfigurableLanguage.

I didn't reinstall my site while developing the patch and uuid and language were not generated. If I create a new node type, the language and uuid are exported correctly. Checked in simplytest.me and uuid and language are assigned correctly.

Renamed language_show to language_exposed everywhere...

Renamed $entity_type to $target_entity_type_id. Added getters for $target_entity_type_id and $bundle.

1. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,173 @@
   +  /**
   +   * The entity type ID (machine name).
   +   *
   +   * @var string
   +   */
   +  protected $target_entity_type_id;
   +
   +  /**
   +   * The bundle (machine name).
   +   *
   +   * @var string
   +   */
   +  protected $bundle;
   

   Should we rename $bundle to $target_bundle as well? (We went through a similar issue in #2346421: WTF with getTargetEntityId() / getBundle()).

2. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,173 @@
   +   * The id. Combination of $entity_type.$bundle.
   

   Needs to refer to the new variable names.

Added thirdpartysettings (as I'm not sure we should postpone on #2361775: Third party settings dependencies cause config entity deletion).

Fixed #44, thanks @effulgentsia! Two interdiffs attached as I saw the comment when I was going to upload.

From my list, the only thing remaining is adding a interface to ContentLanguageSettings, and I'm not sure now if we need that.
So IMHO this is ready for reviews.

Great work!

1. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,176 @@
   +
   

   Surplus blank line :)

2. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,176 @@
   +  protected $language_exposed = FALSE;
   

   Not sure whether this name gets it when we come to API/REST usage. As implemented in #2230637: Create a Language field widget and the related formatter, basically this determines whether language can be manually altered after being initialized. What about 'language_alterable' instead?

3. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,176 @@
   +  public function setLanguageExposed($language_show) {
   

   Param name mismatch.

4. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,176 @@
   +  public function differsFromDefaultConfiguration() {
   

   The negation in the name feels a bit weird, what about something like ::isOverridden()?

Thanks for reviewing, plach!

#47.2: Not sure about the name yet. Didn't change anything yet.

Fixed the rest of your points. For #47.4, what I did is reversing with isDefaultConfiguration().

1. +++ b/core/modules/language/language.module
   @@ -193,10 +195,17 @@ function language_configuration_element_submit(&$form, FormStateInterface $form_
    function language_save_default_configuration($entity_type, $bundle, $values = array()) {
   

   Should we mark those functions as deprecated and directly interact with the config entity instead?

   This one for example has 11 usages and most should be easy to convert, the code for most is already touched because we rename keys and if we use the load + fluent methods for this, it should result in more self-explaining, D8-like code.

   A few calls seem tricky, where it is being used directly from form values, or as @default_value for #type language_configuration, but we could keep anything non-trivial and update it in a follow-up issue.

2. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,175 @@
   +class ContentLanguageSettings extends ConfigEntityBase {
   

   Yes, we should add an interface. All entities have one, this shouldn't be an exception. Additionally to using the third party trait, we should also implement the corresponding interface (by extending from it in the interface, see other examples).

3. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,175 @@
   +   * @return static
   +   *   The content language config entity if one exists. Otherwise, returns
   +   *   sensible default values.
   

   Who says that they are "sensible" ? :) => Just "returns a new entity with default values"?

4. +++ b/core/modules/language/src/Tests/LanguageConfigSchemaTest.php
   @@ -56,20 +56,20 @@ function testValidLanguageConfigSchema() {
    
   -    $config_data = \Drupal::config('language.settings')->get();
   +    $config_data = \Drupal::config('language.content_settings.menu_link_content.menu_link_content')->get();
        // Make sure configuration saved correctly.
   -    $this->assertTrue($config_data['entities']['menu_link_content']['menu_link_content']['language']['default_configuration']['language_show']);
   +    $this->assertTrue($config_data['language_exposed']);
    
   

   I think the first part here should be updated to use the config entity API.

Any feedback on language_exposed/language_alterable/(other suggestion)?

@plach: @berdir had the same opinion, I will change that.

Renamed language_exposed to language_alterable. The getter is ::isLanguageAlterable now.

#49.1: Deprecated language_save_default_configuration, language_get_default_configuration, language_clear_default_configuration. I didn't change any usage, will do that in follow-up if you agree.

#49.2: Created Drupal\Core\Language\ContentLanguageSettingsInterface.

#49.3: Fixed.

#49.4: Fixed (I used BlockConfigSchemaTest as reference.

Oops, forgot the use.

While language_alterable sounds like a better name for the setting, I agree, it is not required to fix this issue, right? I mean we can rename it now, but at least lets keep the scope in check to do no more of these for this issue. I can see how renaming it is good to introduce an API / class that does not have renames already lined up.

The patch looks good to me. It does not have any new tests added, but it is validated by the existing tests to actually work. Not sure what kind of tests would be great, but validating the config entity itself would be useful.

Chatted with Gábor Hojtsy and Alex Pott, we need to add dependency calculation (depend on the bundle) and unit testing the config entity methods (including this dependencies calculation).

Added calculateDependencies(). Added unit tests.

Silly me, wrong namespace.

@penyaskito asked me to look at the fails in Drupal\taxonomy\Tests\VocabularyLanguageTest. This issue was that the sumbit handler was firing before the vocabulary had actually been saved!

Patch contains a couple of other clean ups.

I also think we should implement hook_entity_bundle_rename and clean up code like VocabularyForm::languageConfigurationSubmit()

Drupal\language\Tests\LanguageConfigurationElementTest is failing because it is using non existing entity types and bundles.

Oops forgot to rebase.

#55 had language_show => language_alterable changed in LanguageConfigurationElementTest also, but not the recent patches. That contributes to most fails there. Also the lack of an entity type named some_entity :D We can use the entity_test type very well for this.

That leaves only one fail where the node type is renamed and the language config is not renamed accordingly. Expecting to only see that fail now.

#68 automatic 3-way merge reroll.

I'm trying to figure out what's wrong with this test.

This patch actually makes the test pass but I am not sure it is the right way to do even if it seems to be a good idea to check the return value in case of anything went wrong instead of letting the method throw a fatal error.

So, while trying to figure out what was going wrong here, I found a strange behavior.
The thing is that, in \Drupal\language\Tests\LanguageConfigurationElementTest::testNodeTypeUpdate() at line 138, while trying to change the NodeType 'type' property, the \Drupal\language\Entity\ContentLanguageSettings::calculateDependencies() method is called twice. The first time it is called on the renamed NodeType (article_2) and the second time on the original NodeType (article) that doesn't exist anymore! Taht particular problem could be related to another issue but I'll let you decide.

Sorry @DuaelFr, I didn't expect that someone would work on this issue and we had already discussed the test failure on IRC last week.
My interdiff is against #70 because I saw that too late.

We are calling twice because we implement the hook_ENTITY_TYPE_update hook for handling the changes in the node type, but later the language_configuration_element_submit handler is fired.

Anyway, your patch fix the symptoms, not the cause. I added a new assert in the test that ensures that the config entity is changed, not recreated. If you apply those lines to your patch, you will see a failure then.

As in hook_ENTITY_TYPE_update we don't have any way to flag the form or anything, I'm checking in language_configuration_element_submit if we are in the node type edit form, and in that case we take the entity type bundle from the form. Not clean, but it works (TM). Cleaner ideas are welcome.

PS: I'm not sure if instead of hook_ENTITY_TYPE_update I should use hook_entity_bundle_rename, I don't know if there is a benefit.

@DuaelFr Thanks for looking into this. Sorry there were offline conversations not documented on the issue. Since you have looked into this, maybe you can take a stab at updating the issue summary to reflect the current information and resolution direction? That would be a big help. https://www.drupal.org/contributor-tasks/write-issue-summary

1. +++ b/core/modules/language/language.module
   @@ -180,7 +180,18 @@ function language_configuration_element_submit(&$form, FormStateInterface $form_
   +      // In case we are editing the node, we must check the new entity type,
   +      // because language_node_type_update fired before.
   +      if ($form['#id'] == 'node-type-edit-form') {
   +        $bundle = $form_state->getValue('type');
   +      }
   

   Erm node specific code in a generic function for all entity types? What about other entity types?

2. +++ b/core/modules/language/language.module
   @@ -259,9 +270,10 @@ function language_clear_default_configuration($entity_type, $bundle) {
    function language_node_type_update(NodeTypeInterface $type) {
   -  if ($type->original->id() != $type->id()) {
   -    language_save_default_configuration('node', $type->id(), language_get_default_configuration('node', $type->original->id()));
   -    language_clear_default_configuration('node', $type->original->id());
   +  if ($type->getOriginalId() != $type->id()) {
   +    $config = ContentLanguageSettings::loadByEntityTypeBundle('node', $type->getOriginalId());
   +    $config->setTargetBundle($type->id());
   +    $config->save();
      }
    }
   

   Is this code we only need for node types, not for eg. vocabularies or contact categories or block types?

#77.1. How can we detect that we are in an edit form? How can we detect the bundle field name (type for nodes, but for others?)

#77.2. Probably you are right. Same question may apply.

So as my snippet showed the node_type specific rename handling is preexisting, and we already do a lot in this patch to demand that all entity bundle renames be handled IMHO. If it otherwise looks good I would not hold it up on this. The newly added code that I spotted in the generic function though for node types only is still concerning. Can we get around needing that?

Looks like one way to detect if the form is a content entity edit form (in most cases, probably in all but needs to be verified) is checking for $form['actions']['delete'].

Found better ways of checking if we are in a content entity bundle edit form, silly me.

I'm not sure yet about #77.2. We have LanguageConfigurationElementTest::testNodeUpdate(), we probably need a similar test for other entity type (Vocabulary?) and this will show if we need it or not.

+++ b/core/modules/language/language.module
@@ -182,10 +184,18 @@ function language_configuration_element_submit(&$form, FormStateInterface $form_
+      // A way of checking if the form is an edit form is checking for the
+      // delete action presence.

Forgot to change this comment and does not make sense anymore, yay!

Added the test for vocabulary, and then realized that #77.2 made sense, so implemented hook_entity_bundle_rename and then got forced to do what @alexpott suggested at #63 for making tests pass (I didn't get it then, now I do).

I really would prefer to check for the form $operation (as I'm doing on this patch), but probably we need to use $entity->isNew() because not every entity is defining the 'edit' 'form' in the annotation (which IMHO is bad, but should not be fixed here).

Using ::isNew() and reverting Vocabulary entity annotation and routing changes. Let's do that in a different issue. Should be green.

The recent changes look great as a generic solution for bundle renames. Superb. I cannot vouch for the rest of the patch so hope @plach has some time to re-review and RTBC if it looks good for him too.

Great work, almost good to go IMO :)

1. +++ b/core/modules/language/language.module
   @@ -178,7 +182,24 @@ function language_configuration_element_submit(&$form, FormStateInterface $form_
   +      $entityType = $values['entity_type'];
   

   This should be $entity_type_id...

2. +++ b/core/modules/language/language.module
   @@ -178,7 +182,24 @@ function language_configuration_element_submit(&$form, FormStateInterface $form_
   +      if ($form_object instanceof EntityForm && !$form_object->getEntity()->isNew()) {
   

   Wouldn't it be more reliable if we checked for ($operation == 'edit' || $operation == 'default')? Theoretically this test would pass also with delete entity forms.

3. +++ b/core/modules/language/language.module
   @@ -178,7 +182,24 @@ function language_configuration_element_submit(&$form, FormStateInterface $form_
   +        $entity_definition = Drupal::entityManager()->getDefinition($entityType);
   

   This should be $entity_type. Also \Drupal :)

4. +++ b/core/modules/language/language.module
   @@ -178,7 +182,24 @@ function language_configuration_element_submit(&$form, FormStateInterface $form_
   +        if ($entity_definition instanceof ContentEntityType) {
   

   I think here we could do

   if ($entity_type->getBundleOf()) {
   

   which should be more reliable.

5. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,171 @@
   +    if ($bundle_entity_type_id != 'bundle') {
   

   I don't get what this check means, maybe we need to add some more comments? Would the ::getBundleOf() approach mentioned above help here?

Doesn't #87.3 imply untested code? Whatever that is doing - we need tests for that.

Nope, it is in a module, so the \ is just coding standard.

Here's an additional quick review.

1. +++ b/core/modules/language/src/ContentLanguageSettingsInterface.php
   @@ -0,0 +1,96 @@
   + * Provides an interface defining content language settings entity.
   

   Something's wrong/missing here.

2. +++ b/core/modules/language/src/ContentLanguageSettingsInterface.php
   @@ -0,0 +1,96 @@
   +   * @param string $target_bundle
   +   *
   ...
   +   * @param string $default_langcode
   +   *
   ...
   +   * @param bool $language_alterable
   +   *
   

   Incomplete docblocks.

3. +++ b/core/modules/language/src/Tests/LanguageConfigurationElementTest.php
   @@ -138,6 +140,41 @@ public function testNodeTypeUpdate() {
   +    // Check the language default configuration .
   

   s/ ././

4. +++ b/core/modules/taxonomy/src/VocabularyForm.php
   @@ -93,12 +93,6 @@ protected function actions(array $form, FormStateInterface $form_state) {
   -      // Add the language configuration submit handler. This is needed because
   -      // the submit button has custom submit handlers.
   -      if ($this->moduleHandler->moduleExists('language')) {
   -        array_unshift($actions['submit']['#submit'], 'language_configuration_element_submit');
   -        array_unshift($actions['submit']['#submit'], '::languageConfigurationSubmit');
   -      }
   

   Awesome! :)

#87.1: Fixed.
#87.2: But "default" would also pass with add forms. In #83 I tried to detect the "edit" operation, but this is inconsistent in all entities. I would consider this a bug in entity definitions, but out of scope. Am I getting you right?
#87.3: Fixed.
#87.4: Fixed.
#87.5: It is the same pattern than in EntityDisplayBase, FieldConfigBase, RdfMapping... I think you are right, but could we do this in a follow-up and change all of them?

#90.1: Modified, but still don't sure. Suggestions welcome.
#90.2: Fixed.
#90.3: Fixed.

Thanks all for the reviews!

+++ b/core/modules/language/language.module
@@ -190,13 +190,13 @@ function language_configuration_element_submit(&$form, FormStateInterface $form_
-        $entity_definition = Drupal::entityManager()->getDefinition($entityType);
-        if ($entity_definition instanceof ContentEntityType) {
+        $entity_type = \Drupal::entityManager()->getDefinition($entity_type_id);
+        if ($entity_type->getBundleOf()) {

Failures must be caused by this hunk, the rest is functionally equivalent to the previous (green) patch.

But "default" would also pass with add forms. In #83 I tried to detect the "edit" operation, but this is inconsistent in all entities. I would consider this a bug in entity definitions, but out of scope. Am I getting you right?

You are right, we should combine both checks then :)

It is the same pattern than in EntityDisplayBase, FieldConfigBase, RdfMapping... I think you are right, but could we do this in a follow-up and change all of them?

Sure

Not sure what to think about failures, can we try and see whether we can fix them? I really think the previous approach was fragile...

Combined both checks as plach suggested.

The failure came because I was using the entity_type of the bundle (e.g. taxonomy_term) instead of the entity_type of the entity itself (e.g. taxonomy_vocabulary). Probably I'm wrong in terms of terminology, but you will see it clear at the interdiff.

It should be green again.

Awesome, thanks!

Forgive me, we need to ensure that we have an EntityForm before calling getOperation().

Uh, I thought I saw even the previous one green :D

Reviewing... but immediately obvious that we're going to need a change record.

1. +++ b/core/modules/language/language.module
   @@ -178,7 +182,23 @@ function language_configuration_element_submit(&$form, FormStateInterface $form_
   +      if ($form_object instanceof EntityForm && !$form_object->getEntity()->isNew() && ($form_object->getOperation() === 'default' || $form_object->getOperation() === 'edit')) {
   

   Is there are reason we don't check for EntityFormInterface but rather EntityForm?

   Maybe think about using in_array(..., ..., TRUE) so you don't need the two getOperation() calls which feels verbose to read.

2. +++ b/core/modules/language/language.module
   @@ -178,7 +182,23 @@ function language_configuration_element_submit(&$form, FormStateInterface $form_
   +      /** @var ContentLanguageSettings $config */
   +      $config = ContentLanguageSettings::loadByEntityTypeBundle($entity_type_id, $bundle);
   

   I don't see the point of the inline documentation ... loadByEntityTypeBundle already documents what it returns, just mentioning here. ... Ah you maybe should add /** @var $this $config

3. +++ b/core/modules/language/src/ContentLanguageSettingsInterface.php
   @@ -0,0 +1,99 @@
   +   * @return static
   

   We do use @return $this now

4. +++ b/core/modules/language/src/ContentLanguageSettingsInterface.php
   @@ -0,0 +1,99 @@
   +   *   The content language config entity if one exists. Otherwise, returns
   +   *   default values.
   +   */
   +  public static function loadByEntityTypeBundle($entity_type_id, $bundle);
   

   Do you think there is a big value in making that part of the interface? Would you ever want to swap that out?

5. +++ b/core/modules/locale/src/Tests/LocaleContentTest.php
   @@ -84,7 +84,7 @@ public function testContentTypeLanguageConfiguration() {
   -      'language_configuration[language_show]' => TRUE,
   +      'language_configuration[language_alterable]' => TRUE,
   

   It would be great to describe in the issue summary why we need this rename that

1. +++ b/core/modules/language/language.module
   @@ -193,10 +213,21 @@ function language_configuration_element_submit(&$form, FormStateInterface $form_
   + * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 8.0.0.
   + *   Create and save a \Drupal\language\Entity\ContentLanguageSettings config
   + *   entity instead.
     */
    function language_save_default_configuration($entity_type, $bundle, $values = array()) {
   
   @@ -210,15 +241,16 @@ function language_save_default_configuration($entity_type, $bundle, $values = ar
   + * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 8.0.0.
   + *   Call
   + *   \Drupal\language\Entity\ContentLanguageSettings::loadByEntityTypeBundle
   + *   instead.
     */
    function language_get_default_configuration($entity_type, $bundle) {
   
   @@ -228,40 +260,26 @@ function language_get_default_configuration($entity_type, $bundle) {
   + * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 8.0.0.
   + *   Call
   + *   \Drupal\language\Entity\ContentLanguageSettings::loadByEntityTypeBundle
   + *   and if the returned config entity is new, delete it.
     */
   ...
   +function language_clear_default_configuration($entity_type, $bundle) {
   

   Are we sure we want to remove these functions by 8.0.0? We've been trying to avoid deprecating new functions.

2. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,171 @@
   +      $config = ContentLanguageSettings::create(['target_entity_type_id' => $entity_type_id, 'target_bundle' => $bundle]);
   

   I think ContentLanguageSettings should have a constructor along the lines of FieldConfig to ensure that the target_entity_type_id and target_bundle is set since the id() function assumes they exist so everything will be very broken if they don't.

3. +++ b/core/modules/language/src/Entity/ContentLanguageSettings.php
   @@ -0,0 +1,171 @@
   +      $bundle_entity = $this->entityManager()->getStorage($bundle_entity_type_id)->load($this->target_bundle);
   +      $this->addDependency('config', $bundle_entity->getConfigDependencyName());
   

   We should probably throw an exception here if the bundle does not exist - or check the bundle in the new constructor. See #2374339: FieldConfigBase::calculateDependencies() fatal error is unhelpful

4. +++ b/core/modules/language/src/Tests/LanguageConfigurationElementTest.php
   @@ -138,6 +140,41 @@ public function testNodeTypeUpdate() {
   +  /**
   +   * Tests that the configuration is updated when a vocabulary is changed.
   +   */
   +  public function testTaxonomyVocabularyUpdate() {
   

   Do we have an equivalent test for NodeType renaming?

5. +++ b/core/modules/language/language.module
   @@ -5,14 +5,18 @@
   +use Drupal\Core\Entity\ContentEntityType;
   +use Drupal\Core\Entity\EntityInterface;
   

   Not used.

@penyaskito created a good start for a change notice. I edited it for language and added more before/after examples. I think that looks good. https://www.drupal.org/node/2382481

Could be worth checking if there is anything in the issue summary that needs updating.
I updated some tasks that were clearly done,
and added the beta evaluation template.
Could use someone to make sure the rationale for critical bug is accurate.

Handled #101 and #102.

#101.5: We do still need to document the key change.
#102.1: What should I do? Remove the deprecation? Remove it already? Using LanguageContentSettings everywhere would be quite a big patch if we include that in this issue.
#102.4: Yes we do, testNodeTypeUpdate changes the content type name.

re #105/#102.1 I think we should deprecate for 9.x.

OK, changed that to * @deprecated in Drupal 8.0.x-dev, will be removed in Drupal 9.0.0.

If we want to keep the old functions around then we need a BC layer for the language_show/language_alterable, otherwise it's pretty useless as you would have to change it anyway :)

Discussed #110 with @alexpott, let's remove them in this issue.

I'm okay with removing all these helpers on this issue - yes it will increase patch size but hiding a config entity behind helper functions is unnecessarily convoluted and as @Berdir points out they are all going to have to change anyway. Let's get the break done.

Is #2363155: content_translation.settings config is not scalable postponed on this?

Yes

thanks.

adding blocker tag, since this blocks #2363155: content_translation.settings config is not scalable, so that the d8 blockers is accurate. Remove the blocker tag when this issue is fixed, and unpostpone 2363155.

Work is happening on #2360245: #2355909 helper for trying to keep this one as clean as possible.

Partial work (#11 to #26 on the helper issue):

Removed language_get_default_configuration(), using ContentLanguageSettings instead. Content entity forms had checks for displaying or not the language widget, this has been moved to the language module hook_form_alter implementation, removing duplicated code.

Removed language_save_default_configuration and language_clear_default_configuration. Clean-up of language_entity_bundle_rename.

I am wondering if we should move language_get_default_langcode($entity_type, $bundle) to a new LanguageManager::getContentDefaultLangcode($entity_type, $bundle) but we have to stop somewhere.

Otherwise, this needs review.

Reviews #117 first.

+++ b/core/modules/comment/src/CommentForm.php
@@ -161,13 +162,12 @@ public function form(array $form, FormStateInterface $form_state) {
-    $language_configuration = \Drupal::moduleHandler()->invoke('language', 'get_default_configuration', array('comment', $comment->getTypeId()));
     $form['langcode'] = array(
       '#title' => t('Language'),
       '#type' => 'language_select',
       '#default_value' => $comment->getUntranslated()->language()->getId(),
       '#languages' => Language::STATE_ALL,
-      '#access' => isset($language_configuration['language_show']) && $language_configuration['language_show'],
+      '#access' => FALSE,
     );

Without your comment on the alter being added this looked pretty strange. We had a pattern copied around. Now we have a simpler pattern (yay) but its harder to understand. Can we add a comment right above the access FALSE that says something like "Language module may expose this element, see language_form_alter()."? I think that would make the pattern much easier to understand.

#118 looks good. So other than my comment in #119, I think the changes look great.

Improved comments then.

Changes look great. Prior concerns are resolved nicely IMHO.

+++ b/core/modules/language/language.module
@@ -178,90 +181,33 @@ function language_configuration_element_submit(&$form, FormStateInterface $form_
+      // In case we are editing a bundle, we must check the new bundle name,

Comment confuses me a bit. If we're in the submit handler, why would hook_entity_update() have already fired? Looks like it's trying to account for the case both where it has and it hasn't?

I've been following along and the general approach looks fine to me.

Is it worth a major core issue to change the form? That feels like something we could do in 8.1.x.

@catch: hook_entity_update() has fired before language_configuration_element_submit, so the entity bundle machine name already changed and we must take the new one from the form state instead that from the entity itself. If not, we would be creating a new object, and that would make dependency calculation fail because the entity type is already missing.

How can we improve the wording? Maybe the problem is that this comment should be some lines above?

About changing the form, what I understood is that having everything together is so loved by users that we are not going to make it in core, but can happen in contrib for the 20% of high complex sites who may need it, so I didn't even thought of creating an issue yet.

@penyaskito moving the comment up might be enough.

Moved the comment.

OK gave it another once over and I think this is good. We have the dependent issue this unblocks to revisit anything that comes up there anyway.

Committed/pushed to 8.0.x, thanks!

Published the change record.

Thanks for the heroic efforts here! @penyaskito++