Problem/Motivation

Admin listings of configuration entities load and display the configuration entity with overrides applied (eg. using the language negotiated for the page). This looks flawless on the page itself. Eg. if you view a Hungarian admin page for content types, you get to see the Hungarian name of content types and their Hungarian descriptions. The problem starts when you actually want to edit it. Well, once you hit the edit screen, you'll ("obviously") edit the original configuration entity that is for shipped content types the English configuration. This is true for all kinds of overrides, eg. group based, time based, global settings based, etc.

This is not that bad maybe for content types, but for date formats for example, it can be pretty confusing. Eg. you see a preview of a date format (in Hungarian), then you go edit it, and it is not the date format you've seen in the summary table.

Proposed resolution

Configuration entity admin lists should load / show configuration in their original override-free form.

Remaining tasks

Do it.
Review.
Commit.

User interface changes

Configuration entities will show up in their original form in the admin listings (ie. same language that you get when you go edit them).

API changes

Probably none.

Files: 
CommentFileSizeAuthor
#32 interdiff.txt3.81 KBGábor Hojtsy
#32 2136559-load-original-entity-31.patch9.24 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,890 pass(es). View
#27 interdiff.txt3.74 KBGábor Hojtsy
#27 2136559-load-original-entity-27.patch10.43 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,839 pass(es). View
#23 2136559-load-original-entity-23.patch10.41 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,665 pass(es). View
#22 interdiff.txt858 bytesGábor Hojtsy
#22 2136559-load-original-entity-22.patch14.57 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,664 pass(es). View
#15 2136559-load-original-entity-15.patch10.4 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,542 pass(es). View
#15 interdiff.txt7.8 KBGábor Hojtsy
#13 interdiff.txt4.79 KBGábor Hojtsy
#13 2136559-load-original-entity-13.patch7.68 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,517 pass(es), 28 fail(s), and 9 exception(s). View
#8 2136559-load-original-entity.patch2.89 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,420 pass(es), 29 fail(s), and 9 exception(s). View
Tartalomtípusok | s7cf984bcda1c6f4.s2.simplytest.me 2013-11-15 14-18-58.png50.83 KBGábor Hojtsy

Comments

tim.plunkett’s picture

Status: Active » Closed (duplicate)
Related issues: +#2251915: Overridden config entity bleeds through to admin forms

Arghhh I looked for this issue before opening #2251915: Overridden config entity bleeds through to admin forms, but couldn't find it. Marking as a dupe.

tim.plunkett’s picture

Status: Closed (duplicate) » Active

After re-re-reading this issue, this is NOT covered by #2251915: Overridden config entity bleeds through to admin forms.
In fact, I'm not personally sure we should do this issue.

Reopening for discussion.

Gábor Hojtsy’s picture

@tim.plunkett: well, the problem is in your views list you see a view named "Honlap", you go edit int and "Honlap" is nowhere to be found because its the translation of the name, so you can only see "Frontpage". Is this not the same level of issue then (critical?).

catch’s picture

This feels major to me, there's no issue with writing the configuration entities back incorrectly, and it should be straightforward to fix after release.

pwolanin’s picture

So, seems this will be an issue with localized and translated menu links also.

Gábor Hojtsy’s picture

Title: Config entity admin lists show configuration localized » Config entity admin lists show configuration with overrides (eg. localized)
Issue summary: View changes
Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
2.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,420 pass(es), 29 fail(s), and 9 exception(s). View

So ConfigEntityListBuilder::load() is where the loading happens for the list. I *could* put in an overridestate get/set there but this uses the injected storage to load the values. The injected storage is a ConfigEntityStorage, which in turn has an injected config factory. So doing the overridestate get/set on that ConfigFactory is needed. So basically we need to introduce methods to load override-free config entities one by one or in bulk.

Here is a proof of concept patch that is untested for now.

Status: Needs review » Needs work

The last submitted patch, 8: 2136559-load-original-entity.patch, failed testing.

jhodgdon’s picture

Um. On another issue, we added the ability to, on a particular admin route, tell it to not translate config entities.

Here's an example from the help topic entity routing.yml file:

entity.help_topic.canonical:
  path: '/admin/help-topic/{help_topic}'
  defaults:
    _entity_view: 'help_topic.full'
    _title: 'Help'
  requirements:
    _entity_access: 'help_topic.view'
  options:
    parameters:
      help_topic:
        type: entity:help_topic
        # Force load in current interface language.
        use_current_language: true

Couldn't that be used here? What is being done looks very complicated...

jhodgdon’s picture

Oh never mind, it's the opposite. :(

Gábor Hojtsy’s picture

@jhodgdon: in this issue, the entity does not come from upcasting but an entity list controller loads it explicitly (in bulk even).

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,517 pass(es), 28 fail(s), and 9 exception(s). View
4.79 KB

Updating the entity listing / edit test first.

Status: Needs review » Needs work

The last submitted patch, 13: 2136559-load-original-entity-13.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.8 KB
10.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,542 pass(es). View

1. We don't need that test page to ensure the overrides applied. We can just use the API in the test. Should fix the entity edit page test.

2. The key-value storage test used an entity class for storing config entities that did not implement the config entity storage interface. Hah! So by implementing that and injecting the config factory we need it is fixed. I did not see a point implementing the methods in this storage which are not tested, it would be definitely out of scope for this issue to implement those and test them.

The previously failing tests are passing locally now. Let's see the whole suit.

Gábor Hojtsy’s picture

Issue tags: +sprint
Berdir’s picture

What about settings.php overrides?

If we never use the overrides in those admin lists, how does a user know that they are actually applied? And what if those values are actually used somehow?

Let's take search api servers as an example. You have an override locally to use a different solr server, and overview list somehow uses that value to do a connection check.

Gábor Hojtsy’s picture

@Berdir: That is what I opened "Figure out if we want global config overrides to stick (settings.php overrides don't work on all pages)" originally. Now retitled / rescoped to #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects. As a matter of fact, whatever is displayed in this overview, the actual editing screen will not edit overridden data (unless some modules changes the code behavior, which can equally change the listing behavior). So whatever we display in this list (eg. solr server), the edit form will not display / let you edit that. If you have an entity operation on the solr server config entity that does the connection check, it will be an admin path as well I guess, so that will get the entity upcast without overrides as well, so will do the connection check for the non-overridden server also (already, without this patch).

Gábor Hojtsy’s picture

@alexpott asked me to consult @tim.plunkett for this issue. Quick feedback I got so far is "I'm fine with introducing KeyValueConfigEntityStorage". Asked him to post his feedback here.

tim.plunkett’s picture

So by implementing that and injecting the config factory we need it is fixed

I think this is completely fine. +1 to the addition.

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
    @@ -420,4 +420,26 @@ public function updateFromStorageRecord(ConfigEntityInterface $entity, array $va
    +  public function loadOriginal($id) {
    ...
    +  public function loadMultipleOriginal(array $ids = NULL) {
    
    +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueConfigEntityStorage.php
    @@ -0,0 +1,100 @@
    +  public function loadOriginal($id) {
    ...
    +  public function loadMultipleOriginal(array $ids = NULL) {
    

    Is there any worth in making these a trait?

  2. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueConfigEntityStorage.php
    @@ -0,0 +1,100 @@
    +   * @inheritdoc
    ...
    +   * @inheritdoc
    ...
    +   * @inheritdoc
    

    missing the {}

Test changes look solid.

Gábor Hojtsy’s picture

@tim.plunkett: while I could move this to a trait, it would not work independently. The methods need the injected config factory that is in these classes already. We could add a setter for a specific config factory for these methods but that would be parallel to the already existing factory, no?

Gábor Hojtsy’s picture

FileSize
14.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,664 pass(es). View
858 bytes

Updating the code comments. @tim.plunkett what do you think?

Gábor Hojtsy’s picture

FileSize
10.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,665 pass(es). View

@tim.plunkett pointed out I had default.services.yml and default.settings.php hunks in my patch which were not intended. Just removed those.

Gábor Hojtsy’s picture

@berdir: I think your concerns are addressed with the use_current_language upcasting option as used in #2137595-26: 'Create @name' page title uses override-free configuration (eg. not localized) instead of the overridden configuration (eg. localized) for example. Note that the option is misleadingly named, it toggles all overrides. See #2405939: use_current_language upcasting option is misleading, it toggles all overrides not just language.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListBuilder.php
@@ -21,7 +21,7 @@ class ConfigEntityListBuilder extends EntityListBuilder {
-    $entities = parent::load();
+    $entities = $this->storage->loadMultipleOriginal();

The only reason this worries me is because of issues like #1798332: Add paging to the EntityListBuilder, that improve parent::load(), and this would miss out on that, and need to duplicate that code.

This is unrelated to the trait I requested before, and I don't really see a viable solution.

I'm going to RTBC because this is a good fix with good coverage, and we'll deal with it in the paging issue.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -420,4 +420,26 @@ public function updateFromStorageRecord(ConfigEntityInterface $entity, array $va
+  public function loadOriginal($id) {
...
+  public function loadMultipleOriginal(array $ids = NULL) {

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorageInterface.php
@@ -65,4 +65,27 @@ public function createFromStorageRecord(array $values);
+  public function loadOriginal($id);
...
+  public function loadMultipleOriginal(array $ids = NULL);

How about loadOverrideFree(), loadMultipleOverrideFree() - I think original might be confused with the original property on the entity and the getOriginal method on config objects.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
10.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,839 pass(es). View
3.74 KB

I am fine with those names too. Rolled a quick rename.

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorageInterface.php
    @@ -65,4 +65,27 @@ public function createFromStorageRecord(array $values);
    +   * @param mixed $id
    +   *   The ID of the entity to load.
    

    mixed was never true, and for config, we can limit it to string I think, but there's to argument to keep it consistent, so fine.

    @return could also say ConfigEntityInterface, but again.

  2. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueConfigEntityStorage.php
    @@ -0,0 +1,100 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function getIDFromConfigName($config_name, $config_prefix) { }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function createFromStorageRecord(array $values) { }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function updateFromStorageRecord(ConfigEntityInterface $entity, array $values) { }
    

    If we actually claim that this is a valid storage for config entities, shouldn't we implement this?

    Yay, more arguments for me that this should live in a test module :)

  3. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueConfigEntityStorage.php
    @@ -0,0 +1,100 @@
    +   * {@inheritdoc}
    +   */
    +  public function loadOverrideFree($id) {
    +    $old_state = $this->configFactory->getOverrideState();
    +    $this->configFactory->setOverrideState(FALSE);
    +    $entity = $this->load($id);
    +    $this->configFactory->setOverrideState($old_state);
    +    return $entity;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function loadMultipleOverrideFree(array $ids = NULL) {
    +    $old_state = $this->configFactory->getOverrideState();
    +    $this->configFactory->setOverrideState(FALSE);
    +    $entities = $this->loadMultiple($ids);
    +    $this->configFactory->setOverrideState($old_state);
    +    return $entities;
    +  }
    

    Uhm, this makes no sense :)

    Key value doesn't use config for storage, so messing with the config factory is pointless?

    All key value stored entities are override free, so you could just forward to load()/loadMultiple().

    Either way, even more arguments that we can *not* be serious about offering this as a storage for config.

Berdir’s picture

See #2393751: Move KeyValueEntityStorage to a test folder for a bit more context about why I need arguments ;)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@Berdir:

1. Re the docs of the new methods, I copied them from the docs on the current interface methods, because they are the same. Should be consistent no?

2. Re implementing those methods as well, they were not implemented before and not needed in this test either. They are not being tested, so if we are to implement them, we would need to test them. That I added them is merely technical here.

3. Indeed the addition of the config factory in this issue is/was arbitrary and we should just forward to load/loadMultiple for the scope of this patch. That the key/value storage cannot actually be used to store config because it cannot store overrides is an implementation detail, it could form its keys accordingly :) That is also a pre-existing condition and not in scope to fix. It is not made worse or better by this patch.

Needs work for 3 (removing the config factory and forwarding to load/loadmultiple).

Berdir’s picture

1. I know you did, but because we know that the ID is a config entity, we know that it has to be a string :) Anyway, as mentioned, fine with keeping it.

2. & 3. Well, the difference is, now you explicitly provide a key value entity storage *for config entities*, which looks like it is there to support config entities, when it in fact it does not ;) Again, I would have no problem at all with this if it were clear that it is a test implementation :)

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
9.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,890 pass(es). View
3.81 KB

Here is that update. I looked at KeyValueEntityStorage vs. ConfigEntityStorage, and indeed the way the latter implements "override support" is by relying on an injected config factory (the config factory maintains overrides not the config object). KeyValueEntityStorage is totally config factory agnostic and therefore by design could not support overrides. Oh well. Its clearly a test implementation. I agree #2393751: Move KeyValueEntityStorage to a test folder would be desirable.

Created a new issue for implementing the stubs at #2406645: Stub methods in KeyValueConfigEntityStorage are not implemented or tested and added @todos in them.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

That works for me. Setting back to RTBC, we can deal with that storage in those two issues.

@timplunkett in #25: Yes, I was thinking about the paging issue as well, but it doesn't really change anything other than we have to remember to update the subclass, we can still execute the entity query and then load those specific ID's. But it shows that we should move the query in a separate helper method, to make it easier to re-use here. And customize the query.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 3bbe561 and pushed to 8.0.x. Thanks!

  • alexpott committed 3bbe561 on 8.0.x
    Issue #2136559 by Gábor Hojtsy: Config entity admin lists show...
Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks!

Gábor Hojtsy’s picture

Found and opened #2407907: Configuration translation entity listings displays items overriden which applies to config translation admin lists for the same entities.

Status: Fixed » Closed (fixed)

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