From 4acec33562e4e1230092eee7d76c2b8061ffc914 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 7aaf739a5..dbc46f090 100644 --- a/tests/php/ORM/DataObjectTest.php +++ b/tests/php/ORM/DataObjectTest.php @@ -1062,72 +1062,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 d80ef3d9e6fc211669b6b93bec9c166d23bb1c74 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 e0ad2520b..7eac928d9 100644 --- a/docs/en/02_Developer_Guides/04_Configuration/00_Configuration.md +++ b/docs/en/02_Developer_Guides/04_Configuration/00_Configuration.md @@ -153,12 +153,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. + +