Problem/Motivation

language.settings contains the settings for every translateable bundle in the system. While there's still some debate on how well the entity system scales regarding bundles, the #2075941-60: Port Webform to Drupal 8 official (as in, subsystem maintainer) word is "not so bad".

To the contrary, core/modules/language/src/Form/ContentLanguageSettingsForm.php puts every translateable bundle in a single form. 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. The config object, accordingly, will be huge and unnecessarily it'll be loaded all the time when only a tiny bit of it is needed.

Also, this is a UX problem: node type settings should be on the node form.

Blocking #2363155: content_translation.settings config is not scalable

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug, performance bug.
Issue priority Critical because will effect the entire system for any site more than a simple one.

Allowed in d8 beta because it is a critical bug.

Proposed resolution

Keep the form, when the time comes someone likely will write a contrib form scaling better. But to support it we must split the config object.

The Config object is moved into a specific config entity type which stores the language option on a entity type and bundle base.

Remaining tasks

 

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions Done in #74
Update the issue summary Instructions
Update the issue summary noting if allowed during the beta Instructions done
Add automated tests (added testTaxonomyVocabularyUpdate and ContentLanguageSettingsUnitTest) Instructions done.
Draft a change record for the API changes Instructions done
Manually test the patch Novice Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

None

API changes

Todo

CommentFileSizeAuthor
#126 2355909-language-content-settings-126.patch92.03 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2355909-language-content-settings-126.patch. Unable to apply patch. See the log in the details link for more information. View
#126 2355909-language-content-setting.interdiff.121-126.txt1.16 KBpenyaskito
#121 2355909-language-content-setting.interdiff.118-121.txt5.03 KBpenyaskito
#121 2355909-language-content-settings-121.patch91.91 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,186 pass(es). View
#118 2355909-language-content-setting.interdiff.117-118.txt9.67 KBpenyaskito
#118 2355909-language-content-settings-118.patch90.58 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,187 pass(es). View
#117 2355909-language-content-setting-117.patch90.18 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,175 pass(es). View
#117 2355909-language-content-setting.interdiff.109-117.txt33.56 KBpenyaskito
#109 2355909-language-content-settings-109.patch77.01 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,860 pass(es). View
#109 2355909-language-content-setting.interdiff.107-109.txt1.55 KBpenyaskito
#107 2355909-language-content-settings-107.patch77.02 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,987 pass(es). View
#107 2355909-language-content-setting.interdiff.105-107.txt1.66 KBpenyaskito
#105 2355909-language-content-settings-105.patch76.8 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,960 pass(es), 46 fail(s), and 0 exception(s). View
#105 2355909-language-content-settings.interdiff.98-105.txt6.49 KBpenyaskito
#98 2355909-language-content-settings-98.patch75.12 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,781 pass(es). View
#98 2355909-language-content-settings.interdiff.95-98.txt1015 bytespenyaskito
#95 2355909-language-content-settings-95.patch75.13 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,772 pass(es), 8 fail(s), and 0 exception(s). View
#95 2355909-language-content-settings.interdiff.91-95.txt1.08 KBpenyaskito
#91 2355909-language-content-settings-91.patch75.09 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,757 pass(es), 8 fail(s), and 2 exception(s). View
#91 2355909-language-content-settings.interdiff.85-91.txt3.95 KBpenyaskito
#85 2355909-language-content-settings-85.patch74.99 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,483 pass(es). View
#85 2355909-language-content-settings.interdiff.83-85.txt2.05 KBpenyaskito
#83 2355909-language-content-settings-83.patch76.22 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,472 pass(es), 6 fail(s), and 2 exception(s). View
#83 2355909-language-content-settings.interdiff.81-83.txt8.34 KBpenyaskito
#81 2355909-language-content-settings-81.patch71.58 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,445 pass(es). View
#81 2355909-language-content-settings.interdiff.74-81.txt1.75 KBpenyaskito
#74 2355909-language-content-settings-74.patch71.07 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,204 pass(es). View
#74 2355909-language-content-settings.interdiff.70-74.txt5.25 KBpenyaskito
#73 interdiff.2355909.70.73.txt1.15 KBDuaelFr
#73 2355909-language-content-settings-73.patch68.78 KBDuaelFr
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,049 pass(es). View
#70 2355909-language-content-settings-70.patch68.67 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,049 pass(es), 1 fail(s), and 0 exception(s). View
#68 interdiff.txt9.13 KBGábor Hojtsy
#68 2355909.68.patch68.67 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,576 pass(es), 1 fail(s), and 0 exception(s). View
#65 2355909.65.patch65.83 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,672 pass(es), 9 fail(s), and 1 exception(s). View
#63 2355909.63.patch69.86 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,554 pass(es), 9 fail(s), and 2 exception(s). View
#63 61-63-interdiff.txt2.55 KBalexpott
#61 2355909-language-content-settings-61.patch64.5 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,592 pass(es), 36 fail(s), and 1 exception(s). View
#61 2355909-language-content-settings.interdiff.59-61.txt623 bytespenyaskito
#59 2355909-language-content-settings-59.patch64.5 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#59 2355909-language-content-settings.interdiff.55-59.txt12.32 KBpenyaskito
#55 2355909-language-content-settings-55.patch52.59 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,472 pass(es). View
#55 2355909-language-content-settings.interdiff.53-55.txt578 bytespenyaskito
#53 2355909-language-content-settings-53.patch52.54 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,440 pass(es), 684 fail(s), and 167 exception(s). View
#53 2355909-language-content-settings.interdiff.51-53.txt8.33 KBpenyaskito
#52 2355909-language-content-settings-51.patch50.45 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,473 pass(es). View
#52 2355909-language-content-settings.interdiff.48-51.txt42.98 KBpenyaskito
#48 2355909-language-content-settings.interdiff.45-48.txt2.57 KBpenyaskito
#48 2355909-language-content-settings-48.patch50.3 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,474 pass(es). View
#45 2355909-language-content-settings-45.patch50.31 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,652 pass(es). View
#45 2355909-language-content-settings.interdiff.44-45.txt2.35 KBpenyaskito
#45 2355909-language-content-settings.interdiff.43-44.txt1.36 KBpenyaskito
#43 2355909-language-content-settings-43.patch50.05 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,474 pass(es). View
#43 2355909-language-content-settings.interdiff.42-43.txt2.81 KBpenyaskito
#42 2355909-language-content-settings.interdiff-42.patch49.63 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,471 pass(es). View
#42 2355909-language-content-settings.interdiff.40-42.txt42.21 KBpenyaskito
#40 2355909-language-content-settings-40.patch13.3 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,474 pass(es). View
#34 2355909-language-content-settings-34.patch13.3 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,554 pass(es). View
#34 2355909-language-content-settings.interdiff.32-34.txt762 bytespenyaskito
#32 2355909-language-content-settings-32.patch13.55 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,553 pass(es). View
#32 2355909-language-content-settings.interdiff.30-32.txt1.01 KBpenyaskito
#30 2355909-language-content-settings-30.patch13.56 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,541 pass(es), 10 fail(s), and 0 exception(s). View
#30 2355909-language-content-settings.interdiff.28-30.txt10.13 KBpenyaskito
#28 2355909-language-content-settings-28.patch12.19 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,494 pass(es). View
#20 2355909-language-content-settings.interdiff.17-19.txt3.03 KBpenyaskito
#20 2355909-language-content-settings-19.patch8.42 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,504 pass(es). View
#17 2355909-language-content-settings-17.patch5.82 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,976 pass(es), 82 fail(s), and 1 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Fabianx’s picture

penyaskito’s picture

Issue tags: +D8MI, +sprint, +language-config
Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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?

Gábor Hojtsy’s picture

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

YesCT’s picture

chx’s picture

Issue summary: View changes

Discussed with webchick and dawehner. New proposal: keep the form , we can replace it in contrib but most definitely split the config object.

Gábor Hojtsy’s picture

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.

dawehner’s picture

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?

Gábor Hojtsy’s picture

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

tstoeckler’s picture

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.

alexpott’s picture

Tagging

alexpott’s picture

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

alexpott’s picture

Priority: Major » Critical
Issue tags: +API change, +D8 upgrade path

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

penyaskito’s picture

I'm working on this one.

Berdir’s picture

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

penyaskito’s picture

FileSize
5.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,976 pass(es), 82 fail(s), and 1 exception(s). View

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.

penyaskito’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: 2355909-language-content-settings-17.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
8.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,504 pass(es). View
3.03 KB

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.

Berdir’s picture

Status: Needs review » Needs work
+++ 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.

Gábor Hojtsy’s picture

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

Berdir’s picture

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

Gábor Hojtsy’s picture

@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

Berdir’s picture

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.

penyaskito’s picture

I misunderstood, that will make migrate easier too.

yched’s picture

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

penyaskito’s picture

Status: Needs work » Needs review
FileSize
12.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,494 pass(es). View

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.

Berdir’s picture

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?

penyaskito’s picture

FileSize
10.13 KB
13.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,541 pass(es), 10 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 30: 2355909-language-content-settings-30.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
13.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,553 pass(es). View

Oops

dawehner’s picture

Issue summary: View changes
  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?

penyaskito’s picture

FileSize
762 bytes
13.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,554 pass(es). View

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?
chx’s picture

> Still needs thirdpartysettings. Afaik is just adding the trait and schema change.

Not sure whether it matters but #2361775: Third party settings dependencies cause config entity deletion is adding it to the config entity base.

chx’s picture

> Still needs thirdpartysettings. Afaik is just adding the trait and schema change.

Not sure whether it matters but #2361775: Third party settings dependencies cause config entity deletion is adding it to the config entity base.

Berdir’s picture

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.

yched’s picture

FYI: opened #2362317: No namespacing of "system" properties in ConfigEntities / yaml files means lockdown after 8.0 is out 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...

Gábor Hojtsy’s picture

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

penyaskito’s picture

penyaskito’s picture

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.

penyaskito’s picture

FileSize
42.21 KB
49.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,471 pass(es). View

Renamed language_show to language_exposed everywhere...

penyaskito’s picture

FileSize
2.81 KB
50.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,474 pass(es). View

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

effulgentsia’s picture

  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.

penyaskito’s picture

FileSize
1.36 KB
2.35 KB
50.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,652 pass(es). View

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.

penyaskito’s picture

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.

plach’s picture

Status: Needs review » Needs work

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()?

penyaskito’s picture

Status: Needs work » Needs review
FileSize
50.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,474 pass(es). View
2.57 KB

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

Berdir’s picture

  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.

plach’s picture

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

penyaskito’s picture

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

penyaskito’s picture

FileSize
42.98 KB
50.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,473 pass(es). View

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

penyaskito’s picture

FileSize
8.33 KB
52.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,440 pass(es), 684 fail(s), and 167 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 53: 2355909-language-content-settings-53.patch, failed testing.

penyaskito’s picture

FileSize
578 bytes
52.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,472 pass(es). View

Oops, forgot the use.

penyaskito’s picture

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

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.

penyaskito’s picture

Status: Needs review » Needs work

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

penyaskito’s picture

Status: Needs work » Needs review
FileSize
12.32 KB
64.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Added calculateDependencies(). Added unit tests.

Status: Needs review » Needs work

The last submitted patch, 59: 2355909-language-content-settings-59.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
623 bytes
64.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,592 pass(es), 36 fail(s), and 1 exception(s). View

Silly me, wrong namespace.

Status: Needs review » Needs work

The last submitted patch, 61: 2355909-language-content-settings-61.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
69.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,554 pass(es), 9 fail(s), and 2 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 63: 2355909.63.patch, failed testing.

alexpott’s picture

FileSize
65.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,672 pass(es), 9 fail(s), and 1 exception(s). View

Oops forgot to rebase.

alexpott’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 65: 2355909.65.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
68.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,576 pass(es), 1 fail(s), and 0 exception(s). View
9.13 KB

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

Status: Needs review » Needs work

The last submitted patch, 68: 2355909.68.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
68.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,049 pass(es), 1 fail(s), and 0 exception(s). View

#68 automatic 3-way merge reroll.

Status: Needs review » Needs work

The last submitted patch, 70: 2355909-language-content-settings-70.patch, failed testing.

DuaelFr’s picture

Assigned: Unassigned » DuaelFr

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

DuaelFr’s picture

Assigned: DuaelFr » Unassigned
Status: Needs work » Needs review
FileSize
68.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,049 pass(es). View
1.15 KB

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.

penyaskito’s picture

FileSize
5.25 KB
71.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,204 pass(es). View

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.

YesCT’s picture

@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

DuaelFr’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

  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?

penyaskito’s picture

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

Gábor Hojtsy’s picture

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?

penyaskito’s picture

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

penyaskito’s picture

Issue summary: View changes
FileSize
1.75 KB
71.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,445 pass(es). View

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.

penyaskito’s picture

Status: Needs review » Needs work
+++ 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!

penyaskito’s picture

Status: Needs work » Needs review
FileSize
8.34 KB
76.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,472 pass(es), 6 fail(s), and 2 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 83: 2355909-language-content-settings-83.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
2.05 KB
74.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,483 pass(es). View

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

Gábor Hojtsy’s picture

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.

plach’s picture

Status: Needs review » Needs work

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?

alexpott’s picture

Issue tags: +Needs tests

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

Berdir’s picture

Issue tags: -Needs tests

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

Wim Leers’s picture

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! :)

penyaskito’s picture

Status: Needs work » Needs review
FileSize
3.95 KB
75.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,757 pass(es), 8 fail(s), and 2 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 91: 2355909-language-content-settings-91.patch, failed testing.

Wim Leers’s picture

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

plach’s picture

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

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
75.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,772 pass(es), 8 fail(s), and 0 exception(s). View

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.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 95: 2355909-language-content-settings-95.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1015 bytes
75.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,781 pass(es). View

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

plach’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

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

dawehner’s picture

  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

alexpott’s picture

  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.

Gábor Hojtsy’s picture

Issue tags: -Needs change record

@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

YesCT’s picture

Issue summary: View changes

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.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
6.49 KB
76.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,960 pass(es), 46 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 105: 2355909-language-content-settings-105.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1.66 KB
77.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,987 pass(es). View
alexpott’s picture

Status: Needs review » Needs work

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

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
77.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,860 pass(es). View

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

Berdir’s picture

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

penyaskito’s picture

Status: Needs review » Needs work

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

alexpott’s picture

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.

YesCT’s picture

plach’s picture

Yes

YesCT’s picture

Issue summary: View changes
Issue tags: +blocker

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.

penyaskito’s picture

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

penyaskito’s picture

Status: Needs work » Needs review
FileSize
33.56 KB
90.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,175 pass(es). View

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.

penyaskito’s picture

FileSize
90.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,187 pass(es). View
9.67 KB

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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

Gábor Hojtsy’s picture

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

penyaskito’s picture

Status: Needs work » Needs review
FileSize
91.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,186 pass(es). View
5.03 KB

Improved comments then.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Changes look great. Prior concerns are resolved nicely IMHO.

catch’s picture

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

penyaskito’s picture

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

catch’s picture

@penyaskito moving the comment up might be enough.

penyaskito’s picture

FileSize
1.16 KB
92.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2355909-language-content-settings-126.patch. Unable to apply patch. See the log in the details link for more information. View

Moved the comment.

catch’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • catch committed 30a5046 on 8.0.x
    Issue #2355909 by penyaskito, alexpott, Gábor Hojtsy, DuaelFr: language....
penyaskito’s picture

Published the change record.

Status: Fixed » Needs work

The last submitted patch, 126: 2355909-language-content-settings-126.patch, failed testing.

penyaskito’s picture

Status: Needs work » Fixed
Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks for the heroic efforts here! @penyaskito++

Status: Fixed » Closed (fixed)

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