From ccbbcd45a223ffaba4ab33a4fce0375952c20c1d Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Thu, 29 Mar 2018 14:46:34 +0100 Subject: [PATCH 1/2] FIX Fixed bug in config merging priorities so that config values set by extensions are now least important instead of most important --- .../Config/Middleware/ExtensionMiddleware.php | 2 +- tests/php/ORM/DataObjectTest.php | 116 ++++++++++-------- 2 files changed, 67 insertions(+), 51 deletions(-) diff --git a/src/Core/Config/Middleware/ExtensionMiddleware.php b/src/Core/Config/Middleware/ExtensionMiddleware.php index 3edbc5b0d..a79eaa44d 100644 --- a/src/Core/Config/Middleware/ExtensionMiddleware.php +++ b/src/Core/Config/Middleware/ExtensionMiddleware.php @@ -39,7 +39,7 @@ class ExtensionMiddleware implements Middleware } foreach ($this->getExtraConfig($class, $config, $excludeMiddleware) as $extra) { - $config = Priority::mergeArray($extra, $config); + $config = Priority::mergeArray($config, $extra); } return $config; } diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php index 7a9422abf..c0d81c15f 100644 --- a/tests/php/ORM/DataObjectTest.php +++ b/tests/php/ORM/DataObjectTest.php @@ -1102,72 +1102,88 @@ class DataObjectTest extends SapphireTest // Test logical fields (including composite) $teamSpecifications = $schema->fieldSpecs(DataObjectTest\Team::class); + $expected = array( + 'ID', + 'ClassName', + 'LastEdited', + 'Created', + 'Title', + 'DatabaseField', + 'ExtendedDatabaseField', + 'CaptainID', + 'FounderID', + 'HasOneRelationshipID', + 'ExtendedHasOneRelationshipID' + ); + $actual = array_keys($teamSpecifications); + sort($expected); + sort($actual); $this->assertEquals( - array( - 'ID', - 'ClassName', - 'LastEdited', - 'Created', - 'Title', - 'DatabaseField', - 'ExtendedDatabaseField', - 'CaptainID', - 'FounderID', - 'HasOneRelationshipID', - 'ExtendedHasOneRelationshipID' - ), - array_keys($teamSpecifications), + $expected, + $actual, 'fieldSpecifications() contains all fields defined on instance: base, extended and foreign keys' ); $teamFields = $schema->databaseFields(DataObjectTest\Team::class, false); + $expected = array( + 'ID', + 'ClassName', + 'LastEdited', + 'Created', + 'Title', + 'DatabaseField', + 'ExtendedDatabaseField', + 'CaptainID', + 'FounderID', + 'HasOneRelationshipID', + 'ExtendedHasOneRelationshipID' + ); + $actual = array_keys($teamFields); + sort($expected); + sort($actual); $this->assertEquals( - array( - 'ID', - 'ClassName', - 'LastEdited', - 'Created', - 'Title', - 'DatabaseField', - 'ExtendedDatabaseField', - 'CaptainID', - 'FounderID', - 'HasOneRelationshipID', - 'ExtendedHasOneRelationshipID' - ), - array_keys($teamFields), + $expected, + $actual, 'databaseFields() contains only fields defined on instance, including base, extended and foreign keys' ); $subteamSpecifications = $schema->fieldSpecs(DataObjectTest\SubTeam::class); + $expected = array( + 'ID', + 'ClassName', + 'LastEdited', + 'Created', + 'Title', + 'DatabaseField', + 'ExtendedDatabaseField', + 'CaptainID', + 'FounderID', + 'HasOneRelationshipID', + 'ExtendedHasOneRelationshipID', + 'SubclassDatabaseField', + 'ParentTeamID', + ); + $actual = array_keys($subteamSpecifications); + sort($expected); + sort($actual); $this->assertEquals( - array( - 'ID', - 'ClassName', - 'LastEdited', - 'Created', - 'Title', - 'DatabaseField', - 'ExtendedDatabaseField', - 'CaptainID', - 'FounderID', - 'HasOneRelationshipID', - 'ExtendedHasOneRelationshipID', - 'SubclassDatabaseField', - 'ParentTeamID', - ), - array_keys($subteamSpecifications), + $expected, + $actual, 'fieldSpecifications() on subclass contains all fields, including base, extended and foreign keys' ); $subteamFields = $schema->databaseFields(DataObjectTest\SubTeam::class, false); + $expected = array( + 'ID', + 'SubclassDatabaseField', + 'ParentTeamID', + ); + $actual = array_keys($subteamFields); + sort($expected); + sort($actual); $this->assertEquals( - array( - 'ID', - 'SubclassDatabaseField', - 'ParentTeamID', - ), - array_keys($subteamFields), + $expected, + $actual, 'databaseFields() on subclass contains only fields defined on instance' ); } From ef97164c0a83efee99556ceab84f7d6f1aaa2974 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Thu, 12 Jul 2018 01:09:17 +0100 Subject: [PATCH 2/2] DOCS Update docs to reflect true config merge priorities --- .../04_Configuration/00_Configuration.md | 4 ++-- docs/en/04_Changelogs/4.0.5.md | 12 ++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) create mode 100644 docs/en/04_Changelogs/4.0.5.md diff --git a/docs/en/02_Developer_Guides/04_Configuration/00_Configuration.md b/docs/en/02_Developer_Guides/04_Configuration/00_Configuration.md index 09f940bc0..3d543b2fe 100644 --- a/docs/en/02_Developer_Guides/04_Configuration/00_Configuration.md +++ b/docs/en/02_Developer_Guides/04_Configuration/00_Configuration.md @@ -168,12 +168,12 @@ the result will be the higher priority false-ish value. The locations that configuration values are taken from in highest -> lowest priority order are: -- Any values set via a call to Config#merge / Config#set +- Runtime modifications, ie: any values set via a call to `Config::inst()->update()` - The configuration values taken from the YAML files in `_config/` directories (internally sorted in before / after order, where the item that is latest is highest priority) -- Any static set on an "additional static source" class (such as an extension) named the same as the name of the property - Any static set on the class named the same as the name of the property - The composite configuration value of the parent class of this class +- Any static set on an "additional static source" class (such as an extension) named the same as the name of the property
It is an error to have mixed types of the same named property in different locations. An error will not necessarily diff --git a/docs/en/04_Changelogs/4.0.5.md b/docs/en/04_Changelogs/4.0.5.md new file mode 100644 index 000000000..0fc65d7f7 --- /dev/null +++ b/docs/en/04_Changelogs/4.0.5.md @@ -0,0 +1,12 @@ +# 4.0.5 + +## Notable changes + +Fix [#7971](https://github.com/silverstripe/silverstripe-framework/pull/7971) introduces a subtle change of behaviour +to how `Config` settings are prioritised. In SilverStripe 4 there was a change where `Extension` objects took the +highest importance when determining Config values; this was deemed to be unexpected and unintuitive as well as making it +cumbersome and difficult for developers to override module defaults defined in `Extension`s. This change reverts +behaviour to that of SilverStripe 3 where `Extension` instances are of lowest importance and are used only to set a +default value. If you rely on your `Extension` or module providing an overriding config value, please move this to yaml. + +