Overridden config bleeding through to configuration forms

This is closely related to #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects.

Oh my.
Working on this.

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

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.

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

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.

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.

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.

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.

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.

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.

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

Oops meant to leave at needs review

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.

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.

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.

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

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.

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

Random fail, back to RTBC.

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

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.

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.

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.

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

This broke our attempts at #2208115: Add DependencySerializationTrait.

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

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.

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.