Sorry, meant to file this a month ago when I hit this prepping for a D8 demo, but ran into it again tonight whilst prepping for a NEW demo, so am filing it now. :) https://github.com/webchickenator/drupal8-demo/blob/master/cmi.sh is the script I use, which goes against stable alphas, but I've also verified this problem against HEAD.

When you do a config override on site A, say, uncommenting the $config['system.site']['name'] = 'My Drupal site'; line to force-set the site name, and then save something else on the page like, say, the site slogan, the YAML file gets updated with both the changed slogan and the site name, like so:

 uuid: f46df212-f995-42e7-9400-da0d9f6626f0
-name: Site-Install
+name: 'Drupal 8 [Dev]'
 mail: admin@example.com
-slogan: ''
+slogan: sdfsdf
 page:
   403: ''
   404: ''

Expected behaviour: Only slogan would change.

We need tests for this.

Files: 
CommentFileSizeAuthor
#24 config-2239065-24.patch56.12 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,908 pass(es). View
#24 interdiff.txt578 bytestim.plunkett
#11 config-2239065-11-PASS.patch49.96 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,727 pass(es). View
#11 config-2239065-11-FAIL.patch2.2 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,716 pass(es), 4 fail(s), and 0 exception(s). View
#11 interdiff.txt2.26 KBtim.plunkett

Comments

catch’s picture

webchick’s picture

Note that in alpha11 this seems to have gotten somewhat worse. The site name override is showing up in the diff even when I change a totally unrelated page like user permissions.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Oh my.
Working on this.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
48.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,626 pass(es), 10 fail(s), and 3 exception(s). View
1.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,705 pass(es), 2 fail(s), and 0 exception(s). View

#2098119: Replace config context system with baked-in locale support and single-event based overrides broke this.

Basically, we used to set up the override protection on $this->configFactory directly, now its in the helper $this->config() method.
Making $this->configFactory protected will prevent any forms from making this mistake again.

Something is wrong with my test, the FAIL should fail in 2 places but fails in the wrong place... Will have to look into that.

The last submitted patch, 4: config-2239065-4-PASS.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: config-2239065-4-FAIL.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
49.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,642 pass(es), 4 fail(s), and 0 exception(s). View
985 bytes
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

It should be clearly documented somewhere that you need this behavior in every place you deal with config load/save. +1 to make the code easier, especially because it removes the possibility to do evil things in the constructor.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: config-2239065-7.patch, failed testing.

tim.plunkett’s picture

Thanks @dawehner! I do think the fix is correct, but it appears my test needs some more work. Once I get them passing/failing as expected, I'll ask for another final review.

What's interesting is that the fails are displayed twice by testbot, I'm not sure why it would be run twice...

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.26 KB
2.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,716 pass(es), 4 fail(s), and 0 exception(s). View
49.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,727 pass(es). View

Okay, using $GLOBALS in web test doesn't work, so I need to write out the settings directly.
Rearranging the test a bit.
No other changes.

tim.plunkett’s picture

Title: Overridden config showing up in diff when changing other things on page » Overridden config bleeding through to configuration forms

Opened #2251915: Overridden config entity bleeds through to admin forms for config entities, that was never a fixed problem, while this issue is a regression.

The last submitted patch, 11: config-2239065-11-FAIL.patch, failed testing.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

Kinda sucks that we have to pull it off the container like that in config() but that's the way we've got the API setup. Good fix.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The change looks great.

+++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
@@ -84,6 +84,13 @@
   /**
+   * The config factory.
+   *
+   * @var \Drupal\Core\Config\ConfigFactoryInterface
+   */
+  protected $configFactory;

I think we should be documentation why we are changing the scope from private to protected here. Obvious the whole point of the ConfigTranslationFormBase is to aid in the creation of configuration overrides and the default FormBase override handling because of that.

Gábor Hojtsy’s picture

I think it makes total sense that we access config through the same accessor designed to avoid overrides. Indeed #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects is where the global overrides applicability is in question where people argued the global overrides should apply all the time and their appearance is a feature not a bug in this case. That needs to be figured out there. Its a critical in its own right.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
51.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,712 pass(es), 6 fail(s), and 0 exception(s). View

We don't need to mark it protected, just to redeclare it in the class as private. PHP--

Added some comments.

Thanks for the insight @Gábor Hojtsy, that helps a lot.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
@@ -84,6 +84,15 @@
+   * The config factory.
+   *
+   * This property is redeclared so it can be used directly by this class.
+   *
+   * @var \Drupal\Core\Config\ConfigFactoryInterface
+   */
+  private $configFactory;

Doesn't doing this break the BaseForm::config() function for classes that extend ConfigTranslationFormBase?

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Oops meant to leave at needs review

tim.plunkett’s picture

Neither ConfigTranslationAddForm nor ConfigTranslationEditForm need it, just like none of the other subclasses of FormBase/ConfigFormBase need it. Only the classes that access it directly to handle config loading need to specify it, and that's just FormBase, ConfigFormBase, and ConfigTranslationFormBase.

Status: Needs review » Needs work

The last submitted patch, 17: config-2239065-17.patch, failed testing.

tim.plunkett’s picture

Except, it turns out we were just getting lucky, and private won't work at all for us:
http://3v4l.org/ij3dS

Back to the drawing board.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
55.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,721 pass(es). View
7.54 KB

FormBase has setters and getters for each service, in addition to helper methods. Except there is no configFactory() method.
Using that allows us to bypass all of the private nonsense in subclasses.

I don't know why I didn't think of this before, much cleaner.

tim.plunkett’s picture

FileSize
578 bytes
56.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,908 pass(es). View

After further discussion, added a comment to configFactory() to deter usage.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, I don't think there is anything better we can do. Looks good to me and I _think_ this addresses alex's concerns so going to bump it back to RTBC under the assumption that the doc change won't cause testbot to fail.

webchick’s picture

Assigned: tim.plunkett » alexpott

Probably best for Alex to sign off on this. Thanks so much for taking it on!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: config-2239065-24.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

24: config-2239065-24.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Random fail, back to RTBC.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Form/FormBase.php
@@ -121,10 +126,22 @@ protected function translationManager() {
+  /**
+   * Gets the config factory for this form.
+   *
+   * When accessing configuration values, use $this->config(). Only use this
+   * when the config factory needs to be manipulated directly.
+   *
+   * @return \Drupal\Core\Config\ConfigFactoryInterface
+   */
+  protected function configFactory() {

Well the advantage of the private approach was that classes that extend FormBase never had a config factory to play with - but with this approach they do using $this->configFactory() - it is going to be an easy mistake to make.

tim.plunkett’s picture

Well the private approach didn't actually work except due to coincidence.

In HEAD they have $this->configFactory, here they have $this->configFactory(), but with docs, and we've fixed all of core. I don't see how we can truly lock this down without supporting the needs of ConfigFormBase and ConfigTranslationFormBase.

alexpott’s picture

Assigned: alexpott » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed def5707 and pushed to 8.x. Thanks!

diff --git a/core/modules/config/lib/Drupal/config/Tests/ConfigFormOverrideTest.php b/core/modules/config/lib/Drupal/config/Tests/ConfigFormOverrideTest.php
index 5a22e90..3d908ce 100644
--- a/core/modules/config/lib/Drupal/config/Tests/ConfigFormOverrideTest.php
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigFormOverrideTest.php
@@ -10,7 +10,9 @@
 use Drupal\simpletest\WebTestBase;
 
 /**
- * Tests that config overrides do not bleed through in forms.
+ * Tests config overrides do not appear on forms that extend ConfigFormBase.
+ *
+ * @see \Drupal\Core\Form\ConfigFormBase
  */
 class ConfigFormOverrideTest extends WebTestBase {
 
@@ -20,7 +22,7 @@ class ConfigFormOverrideTest extends WebTestBase {
   public static function getInfo() {
     return array(
       'name' => 'Config form overrides',
-      'description' => 'Tests that config overrides do not bleed through in forms.',
+      'description' => 'Tests config overrides do not appear on forms that extend ConfigFormBase.',
       'group' => 'Configuration',
     );
   }

Removed the unnecessary use of the word bleed :) and improved the test description.

  • Commit def5707 on 8.x by alexpott:
    Issue #2239065 by tim.plunkett | webchick: Overridden config bleeding...
Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config

Yay, superb! Tagging for D8MI since this has profound lessons on how to write config forms for multilingual compatibility, so makes it easier to back-reference.

Gábor Hojtsy’s picture

BTW updated https://drupal.org/node/1934152#comment-8732445 with the potentially bad side effects of this issue, depending on what you use global overrides for -- good stuff to discuss :)

Xano’s picture

This broke our attempts at #2208115: Add DependencySerializationTrait.

Gábor Hojtsy’s picture

Xano: but this is fixed? Is the fix causing issues?!

Xano’s picture

The fix causes problems for #2208115: Add DependencySerializationTrait, yes. I'm not sure whether we want to follow-up on the config factory fix, or if we should change the approach the trait takes, so I just linked the other issue here instead of opening a follow-up already.

tim.plunkett’s picture

This is a critical bug fix. private properties are part of the language.
It's a shame they broke the attempt to use a trait for serialization, but that is really inconsequential compared to this bug.

Status: Fixed » Closed (fixed)

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