Prevent saving config entities when configuration overrides are applied

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.

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.

The last submitted patch, 114: 2910353-114.patch, failed testing. View results →

+++ 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 📌 Introduce 'update' environment for DrupalKernel Needs work

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 🐛 Prevent saving config entities when configuration overrides are applied Needs work in the next commit.

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 along with that i saw 2 tests are 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/676912

ErrorException: Method "IteratorAggregate::getIterator()" might add
"\Traversable" as a native return type declaration in the future. Do the
same in implementation "Drupal\Core\Entity\ContentEntityBase" now to avoid
errors or add an explicit @return annotation to suppress this message. in
/builds/issue/drupal-2910353/vendor/symfony/error-handler/DebugClassLoader.php:337

2. 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_revoke_permissions() and user_role_revoke_permissions(). The logic seems inverted git land 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.

To address the problem of language discrepancies in Drupal's configuration when using the admin interface in a language other than the default, a clear and user-friendly solution is required to prevent confusion and errors. Here’s a structured approach to achieve this:
Solution 1: Add a Warning Message in Admin Interface
A simple but effective approach would be to add a warning message or notification in the admin interface, particularly when the user is working in a language other than the default.
• Where to Place the Warning:

• On pages related to enabling modules, adding fields, and configuring translatable content.
• In the configuration settings related to language and content translation.
• When exporting configuration to remind users of potential language mismatches in the configuration.

• Example Warning Message:
o "You are currently editing configuration in a non-default language. This may result in inconsistent language settings when exporting configuration. It is recommended to use the default language for configuration changes."
• Contextual Help: Provide a link to documentation or a "Learn More" popup explaining why configuration changes should be done in the default language and how it impacts the exported configuration.
Solution 2: Force Configuration to Save in Default Language by Default
Modify the configuration flow so that by default, the system saves configuration in the default language. This can help mitigate language discrepancies in exported configurations.
• Changes to Implement:
o During installation or setup, clearly identify the default language.
o Provide an option during the configuration process to enforce saving settings in the default language, even if the admin interface is used in another language.
o Optionally, allow users to override this setting with a warning, but default to saving in the default language.
• User Flow:
o When configuring a module, field, or other settings that are translatable, a default language selector is shown, and the system should always default to this language when saving.
o Provide a setting to force configuration changes to be saved in the default language even if the user interface is set to another language.
Solution 3: Lock the Admin Language to the Default Language by Default
Another solution is to lock the admin interface to the default language by default and only allow the user to switch to another language with clear warnings about the potential risks involved.
• How it Works:
o Upon logging into the admin interface, Drupal detects the default language and presents a clear notification or banner that the admin interface is set to the default language.
o Allow users to change the admin language if needed, but warn them of potential risks if the configuration might not align with the default language.
o Provide a toggle or setting to switch back to the default language automatically for all configuration-related actions.
• Example:
o A setting can be added during setup that forces the admin interface to use the default language unless the user explicitly opts out. If the user switches to another language, a prominent warning or notification should appear.
o “Warning: You are editing the configuration in a language other than the default. Proceed with caution. Changes may not be applied correctly when exporting configuration.”
Solution 4: Provide Clear Distinction Between Content and Configuration
Users sometimes confuse content (which is language-dependent) with configuration (which should be language-neutral or based on the default language). A clear distinction between configuration and content in the admin interface would help alleviate confusion.
• Visual Cues:
o Introduce a visual distinction between content and configuration. For example, use different colors or labels to clearly indicate when a user is editing configuration as opposed to content.
o Use tooltips or inline documentation to guide users and remind them that configuration changes should generally be made in the default language.
• Label Configuration Screens Clearly:
o Add a prominent label or header in the configuration screens like “System Configuration (default language)” and “Content (language-dependent)” to make it clear which type of content they are working on.
o In the content settings pages, clearly label sections for translatable fields or language-dependent settings, as well as the default language for configuration.
Solution 5: Better Installer Flow with Language Clarity
Improve the installer process to make the default language explicit and to give users clear guidance about how language settings will affect configuration.
• Enhancements to Installer:
o During installation, have a step that clearly defines the default language and explains that configuration should be saved in this language.
o Offer the option to force configuration to always be saved in the default language and explain the implications of changing this setting.
• Example Prompt During Installation:
o "We’ve detected the default language as [Language]. Please confirm that configuration changes should always be saved in this language to avoid inconsistencies when exporting configuration."
o “Do you want to lock the admin interface to this language? This is recommended for consistency across your site configuration.”
Solution 6: Documentation and Educational Resources
Provide clear documentation and educational resources for administrators and site builders on the topic of configuration and language settings.
• Educational Popups:
o When editing configuration, provide a small educational popup or tooltip that reminds users of the implications of making changes in a non-default language.

Uploaded MR diff as patch for ease of use