Prevent saving config entities when configuration overrides are applied

@jrockowitz pointed out to this issue while discussing in #2913548: Source element values are replaced with translated values.

In case there is an entity created in non-default site's language (with a translation in site's default language), just loading and resaving this entity would override entity's values in a source language.

[#2913548] Comment #5 describes the steps to reproduce it in core.

Edit: Another way to reproduce would be if you use admin panel in a language different than site's default language and create a config entity, that language will be used as the config entity's source language. Then, loading and saving it would mess up entity's values.

Based on the above, I would classify this issue as a major bug.

Actually, I'm pretty sure this is a critical as it results in data loss/overwriting.

There are various other variants for this, I'm actually very confused how that hasn't been a bigger issue so far.

The basic problem is that config entities are by default loaded with overrides and when saving, we load the editable config and overwrite it. In the UI, that usually works fine, because a) you usually use the default language and b) config is loaded without overrides for the edit forms.

But any code that loads and saves configuration outside of the admin context can be affected by this, for example update functions.

The most recent problem that we noticed in a project is that we executed the 8.4 updates in the italian language while the default language is german. Somehow the normal updates don't do language negotation, so that didn't do much and the current language was DE. But then the cache clear happens, which triggers it and then the current language is IT. The post updates are executed, including the one that resaves all views. That loaded the views, loads the italian overrides, changes some stuff and then saves them again. Now two fun things happened:

1) Obviously it now saved all the italian translations it found for those views, instead of the raw (german) labels.
2) Now, because that's not enough, \Drupal\locale\LocaleConfigSubscriber::onConfigSave kicks in, finds translations and saves them back, and because the config langcode is de, it saves those italian strings as german translations of the original value.*

This is actually also not config_translation related, loading the overrides and saving the back is locale and the fix might need to be in the config entity system itself.

As a workaround, we can probably disable overrides during updates (including post updates) but this can also happen anywhere when updating configuration. The only reliable thing that I can think of right now is that config entities keep track of whether they were loaded with or without overrides and then compare their data against the original data and only update/save a diff against that.

The only reliable thing that I can think of right now is that config entities keep track of whether they were loaded with or without overrides and then compare their data against the original data and only update/save a diff against that.

This is exactly what #2923004: Add method to check if any overrides are applied to \Drupal\Core\Config\Config is doing!

That's for the config object. We need it for the config entity object, as there is no connection between the two.

Also faced when panel or view edited in French and becomes trouble to translate

Thinking about this some more I guess we have the same problem as the ImmutableConfig is trying to solve. We need to prevents saves if there are overrides in place. Or try and do what @Berdir suggests and only apply the changes - but that feels super tricky.

Yes, it is the same problem and with simple config, it does work just fine. But somehow we didn't really think config entities through when doing this.

I'm not sure how you propose to solve it other than what I suggested? Only supporting saving config entities when they were loaded using loadMultipleOverrideFree() would IMHO be a major API change, we can't just do that?

Thinking more about this, I don't think my idea of trying to detect what changed is realistic, who knows what kind of side effect having the overrides loaded could have on the code that is changing the values and what should happen if a property that was overridden should be changed.

But what @alexpott said in #9 makes sense (as always), we need to prevent saving if there actually are overrides in place, which is not the same as explicitly loading without overrides. Otherwise I'd expect we have dozens if not hundreds of cases in core alone. We could then think about forcing an override free load when afterwards trying to save config entities always in 9.x?

But I think we also need to address the most common scenario which is updates. While it is not recommended to use config entities in update functions, at least in post updates it is officially supported. Not quite sure how to address that, we don't have a global flag to disable overrides in storage, we only have methods that set the flag for a specific load. And even if we'd add an explicit setOverrideFreeLoad(TRUE), that would be per entity type, so it would need to be something on the entity type manager which can then enforce it on storages? or maybe on the config factory?

Something like this, lets see if something breaks with this change.

Aw, ViewUI...

Ok, so I only need to call setHasOverrides() when we are loading with overrides.

I also tried to do a test that runs in a translated language but it fails hard on the second step when building the block plugins:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'revision.revision_user' in 'field list': SELECT revision.revision_id AS revision_id, revision.langcode AS langcode, revision.revision_user AS revision_user, revision.revision_created AS revision_created, revision.revision_log AS revision_log, revision.revision_default AS revision_default, base.id AS id, base.type AS type, base.uuid AS uuid, CASE base.revision_id WHEN revision.revision_id THEN 1 ELSE 0 END AS isDefaultRevision FROM {block_content} base INNER JOIN {block_content_revision} revision ON revision.revision_id = base.revision_id; Array ( ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->getFromStorage() (line 455 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

Drupal\Core\Database\Statement->execute(Array, Array) (Line: 625)
Drupal\Core\Database\Connection->query('SELECT revision.revision_id AS revision_id, revision.langcode AS langcode, revision.revision_user AS revision_user, revision.revision_created AS revision_created, revision.revision_log AS revision_log, revision.revision_default AS revision_default, base.id AS id, base.type AS type, base.uuid AS uuid, CASE base.revision_id WHEN revision.revision_id THEN 1 ELSE 0 END AS isDefaultRevision
FROM 
{block_content} base
INNER JOIN {block_content_revision} revision ON revision.revision_id = base.revision_id', Array, Array) (Line: 87)
Drupal\Core\Database\Driver\mysql\Connection->query('SELECT revision.revision_id AS revision_id, revision.langcode AS langcode, revision.revision_user AS revision_user, revision.revision_created AS revision_created, revision.revision_log AS revision_log, revision.revision_default AS revision_default, base.id AS id, base.type AS type, base.uuid AS uuid, CASE base.revision_id WHEN revision.revision_id THEN 1 ELSE 0 END AS isDefaultRevision
FROM 
{block_content} base
INNER JOIN {block_content_revision} revision ON revision.revision_id = base.revision_id', Array, Array) (Line: 510)
Drupal\Core\Database\Query\Select->execute() (Line: 455)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->getFromStorage(NULL) (Line: 426)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->doLoadMultiple(NULL) (Line: 242)
Drupal\Core\Entity\EntityStorageBase->loadMultiple() (Line: 46)
Drupal\block_content\Plugin\Derivative\BlockContent->getDerivativeDefinitions(Array) (Line: 101)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives(Array) (Line: 87)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions() (Line: 284)
Drupal\Core\Plugin\DefaultPluginManager->findDefinitions() (Line: 175)
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() (Line: 22)
Drupal\Core\Plugin\DefaultPluginManager->getDefinition('system_menu_block:account') (Line: 16)
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('system_menu_block:account', Array) (Line: 76)
Drupal\Component\Plugin\PluginManagerBase->createInstance('system_menu_block:account', Array) (Line: 62)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->initializePlugin('system_menu_block:account') (Line: 57)
Drupal\block\BlockPluginCollection->initializePlugin('system_menu_block:account') (Line: 80)
Drupal\Component\Plugin\LazyPluginCollection->get('system_menu_block:account') (Line: 45)
Drupal\block\BlockPluginCollection->get('system_menu_block:account') (Line: 83)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->setConfiguration(Array) (Line: 99)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->addInstanceId('system_menu_block:account', Array) (Line: 55)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->__construct(Object, 'system_menu_block:account', Array) (Line: 34)
Drupal\block\BlockPluginCollection->__construct(Object, 'system_menu_block:account', Array, 'bartik_account_menu') (Line: 149)
Drupal\block\Entity\Block->getPluginCollection() (Line: 138)
Drupal\block\Entity\Block->getPlugin() (Line: 113)
Drupal\block\BlockAccessControlHandler->checkAccess(Object, 'view', Object) (Line: 105)
Drupal\Core\Entity\EntityAccessControlHandler->access(Object, 'view', Object, 1) (Line: 362)
Drupal\Core\Entity\Entity->access('view', NULL, 1) (Line: 56)
Drupal\block\BlockRepository->getVisibleBlocksPerRegion(Array) (Line: 137)

Makes sense to me that this happens because we use the latest entity type definitions to run queries, but obviously at this point the schema has not yet been updated. Not sure why it only happen with a translated language, I tried clearing discovery cache on a normal update path test, didn't fail like that.

This is basically what #2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code is about.

There are different ways to trigger this, one would be to have a view or other config in a non-default language with an override for the default language, which is how this was originally reported I think. I'll look into that tomorrow.

Forget the previous patch, diffed against an older commit also the interdiff was incomplete.

This has a few renames and interdiff would be huge, so just starting from scratch without interdiff.

Fixed and extended the failing kernel/unit tests. Added a upgrade test that creates a spanish view with en overrides, this now makes the upgrade fail hard, even with english. I was a bit lazy with the view and copied one of the existing ones and just renamed it and copied a few keys around, so the en "translation" still has mostly spanish labels, might clean that up a bit more. But it's enough to show the problem.

New title.

Looks like we have extensive test coverage.

some review questions/comments:
1.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -639,4 +646,18 @@ public function save() {
+  public function setHasOverriddes($is_overridden) {

1. s/Overriddes/Overrides?
2. also shouldn't it be setOverrides instead of setHasOverrides?

2. Would it be worth throwing a special exception as we know the specific reason why this happen instead of generic EntityMalformedException?

Disallow to save config entities with applied configuration overridden

Crystal-clear title now 👌

Well, the grammar isn't so crystal-clear yet ;)

#22

1. Thanks for the review. changed to Overrides, complicated word ;) I don't think setOverrides() makes sense, we don't set overrides, we just set if there are any.

2. Yes, a separate exception probably makes sense, this is still pretty early, didn't want to spend time on stuff like that until we figure out the big questions/make the tests pass.

Here is a very ugly proof of concept that adds a new method on ConfigFactory to disable config overrides globally, just serves as a flag right now that is used by ConfigEntityStorage (we can't set it on config entity storage as there are dozens of those instances). It passes my config update test, which now is extended to also assert for the string and fails on its own.

Below are my steps to reproduce the issue and confirm that the patch addresses the problem by throwing an exception
From: #2913548: Source element values are replaced with translated values

• Setup new install in English
• Enable Configuration Translation (/admin/modules)
• Add 'Spanish' (/admin/config/regional/language)
• Translate the 'Article 'content type to Spanish (/admin/structure/types/manage/article/translate)
• Confirm Article in 'English' (/en/admin/structure/types/manage/article)
• Confirm Article in 'Spanish' (/admin/structure/types/manage/article/translate/es/edit)
• Change Default language to 'Spanish' (/admin/config/regional/language)
• Execute drush command to resave 'Article' content type. `drush ev "\Drupal\node\Entity\NodeType::load('article')->save();"`
• Confirm Article title is now 'Spanish' (/admin/structure/types/manage/article/translate)

Drupal\Core\Entity\EntityMalformedException: A config entity with config overriddes must not be saved. Use \Drupal\Core\Config\Entity\ConfigEntityStorageInterface::loadOverrideFree() to load a non-overridden config entity.

When I use...

drush ev "\Drupal::entityTypeManager('article')->getStorage('node_type')->loadOverrideFree('article')->save();"

Everything is fine.

Ah, this passed locally but I still had the exception commented out to check the test without the changes. Have to add the override free check also in the second place.

The only remaining test fails are the unit tests, I already fixed those once and it would be trivial to fix them again but I'm pretty sure that this is not the final API, so we'll have to update them again anyway.

What this needs now is architecture review/input, so I'll set it back to needs review.

Posted my dreditor review too early, so commenting directly here:
I think the new methods and the places where they are checked are perfect.

However I do not think it should also throw EntityMalformedException.
Interestingly enough, while \Drupal\Core\Config\Entity\ConfigEntityStorage::save() is only documented as throwing that one exception class, it also has a custom exception class ConfigEntityIdLengthException.
I think this should also use a more focused class, and not masquerade as a malformed entity.
But that's just my opinion, if you feel strongly that being overridden equates to being malformed, so be it.

1. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
   @@ -59,6 +59,13 @@ class ConfigFactory implements ConfigFactoryInterface, EventSubscriberInterface
   +   * Whether or not overrides should be ignored globally.
   
   +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -106,6 +106,13 @@
   +   * Whether the config entity contains overriden data.
   

   Nit: Indicates whether

2. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
   --- a/core/lib/Drupal/Core/Config/ConfigFactoryInterface.php
   +++ b/core/lib/Drupal/Core/Config/ConfigFactoryInterface.php
   
   +++ b/core/lib/Drupal/Core/Config/ConfigFactoryInterface.php
   @@ -117,4 +117,22 @@ public function listAll($prefix = '');
   +  public function setOverrideFree($override_free = TRUE);
   ...
   +  public function isOverrideFree();
   

   This should fall under the 1:1 rule for BC, since 99% of people will not implement ConfigFactoryInterface without also extending ConfigFactory

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -639,4 +646,18 @@ public function save() {
   +   * @inheritDoc
   ...
   +   * @inheritDoc
   
   +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -1351,4 +1352,18 @@ public function addCacheTags(array $cache_tags) {
   +   * @inheritDoc
   ...
   +   * @inheritDoc
   

   Nit: Someone's editor is misconfigured, these aren't the right style {@inheritdoc}

4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
   @@ -217,4 +217,22 @@ public function trustData();
   +   * Set if the config entity has overridden data.
   

   Nit: Sets

5. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -248,6 +252,11 @@ public function save(EntityInterface $entity) {
   +      throw new EntityMalformedException('A config entity with config overriddes must not be saved. Use \Drupal\Core\Config\Entity\ConfigEntityStorageInterface::loadOverrideFree() to load a non-overridden config entity.');
   

   Super nit: don't end exception messages with a full stop :)

6. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -150,6 +150,10 @@ public function handle($op, Request $request) {
   +    // Disable config entity overrides. Where should this go? Can we also make
   +    // it work for drush updates?
   

   Either this needs to be resolve before commit, or the second/third sentences need to be split out to @todo lines with an issue link.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -183,7 +183,7 @@ protected function doLoadMultiple(array $ids = NULL) {
+      $records[$id] = $this->overrideFree || $this->configFactory->isOverrideFree() ? $config->getOriginal(NULL, FALSE) : $config->get();

some brackets here would help readability, and prevent weird bugs if someone modified this later and the order of operations got out of whack

i like the direction here

Thanks for the feedback so far, some quick answers/open questions that I have

* very open to introduce a new exception, I was just being lazy and copy & pasted an existing check.
* Feels a bit strange to have override methods on ConfigFactory that it itself doesn't care about, we just (mis-)use it as a place to store the state right now
* alexpott wasn't too happy about adding setHasOverrides() to config entity, but the only alternative I can think of is smuggling it in through the config data, e.g. inside _core, which also isn't nice. The method could be @internal with instructions that it should not be called by anyone else but the storage?
* when and how to set the override free mode.. we have \Drupal\Core\Update\UpdateKernel, maybe we can do something through that, if drush uses that too then it might work there too

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
   @@ -217,4 +217,22 @@ public function trustData();
   +  /**
   +   * Set if the config entity has overridden data.
   +   *
   +   * @param bool $overridden_data
   +   *   Whether the config entity has overridden data.
   +   *
   +   * @return $this
   +   */
   +  public function setHasOverrides($overridden_data);
   

   ❓ I would kinda expect that this isn't something determined from outside.

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -183,7 +183,7 @@ protected function doLoadMultiple(array $ids = NULL) {
   +      $records[$id] = $this->overrideFree || $this->configFactory->isOverrideFree() ? $config->getOriginal(NULL, FALSE) : $config->get();
   

   Do you mind adding some extra parenthesis around the right bit of the executed code?

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -248,6 +252,11 @@ public function save(EntityInterface $entity) {
   +      throw new EntityMalformedException('A config entity with config overriddes must not be saved. Use \Drupal\Core\Config\Entity\ConfigEntityStorageInterface::loadOverrideFree() to load a non-overridden config entity.');
   

   It would be nice if this message could explain WHY. Maybe link to the change record introducing config overrides?

It would be nice if we would somehow put into ConfigEntityForm/ConfigFormBase an indicator of the same, aka. some warning message thrown when you load the add form.

I uploaded the patch that tried to fix the obvious comments above I was able to follow on. I also added a unit test that tested one case that wasn't tested (test of load() method if isOverrideFree() method on configFactory returns TRUE). The tests should (hopefully) pass as well.

Unfortunately, I don't feel skilled enough to dig in deeper into some mentioned structural doubts this topic might require, but I would suggest a pragmatic approach here. As this issue critically affects significant number of clients with an issue that is very hard to reproduce and track, every solution that prevents data loss is better than nothing. The follow-up issues can solve the structure further more precisely.

Note: for some reason, interdiff shows the two removed files inside. Please ignore this - all the files are correctly included in the patch.

Thanks for the update.

I've discussed it a bit with @alexpott to try a different approach for the upgrade path override, by re-introducing the maintenance constant and setting it correctly, or maybe something through the update kernel.

The thing is this also has quite a high risk of resulting in exceptions during module installations and other common operations, where it would have previously resulted in accidently overridden data. That is better in some ways but isn't going to make customers happy either especially as there might not be an easy way around it.

For example, there are tons of helper functions and wrappers that save config entities, for example things as simple as user_role_grant_permissions(). A call of that in the wrong language will be enough to throw the new exception if e.g. the role label is translated. For example in media_install(), so when you install media while not being in the default language, things are likely to break. The tests are passing, but we don't have a lot of tests like that with real-life scenarios from multilingual sites.

What would help I think is people using the patch on real sites, testing it extensively there and so on.

Thanks Berdir, that makes sense.

I think that the possible reliable solution in that case would be what you suggested as a second option: "keep track of whether config entities were loaded with or without overrides and then compare their data against the original data and only update/save a diff against that".

To dig in further into this possibility, we already keep track of first part, so we "only" need the second part - to compare their data against the original data and update/save a diff. Is my thinking correct?

Yes, that was my original idea, but what if you start to mix it?

Because the logic that will update will see the overridden values and while they are mostly just string changes/translations with config translation, you can do anything through settings.php based overrides, domain based overrides and so on. What if you increment a value or append something to a string that is overridden? I don't think we can guarantee to reliable do that magic.

Setting this to "needs work" because #38 throws exceptions when Views are loaded/re-saved in the Views post-update operations needed to go from 8.4.5 to 8.5.0, and this makes it impossible to apply them.

We have English as the site default and all our config is stored with langcode 'en'. Swedish is also enabled and we fall back to always using that if no langcode was detected in the URL. Unfortunately language negotiation applies during updates too and any update which touches config always replaces all English source strings with the Swedish translations, which gets quite tedious to clean up.

Seems like this case is not covered by a test yet?

I was hoping there was a patch to fix that by forcing config to be loaded and saved with the source language (or no overrides at all) during updates, and this one seemed to come closest. Some config, like Actions, were updated and re-saved successfully but those source strings were still changed.

> Setting this to "needs work" because #38 throws exceptions when Views are loaded/re-saved in the Views post-update operations needed to go from 8.4.5 to 8.5.0, and this makes it impossible to apply them.

This does need work but that is exactly the point of this change, disallowing that kind of change.

Did you run the updates with drush or with update.php? This only automatically applies the override free thing to update.php currently, we'll either need a separate change for drush or find a way that works for both drush and update.php.

Ah, then I have found the correct issue.

Hah, I just looked closer at the patch and saw the note about Drush in there.
I did use drush updb. Tried again with update.php and then it did work. No exceptions, and no changed source strings, yay!

Maybe it's too much of a hack but setting this new flag in drupal_load_updates() would cover all cases I think.

Exporting the updated configuration with Drush, restoring the database pre-updates and trying to import the updated config fails with this exception on all views though. Maybe the flag can be set in ConfigImporter::import() too?
Update: Importing the config again after that did work without exceptions.

Tentatively adding the CMI 2.0 candidate tag.

Reroll and switched to use the MAINTENANCE_MODE constant, not 100% sure about that yet, but lets see.

Struggled a bit with the interdiff from the last patch, that did some strange things but figured it out.

Also looks like there is no way make it work in drush automatically, so we'll have to separate that. That does mean that instead of silent data loss, people will see an exception on drush updb.

Interestingly, Drush does set MAINTENANCE_MODE for D6 & D7, but not for D8 anymore.

Created an issue and merge requests for drush: https://github.com/drush-ops/drush/issues/3604

Added another constant definition because drush 8.x re-uses our code for the post update functions at least.

Discussed with alexpott, he didn't like setHasOverrides(), neither do I, so we agreed to instead inject it through the values and deal with it in the constructor. This is not a value that anyone except the storage should set. Worst case is that people could just call it with FALSE to force a data loss/overwrite.

Also, testing in our project showed that this breaks classes that extend from ConfigEntityInterface without extending from the base class (obviously) and that search_api has such a class and will therefore be broken by this change. But we are allowed to make such a change according to our BC Policy. We should look into making it easier to have such classes however if that is indeed a common use case.

I've been doing first tests in our project and it is working well so far. I have a webform in a non-default language and without the patch, updating to the latest webform version breaks it completely (only General Element things left), and with this patch as well as the matching drush patch/PR (the patch is easy to apply to a project if you're using drupal-composer and composer-patches), the update did run through without breaking it.

This certainly needs more testing, it's for example possible that actions in the UI will try to save config entities with overrides outside of the upgrade path.

What we need from this is extensive testing on real multilingual sites, try to change/save all kinds of config things and see if you can break it (aka get the exception that prevents an accidental config override).

I think it needs another test to be sure that language override is not drain into edit form

I faced the issue today
- site 2 languages (Fr & En)
- negotiation - user lang and selected (Fr)
- roles page (roles saved in En)
- when I click edit role I see translated text in edit form

> - when I click edit role I see translated text in edit form

Config entity edit forms already ensure that config is not overridden. If you have that, then you probably already accidentally saved the translated label in there, for example I guess saving the permissions form in FR (in your case) might do that.

Got a first exception in a project, and it really was through user_role_grant_permissions(), although on the filter form and not the permissions page, but I suspect the same could happen there.

Steps to reproduce:
* Have a site with DE enabled, default language EN
* add a translation for a role
* Edit a filter format with DE as the active language, grant that role the permission to use that format:

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Drupal\Core\Entity\UnsupportedEntityOperationException</em>: A config entity with config overrides must not be saved. Use \Drupal\Core\Config\Entity\ConfigEntityStorageInterface::loadOverrideFree() to load a non-overridden config entity. Check issue https://www.drupal.org/project/drupal/issues/2910353 for more details in <em class="placeholder">Drupal\Core\Config\Entity\ConfigEntityStorage-&gt;save()</em> (line <em class="placeholder">269</em> of <em class="placeholder">core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php</em>). <pre class="backtrace">Drupal\Core\Entity\Entity-&gt;save() (Line: 652)
Drupal\Core\Config\Entity\ConfigEntityBase-&gt;save() (Line: 1166)
user_role_grant_permissions(&#039;administrator&#039;, Array) (Line: 1140)
user_role_change_permissions(&#039;administrator&#039;, Array) (Line: 219)
Drupal\filter\FilterFormatFormBase-&gt;submitForm(Array, Object) (Line: 33)
Drupal\filter\FilterFormatEditForm-&gt;submitForm(Array, Object)
call_user_func_array(Array, Array) (Line: 111)

We can fix this by changing user_role_grant_permissions() to explicitly load the role without overrides, but it's going to require a lot of changes in a lot of places to always do that... But there is no other way, it's either that exception or data loss.

Ok, this test fails both on HEAD (by overwriting the role name with the translation) and with the previous patch (by throwing the exception).

I fear there is going to be a considerable amount of such cases that could result in such an error, I doubt we can identify, fix and test them all.

Adding the legacy group that upgrade path tests now need.

There is one more problem. webform has some custom logic because it stores its structure in a yaml string instead of a nested structure. Because of that, it needs to deal with language override itself and merge those together again. Not sure why it does that, we wouldn't have that problem if it would let merging overrides, it also only supports language and not settings.php/domain/... overrides.

Anyway, the problem is that currently, we only have a flag about whether a config entity *actually* has overrides and not whether it tried to load overrides or not. I think we need a second flag for that, also because I think we might want to deprecate saving config entities with applies config overrides. That would be a good way to figure out what other places in core/contrib could be affected by this.

Discussed with @alexpott a bit, lets see what happens when we add a deprecation message on saving non-override free config entities.

Some test runs show that there might be fewer than I feared, the most common one indeed seems to be user permissions.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -252,6 +268,11 @@ public function save(EntityInterface $entity) {
+    /** @var \Drupal\Core\Config\Entity\ConfigEntityInterface $entity */
+    if ($entity->hasOverrides()) {
+      throw new UnsupportedEntityOperationException('A config entity with config overrides must not be saved. Use \Drupal\Core\Config\Entity\ConfigEntityStorageInterface::loadOverrideFree() to load a non-overridden config entity. Check issue https://www.drupal.org/project/drupal/issues/2910353 for more details');
+    }
+

Not allowing the the importing of config entities which have been overridden is really a problem.

These are generally overridden in the settings.php and will not get blown away, but just override on the next load.

In the case where I need this change I have changes overridden in settings.php which I do not want saved to disk but I will not loose these changes if they are updated. But with this throwing of an exception it breaks the config import into all environments except for production.

Yes, this patch is WIP and breaks various things. But that's not the fault of that exception, that's the fault of those many things loading a config entity *with* overrides and then saving. The config importer needs to be updated to use loadOverrideFree(), then it will work just fine. (and then another 100 places too)

This is a reroll of #61, which we are using on our projects that fixes config import of config with overrides. Will reroll for 8.7 once we'll update to that, but I think it is not viable to introduce the deprecation here in this issue, fixing all those tests is going to be a massive effort but also importnat because at least all the places outside test-only code will indicate places where it could break on real sites with overrides.

Damn, wrong patch/project.

We're also experiencing data loss (previously translated strings are now set to their English source) but our sites have English as a default language (we do use a the "Selected language" negotiator to display Dutch by default).

You can ignore this comment, turns out there was a bug in admin toolbar.

Do we have a test how this behaves for config that is configured as language undefined and language unspecified?

Needs a reroll.

Added reroll as there were merge conflicts with hasOverrides.

The default entity storage interface Drupal\Core\Entity\EntityStorageInterface is used in several places:
- ConfigEntityListBuilder::storage (inherited from EntityListBuilder)
- ConfigEntityListBuilder__construct (inherited from EntityListBuilder)
- ConfigEntityListBuilder::getStorage (inherited from EntityListBuilderInterface)

Drupal\Core\Entity\EntityStorageInterface doesn't provide loadMultipleOverrideFree so we should update it:

class ConfigEntityListBuilder extends EntityListBuilder {

  /**
   * The entity storage class.
   *
   * @var \Drupal\Core\Config\Entity\ConfigEntityStorageInterface
   */
  protected $storage;

  /**
   * Constructs a new ConfigEntityListBuilder object.
   *
   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
   *   The entity type definition.
   * @param \Drupal\Core\Config\Entity\ConfigEntityStorageInterface $storage
   *   The entity storage class.
   */
  public function __construct(EntityTypeInterface $entity_type, ConfigEntityStorageInterface $storage) {
    parent::__construct($entity_type, $storage);
  }

  /**
   * Gets the entity storage.
   *
   * @return \Drupal\Core\Config\Entity\ConfigEntityStorageInterface
   *   The storage used by this config list builder.
   */
  public function getStorage() {
    return $this->storage;
  }

Added a patch to fix the interface issues mentioned in #73 and fixes BlockListBuilder::submitForm.

BlockListBuilder::submitForm should be using the loadMultipleOverrideFree or we'll run into the UnsupportedEntityOperationException when saving the block form ui (e.g. when reordering blocks with overrides):

   public function submitForm(array &$form, FormStateInterface $form_state) {
-    $entities = $this->storage->loadMultiple(array_keys($form_state->getValue('blocks')));
+    $entities = $this->storage->loadMultipleOverrideFree(array_keys($form_state->getValue('blocks')));

This is the patch for #74.

Drupal\user\AccountSettingsForm does this to Roles as well:

  $this->roleStorage->load($rid)->setIsAdmin(FALSE)->save();

Having trouble generating a patch right now, but it should be:

  $this->roleStorage->loadOverrideFree($rid)->setIsAdmin(FALSE)->save();

Other than that the patch in #74 seams to be working fine. We have it running on a pretty complex site with loads of custom and contributed config entites. Errors are raised when they should.

Reroll for 8.8.x and also including the part for AccountSettingsForm.

I'm still unsure on how to get this in. I'm sure there are still tons of config entity saves like this that can cause problems. And committing a patch that is going to throw exceptions like this is not going to be committed.

It might be a better solution to raise a notice and refuse to save rather than an raising an error.

I do agree, but the ->save() call itself neds to throw an exception, that's the only way to abort the process at that point. It is the job of the form to catch those exceptions (or validate it before that) and present them to the user in a better way. Unfortunately, we're not very good at doing that at the moment and each place that triggers such a save (often indirectly through helper functions) needs to be updated specifically for that.

I'm using the patch and it solves the issue with re reappearing translations. But I don't understand the risk, you mentioned above.

I can see that an exception is thrown. But which module / use-case could raise such an exception?

The patch breaks search_api module because it introduced a new method "hasOverrides" in the interface and search_api "UnsavedIndexConfiguration" does not implement this method.

Yes: #2983999: Add new hasOverrides method to UnsavedIndexConfiguration.

Reroll for 8.8.x and also solving failed test cases and removing following coding standards error

solving failed test cases.

Apologies members , due to some project commitments , not able to work hence unassigned this issue.

I have another use case for part of this patch - the addition of the hasOverrides method.

I am working on SalesForce integration, and want to be able to detect when a database is copied from production to a non-production environment, so that I don't continue to use production settings. If I can detect that the settings are not the same as the last time a check was made, I can know to invalidate auth tokens. The trick, though, is detecting that accurately.

Among other things, I need to be able to know whether the settings loaded in the config that records authentication details are loaded override free. If they are, I might get false positives or false negatives and so should not use that instance of the config entity to determine whether things have changed.

Hope that helps in some way.

After further testing, I've realised I was wrong in thinking I could use this patch. I need to know whether the config entity was loaded override free, not whether there actually are any overrides applied. I'll open another issue.

I just made fresh website and I learned its broken, just hours launch and I am a bit stuck.

I know little about configurations, but yes, I made my site's default language non-english one.

1. What can I do to fix my website?
2. Since this bug is known for so long, couldn't anything be done to prevent new users to brake their new sites?
3. Could Drupal simply prevent changing language to non english one? To force English be default always and this would be avoided.

Reroll of patch #89 on 9.2.x

cspell fixes

Fixing the cspell fixes sigh

Tried to handle a few of the failed test cases.

One more test fix.

Rebased, conflicted on AccountSettingsForm, the role stuff was moved to another form.

- Patch against 9.4.x
- Fixed cspell error

I think this goes further than translation overrides.

If one where to override some config in the $config array in the settings.php file and then change another value in the same config file (in the UI). When the config is exported it will have the value from the $config array override from the settings.php file.

> I think this goes further than translation overrides.

Absolutely, nothing about this is specific to language overrides.

Reroll.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed patch failed to apply.

try to fixed CCF #112 and remove the second parameter from the expectException() method call and for custom error message, i used the expectExceptionMessage().

Pretty weird conflict on ConfigEntityListBuilder, another issue added the same property but differently. Removed all that. Also attempting to fix the cspell error properly.

+++ b/core/includes/update.inc
@@ -220,6 +220,11 @@ function update_do_one($module, $number, $dependency_map, &$context) {
 function update_invoke_post_update($function, &$context) {
   $ret = [];
 
+  // Disable config entity overrides.
+  if (!defined('MAINTENANCE_MODE')) {
+    define('MAINTENANCE_MODE', 'update');

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -178,9 +179,21 @@ protected function doLoadMultiple(array $ids = NULL) {
+    // Never load config with overrides when running updates.
+    if (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update') {
+      $this->overrideFree = TRUE;

+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -150,6 +150,11 @@ public function handle($op, Request $request) {
+    // Disable config entity overrides.
+    if (!defined('MAINTENANCE_MODE')) {
+      define('MAINTENANCE_MODE', 'update');

Why overrides are disabled only for update, I guess "install" also should be checked.

Checking src I got that it's only way to get the state of update. It needs follow-up to implement some better API to disable overrides... maybe a-la \Drupal\Core\Installer\InstallerKernel::installationAttempted()

OTOH there's #3126222: Introduce 'update' environment for DrupalKernel

I am trying to understand this change. As I understand from the tests, on a multilingual site the config can be only edited in the source language.
So if the site default language is Hungarian, then the developer will have to edit the configs in Hungarian, even if he does not speak Hungarian. Maybe I should test the patch, as that can't be true.

Regular editing is not a problem in most cases, it's things like the block overview, when you reorder blocks in a language that is not the block config default language, you overwrite the blocks with translations of the current language.

Tried reproducing issue using steps mentioned in #28 and patch applied but failed test for 11.x

In /core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php

$this->moduleHandler->getImplementations('entity_load')->willReturn([]);

The method getImplementations() is deprecated in drupal:9.4. 0 and is removed from drupal:10.0.0

Added patch against 11.x

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Converted the patch as a MR , will try to update the https://www.drupal.org/project/drupal/issues/2910353#comment-15031176 in the next commit , definitely we should check for 'install' scenario as well .

Thanks !

Seems to have test failures also.

I have added a fix for the failed test https://git.drupalcode.org/issue/drupal-2910353/-/jobs/676914 and https://git.drupalcode.org/issue/drupal-2910353/-/jobs/676912 , and along with that i saw 1 tests is failing unexpectedly , which looks unrelated to the changes added here , I am adding the failures from the test here for visibility so can someone do a quick review and suggest a path forward.

1. https://git.drupalcode.org/issue/drupal-2910353/-/jobs/676917

 RuntimeException: Adding non-existent permissions to a role is not allowed.
    The incorrect permissions are "use text format oqcx2iea".

Thanks!

Thanks for pushing this forward!

A single remaining failure:

Drupal\Tests\text\Functional\TextFieldTest::testTextfieldWidgetsFormatted
RuntimeException: Adding non-existent permissions to a role is not allowed. The incorrect permissions are "use text format lj1srap9".

So close! 🥹

All the tests are passing now , moving to ready for review !

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

That's great, thanks for your work.

There are some major considerations to take into account though:

* Not using the API correctly will result in an uncaught exception. This *can* happen to regular users as well if contrib/custom UI's don't use the API. This is pretty bad, but the only alternative is data loss.
* One common example where we still see this pop up in our projects is installing modules. There config is often loaded and adjusted, for example form/view displays. Maybe install could also do the same maintenance check as we do for updates now.
* This adds a new method on ConfigEntityInterface, this can break contrib, for example search_api.
* This currently only throws an exception if data loss would really happen. What we should actually do is deprecate *any* config entity save operationthat isn't using loadOverrideFree(), just like we did for simple config, where you can't save unless you use getEditable(). That causes *thousands* of deprecations (a lot in tests, but by far not all). I guess it makes sense to do that in a follow-up. See #63.

I apologize for not adding this in my earlier review. But this needs an issue summary update. I've added the template but not familiar enough with the issue to fill in the missing pieces.

Hiding patches for clarity as the fix is in MR 6268 now

We found another use case when deleting a config entity causes other entities to be updated, so we need those to be loaded without overrides as well.

I don't understand the change that was done to user_role_grant_permissions() and user_role_revoke_permissions(). The logic seems inverted and there is absolutely no reason to ever load roles with overrides in those functions. Config entities that are saved must always be loaded without overrides.

I guess it was done to fix Drupal\Tests\text\Functional\TextFieldTest::testTextfieldWidgetsFormatted() but that doesn't seem right, the test fail I think is valid and points to a flaw somewhere. And I think that flaw is how role config entities are statically cached. \Drupal\Core\Config\Entity\ConfigEntityStorage::buildCacheId() is dynamic based on overrideFree being enabled or not. That means two versions of the same config entity can be cached at the same time, but saving is only going to invalidate one of them.

I've now added an overridden version of that invalidates always using the tag, that means it always invalidates all entities of that type, but I think the overhead of that is minor as config entity saves really shouldn't happen that often.

There's a second change here that on it's own would also fix the test. The specific reason it happens here is that the test disables all text formats, which triggers user_filter_format_disable(), which removes that permission, but this doesn't use loadOverrideFree() yet, so it's invalidating the non-override version. Changing this on its own also fixes this test, but I think it makes sense to fix resetCache() as well.

I expect that those changes might trigger some unit test fails, but we'll just have to update that then.

Also did a first pass of an issue summary.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Uploaded MR diff as patch for ease of use

#140 does not apply to 11.1.7 so uploaded a new diff of the MR.

Tests are now green.

Removing "Needs issue summary update" because the summary was updated in #136.

I thought the changes to user_role_grant_permissions() were lost by mistake will working on the MR, but it turns out they were committed in #3497325: Config entity static cache is not cleared correctly when multiple language overrides are used: https://git.drupalcode.org/project/drupal/-/commit/30fb2bde8c479d9fd3be0...

Believe the API change will need a CR

I'm unsure how to best proceed with this.

One thing to consider is dropping the update specific logic. That's where we originally encountered this, but I think it makes this more complicated (why special case update, why not also install, ...).

I think a feasible option is to split of the actual fix in separate issues and get them in, like the user role stuff in #3497325: Config entity static cache is not cleared correctly when multiple language overrides are used. Everything that we need for that is present in HEAD, we just need to use the API properly. The work done here is forcing that data isn't being overwritten/corrupted, at the price of exceptions for end users and also things like module installs, which can be very tricky to recover from properly.

Patch for Drupal 10.5 for those who patch per file.

diff MR as patch

I agree the changes to BlockListBuilder, RoleSettingsForm, etc should probably be split into their own issues.
These small fixes will be easier to review and commit separately.

Then this issue can focus on warning when a config entity with overrides is saved.
(An exception might be a bit too disruptive but using this patch helped us finding bugs in multiple contrib modules.)

I opened separate issues for fixing config loading in these classes:

• ConfigManager: #3326900: ConfigManager::findConfigEntityDependenciesAsEntities() should not load config overrides
• BlockListBuilder: #3570219: BlockListBuilder::submitForm() should not load config overrides
• RoleSettingsForm: #3570220: RoleSettingsForm::submitForm() should not load config overrides

I think we should get these committed then this issue can focus on triggering an exception when overridden config is saved.

The work done here is forcing that data isn't being overwritten/corrupted, at the price of exceptions for end users and also things like module installs, which can be very tricky to recover from properly.

Maybe we should use an assertion instead?
This way it does not break production websites but we still have a way to detect these problems early in dev.

FYI, I removed the update special cases from this. Instead of this, I updated the config entity updater to use this explicitly. This *will* quite likely result in people running into errors during updates.

I tested this now on a D10 => D11 update and had two problems in contrib projects:

* #3572817: node_revision_delete_update_9001() does not use loadOverrideFree()
* #3572819: Use ::loadMultipleOverrideFree() in webform_update_8648()

There's also a change in importUpdate() that I think we should also push into #3326900: ConfigManager::findConfigEntityDependenciesAsEntities() should not load config overrides and as suggested by alexpott, expand the scope there to config import/sync situations.

Additionally, this also has a test for the role UI, which I think now passes on HEAD as we merged those changes and could also put that in a separate issue.

Uploading diff of MR as patch (applies to 11.3.5)

@berdir I can confirm ConfigTranslationRoleUiTest passes on main so I created a separate issue: #3582050: Add test for role UI when using config translation

These have been committed:

• #3570219: BlockListBuilder::submitForm() should not load config overrides
• #3326900: ConfigManager::findConfigEntityDependenciesAsEntities() should not load config overrides
• #3570220: RoleSettingsForm::submitForm() should not load config overrides

So I rebased the MR and moved ConfigTranslationRoleUiTest to #3582050: Add test for role UI when using config translation.