BUG Config updates are now applied after middleware not before

src/Core/Config/Config.php

@@ -4,6 +4,7 @@ namespace SilverStripe\Core\Config;
 
 use InvalidArgumentException;
 use SilverStripe\Config\Collections\ConfigCollectionInterface;
+use SilverStripe\Config\Collections\DeltaConfigCollection;
 use SilverStripe\Config\Collections\MutableConfigCollectionInterface;
 
 abstract class Config
@@ -25,6 +26,13 @@ abstract class Config
      */
     const EXCLUDE_EXTRA_SOURCES = 4;
 
+    /**
+     * Disable all modifications to the config
+     *
+     * @const
+     */
+    const NO_DELTAS = 8;
+
     /**
      * Get the current active Config instance.
      *

src/Core/Config/CoreConfigFactory.php

@@ -4,6 +4,7 @@ namespace SilverStripe\Core\Config;
 
 use Psr\SimpleCache\CacheInterface;
 use SilverStripe\Config\Collections\CachedConfigCollection;
+use SilverStripe\Config\Collections\DeltaConfigCollection;
 use SilverStripe\Config\Collections\MemoryConfigCollection;
 use SilverStripe\Config\Transformer\PrivateStaticTransformer;
 use SilverStripe\Config\Transformer\YamlTransformer;
@@ -47,6 +48,11 @@ class CoreConfigFactory
     {
         $instance = new CachedConfigCollection();
 
+        // Override nested config to use delta collection
+        $instance->setNestFactory(function ($collection) {
+            return DeltaConfigCollection::createFromCollection($collection, Config::NO_DELTAS);
+        });
+
         // Create config cache
         if ($this->cacheFactory) {
             $cache = $this->cacheFactory->create(CacheInterface::class . '.configcache', [

src/Core/Config/Middleware/ExtensionMiddleware.php

@@ -6,6 +6,7 @@ use Generator;
 use InvalidArgumentException;
 use SilverStripe\Config\MergeStrategy\Priority;
 use SilverStripe\Config\Middleware\Middleware;
+use SilverStripe\Config\Middleware\MiddlewareCommon;
 use SilverStripe\Core\ClassInfo;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Extension;
@@ -15,6 +16,11 @@ class ExtensionMiddleware implements Middleware
 {
     use MiddlewareCommon;
 
+    public function __construct($disableFlag = 0)
+    {
+        $this->setDisableFlag($disableFlag);
+    }
+
     /**
      * Get config for a class
      *
@@ -32,7 +38,7 @@ class ExtensionMiddleware implements Middleware
             return $config;
         }
 
-        foreach ($this->getExtraConfig($class, $config) as $extra) {
+        foreach ($this->getExtraConfig($class, $config, $excludeMiddleware) as $extra) {
             $config = Priority::mergeArray($extra, $config);
         }
         return $config;
@@ -43,15 +49,19 @@ class ExtensionMiddleware implements Middleware
      *
      * @param string $class
      * @param array $classConfig
+     * @param int $excludeMiddleware
      * @return Generator
      */
-    protected function getExtraConfig($class, $classConfig)
+    protected function getExtraConfig($class, $classConfig, $excludeMiddleware)
     {
-        if (empty($classConfig['extensions'])) {
+        // Note: 'extensions' config needs to come from it's own middleware call in case
+        // applied by delta middleware (e.g. Object::add_extension)
+        $extensionSourceConfig = Config::inst()->get($class, null, Config::UNINHERITED | $excludeMiddleware | $this->disableFlag);
+        if (empty($extensionSourceConfig['extensions'])) {
             return;
         }
 
-        $extensions = $classConfig['extensions'];
+        $extensions = $extensionSourceConfig['extensions'];
         foreach ($extensions as $extension) {
             list($extensionClass, $extensionArgs) = ClassInfo::parse_class_spec($extension);
             if (!class_exists($extensionClass)) {
@@ -72,7 +82,11 @@ class ExtensionMiddleware implements Middleware
                         // continue
                 }
                 // Merge config from extension
-                $extensionConfig = Config::inst()->get($extensionClassParent, null, true);
+                $extensionConfig = Config::inst()->get(
+                    $extensionClassParent,
+                    null,
+                    Config::EXCLUDE_EXTRA_SOURCES | Config::UNINHERITED
+                );
                 if ($extensionConfig) {
                     yield $extensionConfig;
                 }

src/Core/Config/Middleware/InheritanceMiddleware.php

@@ -4,12 +4,18 @@ namespace SilverStripe\Core\Config\Middleware;
 
 use SilverStripe\Config\MergeStrategy\Priority;
 use SilverStripe\Config\Middleware\Middleware;
-use SilverStripe\Core\ClassInfo;
+use SilverStripe\Config\Middleware\MiddlewareCommon;
+use SilverStripe\Core\Config\Config;
 
 class InheritanceMiddleware implements Middleware
 {
     use MiddlewareCommon;
 
+    public function __construct($disableFlag = 0)
+    {
+        $this->setDisableFlag($disableFlag);
+    }
+
     /**
      * Get config for a class
      *
@@ -20,18 +26,20 @@ class InheritanceMiddleware implements Middleware
      */
     public function getClassConfig($class, $excludeMiddleware, $next)
     {
-        // Check if enabled
+        // Skip if disabled
+        $config = $next($class, $excludeMiddleware);
         if (!$this->enabled($excludeMiddleware)) {
-            return $next($class, $excludeMiddleware);
-        }
-
-        // Merge hierarchy
-        $config = [];
-        foreach (ClassInfo::ancestry($class) as $nextClass) {
-            $nextConfig = $next($nextClass, $excludeMiddleware);
-            $config = Priority::mergeArray($nextConfig, $config);
-        }
-
             return $config;
         }
+
+        // Skip if no parent class
+        $parent = get_parent_class($class);
+        if (!$parent) {
+            return $config;
+        }
+
+        // Merge with parent class
+        $parentConfig = Config::inst()->get($parent, null, $excludeMiddleware);
+        return Priority::mergeArray($config, $parentConfig);
+    }
 }

src/Core/Config/Middleware/MiddlewareCommon.php

@@ -1,48 +0,0 @@
-<?php
-
-namespace SilverStripe\Core\Config\Middleware;
-
-/**
- * Abstract flag-aware middleware
- */
-trait MiddlewareCommon
-{
-    /**
-     * Disable flag
-     *
-     * @var int
-     */
-    protected $disableFlag = 0;
-
-    public function __construct($disableFlag = 0)
-    {
-        $this->disableFlag = $disableFlag;
-    }
-
-    /**
-     * Check if this middlware is enabled
-     *
-     * @param int|true $excludeMiddleware
-     * @return bool
-     */
-    protected function enabled($excludeMiddleware)
-    {
-        if ($excludeMiddleware === true) {
-            return false;
-        }
-        if (!$this->disableFlag) {
-            return true;
-        }
-        return ($excludeMiddleware & $this->disableFlag) !== $this->disableFlag;
-    }
-
-    public function serialize()
-    {
-        return json_encode([$this->disableFlag]);
-    }
-
-    public function unserialize($serialized)
-    {
-        list($this->disableFlag) = json_decode($serialized, true);
-    }
-}

src/Core/Extensible.php

@@ -247,7 +247,7 @@ trait Extensible
 
         // Build filtered extension list
         $found = false;
-        $config = Config::inst()->get($class, 'extensions', true) ?: [];
+        $config = Config::inst()->get($class, 'extensions', Config::EXCLUDE_EXTRA_SOURCES | Config::UNINHERITED) ?: [];
         foreach ($config as $key => $candidate) {
             // extensions with parameters will be stored in config as ExtensionName("Param").
             if (strcasecmp($candidate, $extension) === 0 ||
@@ -332,7 +332,7 @@ trait Extensible
         $sources = null;
 
         // Get a list of extensions
-        $extensions = Config::inst()->get($class, 'extensions', true);
+        $extensions = Config::inst()->get($class, 'extensions', Config::EXCLUDE_EXTRA_SOURCES | Config::UNINHERITED);
 
         if (!$extensions) {
             return null;
@@ -547,7 +547,7 @@ trait Extensible
             if (in_array($class, self::$unextendable_classes)) {
                 continue;
             }
-            $extensions = Config::inst()->get($class, 'extensions', true);
+            $extensions = Config::inst()->get($class, 'extensions', Config::UNINHERITED | Config::EXCLUDE_EXTRA_SOURCES);
 
             if ($extensions) {
                 foreach ($extensions as $extension) {

tests/php/Core/Config/ConfigTest.php

@@ -33,6 +33,7 @@ class ConfigTest extends SapphireTest
 
     public function testUpdateStatic()
     {
+        // Test base state
         $this->assertEquals(
             ['test_1'],
             Config::inst()->get(ConfigTest\First::class, 'first')
@@ -46,7 +47,7 @@ class ConfigTest extends SapphireTest
         );
         $this->assertEquals(
             [ 'test_2' ],
-            Config::inst()->get(ConfigTest\Second::class, 'first', true)
+            Config::inst()->get(ConfigTest\Second::class, 'first', Config::UNINHERITED)
         );
         $this->assertEquals(
             [
@@ -61,59 +62,108 @@ class ConfigTest extends SapphireTest
             Config::inst()->get(ConfigTest\Third::class, 'first', true)
         );
 
+        // Modify first param
         Config::modify()->merge(ConfigTest\First::class, 'first', array('test_1_2'));
         Config::modify()->merge(ConfigTest\Third::class, 'first', array('test_3_2'));
         Config::modify()->merge(ConfigTest\Fourth::class, 'first', array('test_4'));
 
+        // Check base class
         $this->assertEquals(
             ['test_1', 'test_1_2'],
             Config::inst()->get(ConfigTest\First::class, 'first')
         );
         $this->assertEquals(
             ['test_1', 'test_1_2'],
-            Config::inst()->get(ConfigTest\First::class, 'first', true)
+            Config::inst()->get(ConfigTest\First::class, 'first', Config::UNINHERITED)
+        );
+        $this->assertEquals(
+            ['test_1'],
+            Config::inst()->get(ConfigTest\First::class, 'first', Config::NO_DELTAS)
+        );
+        $this->assertEquals(
+            ['test_1'],
+            Config::inst()->get(ConfigTest\First::class, 'first', Config::NO_DELTAS | Config::UNINHERITED)
         );
 
+        // Modify second param
         Config::modify()->merge(ConfigTest\Fourth::class, 'second', array('test_4'));
         Config::modify()->merge(ConfigTest\Third::class, 'second', array('test_3_2'));
 
+        // Check fourth class
         $this->assertEquals(
             ['test_1', 'test_3', 'test_3_2', 'test_4'],
             Config::inst()->get(ConfigTest\Fourth::class, 'second')
         );
         $this->assertEquals(
             ['test_4'],
-            Config::inst()->get(ConfigTest\Fourth::class, 'second', true)
+            Config::inst()->get(ConfigTest\Fourth::class, 'second', Config::UNINHERITED)
         );
+        $this->assertEquals(
+            ['test_1', 'test_3'],
+            Config::inst()->get(ConfigTest\Fourth::class, 'second', Config::NO_DELTAS)
+        );
+        $this->assertEquals(
+            null,
+            Config::inst()->get(ConfigTest\Fourth::class, 'second', Config::NO_DELTAS | Config::UNINHERITED)
+        );
+
+        // Check third class
         $this->assertEquals(
             ['test_1', 'test_3', 'test_3_2'],
             Config::inst()->get(ConfigTest\Third::class, 'second')
         );
         $this->assertEquals(
             ['test_3', 'test_3_2'],
-            Config::inst()->get(ConfigTest\Third::class, 'second', true)
+            Config::inst()->get(ConfigTest\Third::class, 'second', Config::UNINHERITED)
+        );
+        $this->assertEquals(
+            ['test_1', 'test_3'],
+            Config::inst()->get(ConfigTest\Third::class, 'second', Config::NO_DELTAS)
+        );
+        $this->assertEquals(
+            ['test_3'],
+            Config::inst()->get(ConfigTest\Third::class, 'second', Config::NO_DELTAS | Config::UNINHERITED)
         );
 
+        // Test remove()
         Config::modify()->remove(ConfigTest\Third::class, 'second');
+
+        // Check third class ->get()
         $this->assertEquals(
-            ['test_1'],
+            null,
             Config::inst()->get(ConfigTest\Third::class, 'second')
         );
-        $this->assertTrue(
+        $this->assertEquals(
-            Config::inst()->exists(ConfigTest\Third::class, 'second')
+            ['test_1', 'test_3'],
+            Config::inst()->get(ConfigTest\Third::class, 'second', Config::NO_DELTAS)
         );
         $this->assertEquals(
             null,
-            Config::inst()->get(ConfigTest\Third::class, 'second', true)
+            Config::inst()->get(ConfigTest\Third::class, 'second', Config::UNINHERITED)
+        );
+
+        // Check ->exists()
+        $this->assertFalse(
+            Config::inst()->exists(ConfigTest\Third::class, 'second')
         );
         $this->assertFalse(
-            Config::inst()->exists(ConfigTest\Third::class, 'second', true)
+            Config::inst()->exists(ConfigTest\Third::class, 'second', Config::UNINHERITED)
         );
+        $this->assertTrue(
+            Config::inst()->exists(ConfigTest\Third::class, 'second', Config::NO_DELTAS)
+        );
+
+        // Test merge()
         Config::modify()->merge(ConfigTest\Third::class, 'second', ['test_3_2']);
         $this->assertEquals(
-            ['test_1', 'test_3_2'],
+            ['test_3_2'],
             Config::inst()->get(ConfigTest\Third::class, 'second')
         );
+        // No-deltas omits both above ->remove() as well as ->merge()
+        $this->assertEquals(
+            ['test_1', 'test_3'],
+            Config::inst()->get(ConfigTest\Third::class, 'second', Config::NO_DELTAS)
+        );
     }
 
     public function testUpdateWithFalsyValues()