Config entity admin lists show configuration with overrides (eg. localized)

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.

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.

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.

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

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

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

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.

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.

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

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

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.

See #2393751: Document that KeyValueEntityStorage is not scalable beyond a few hundred entities for a bit more context about why I need arguments ;)

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

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.

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!

There is a discussion happening on #2872646: Menu overview always shows the untranslated config entities about this.

Having made this choice, it's really bad-looking if your site has only 1 language that is not English... Let's discuss on that other issue, which has marked this one as Related (since it's the cause of the problem).

#3281219: Language used for config listing should be configurable