From 41dc9229bf6823262bbc4c25edf0da61cb08b260 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Mon, 26 Nov 2018 00:00:02 +0100 Subject: [PATCH] FIX Reverting ExtensionTestState and Extensible extra methods modifications to prevent PHP 5.6 segfault (#8581) * API Revert addition of Extensible::flush_extra_methods_cache() and change to ExtensionTestState This reverts the changes from #8465 and #8505 that relate to ExtensionTestState and the tracking of extra methods between unit tests. The existing test from #8465 testing overloaded Extensions after extra_methods are populated has been updated to show that you must re-add the extension to flush the extra_methods cache if you need this behaviour. * Revert change to InjectorTest::testExtendedExtensions * Revert "Add failing test to show that overloaded extensions are broken in Extensible" This reverts commit 55e79ffdfdd7101825e20fb4585e98ab554bd006. * DOCS Add docs for extending extensions, and upgrade guide note to 4.3 to avoid using PHP config to do so --- .../05_Extending/01_Extensions.md | 50 ++++++++++++++++--- docs/en/04_Changelogs/4.3.0.md | 11 +++- src/Core/Extensible.php | 9 ---- src/Dev/State/ExtensionTestState.php | 40 ++++++++++++++- tests/php/Core/Injector/InjectorTest.php | 25 +--------- .../InjectorTest/SomeCustomisedExtension.php | 13 ----- .../Injector/InjectorTest/SomeExtension.php | 14 ------ 7 files changed, 93 insertions(+), 69 deletions(-) delete mode 100644 tests/php/Core/Injector/InjectorTest/SomeCustomisedExtension.php delete mode 100644 tests/php/Core/Injector/InjectorTest/SomeExtension.php diff --git a/docs/en/02_Developer_Guides/05_Extending/01_Extensions.md b/docs/en/02_Developer_Guides/05_Extending/01_Extensions.md index cc00ecf0b..70e9a5063 100644 --- a/docs/en/02_Developer_Guides/05_Extending/01_Extensions.md +++ b/docs/en/02_Developer_Guides/05_Extending/01_Extensions.md @@ -56,7 +56,7 @@ Alternatively, we can add extensions through PHP code (in the `_config.php` file ```php -SilverStripe\Security\Member::add_extension('MyMemberExtension'); +SilverStripe\Security\Member::add_extension(MyMemberExtension::class); ``` This class now defines a `MyMemberExtension` that applies to all `Member` instances on the website. It will have @@ -256,7 +256,7 @@ $member = Security::getCurrentUser(); print_r($member->getExtensionInstances()); -if($member->hasExtension('MyCustomMemberExtension')) { +if ($member->hasExtension(MyCustomMemberExtension::class)) { // .. } ``` @@ -282,7 +282,7 @@ if not specified in `self::$defaults`, but before extensions have been called: public function __construct() { $this->beforeExtending('populateDefaults', function() { - if(empty($this->MyField)) { + if (empty($this->MyField)) { $this->MyField = 'Value we want as a default if not specified in $defaults, but set before extensions'; } }); @@ -301,9 +301,9 @@ This method is preferred to disabling, enabling, and calling field extensions ma ```php public function getCMSFields() { - $this->beforeUpdateCMSFields(function($fields) { + $this->beforeUpdateCMSFields(function ($fields) { // Include field which must be present when updateCMSFields is called on extensions - $fields->addFieldToTab("Root.Main", new TextField('Detail', 'Details', null, 255)); + $fields->addFieldToTab('Root.Main', new TextField('Detail', 'Details', null, 255)); }); $fields = parent::getCMSFields(); @@ -312,9 +312,45 @@ public function getCMSFields() } ``` -## Related Lessons -* [DataExtensions and SiteConfig](https://www.silverstripe.org/learn/lessons/v4/data-extensions-and-siteconfig-1) +## Extending extensions {#extendingextensions} +Extension classes can be overloaded using the Injector, if you want to modify the way that an extension in one of +your modules works: + +```yaml +SilverStripe\Core\Injector\Injector: + Company\Vendor\SomeExtension: + class: App\Project\CustomisedSomeExtension +``` + +**app/src/CustomisedSomeExtension.php** + +```php +namespace App\Project; + +use Company\Vendor\SomeExtension; + +class CustomisedSomeExtension extends SomeExtension +{ + public function someMethod() + { + $result = parent::someMethod(); + // modify result; + return $result; + } +} +``` + +
+Please note that modifications such as this should be done in YAML configuration only. It is not recommended +to use `Config::modify()->set()` to adjust the implementation class name of an extension after the configuration +manifest has been loaded, and may not work consistently due to the "extra methods" cache having already been +populated. +
+ +## Related Lessons + +* [DataExtensions and SiteConfig](https://www.silverstripe.org/learn/lessons/v4/data-extensions-and-siteconfig-1) ## Related Documentaion diff --git a/docs/en/04_Changelogs/4.3.0.md b/docs/en/04_Changelogs/4.3.0.md index f89db9640..ab31d003e 100644 --- a/docs/en/04_Changelogs/4.3.0.md +++ b/docs/en/04_Changelogs/4.3.0.md @@ -26,7 +26,7 @@ To enable the legacy search API on a `GridFieldFilterHeader`, you can either: * set the `useLegacyFilterHeader` property to `true`, * or pass `true` to the first argument of its constructor. -To force the legacy search API on all instances of `GridFieldFilterHeader`, you can set it in your [configuration file](../../configuration): +To force the legacy search API on all instances of `GridFieldFilterHeader`, you can set it in your [configuration file](../developer_guides/configuration): ```yml SilverStripe\Forms\GridField\GridFieldFilterHeader: force_legacy: true @@ -73,3 +73,12 @@ SilverStripe\Core\Injector\Injector: For information on how to implement the history viewer UI in your own versioned DataObjects, please refer to [the Versioning documentation](../developer_guides/model/versioning). + +### Tests with dynamic extension customisations + +In SilverStripe 4.2, some unit tests that modify an extension class with PHP configuration manifest customisations +may have passed and may now fail in SilverStripe 4.3. This behaviour is inconsistent, is not a recommended approach +to customising extensions and should be avoided in all SilverStripe 4.x releases. + +For information on how to customise extensions, see +["Extending Extensions"](../developer_guides/extending/extensions#extendingextensions). diff --git a/src/Core/Extensible.php b/src/Core/Extensible.php index b29dda32e..79fe536b6 100644 --- a/src/Core/Extensible.php +++ b/src/Core/Extensible.php @@ -215,15 +215,6 @@ trait Extensible return true; } - /** - * Clears all cached extra_methods cache data - */ - public static function flush_extra_methods_cache() - { - self::$extra_methods = []; - } - - /** * Remove an extension from a class. * Note: This will not remove extensions from parent classes, and must be called diff --git a/src/Dev/State/ExtensionTestState.php b/src/Dev/State/ExtensionTestState.php index 2cfedddbd..29c4dd09c 100644 --- a/src/Dev/State/ExtensionTestState.php +++ b/src/Dev/State/ExtensionTestState.php @@ -13,6 +13,16 @@ use SilverStripe\ORM\DataObject; */ class ExtensionTestState implements TestState { + /** + * @var array + */ + protected $extensionsToReapply = []; + + /** + * @var array + */ + protected $extensionsToRemove = []; + /** * Called on setup * @@ -20,7 +30,6 @@ class ExtensionTestState implements TestState */ public function setUp(SapphireTest $test) { - DataObject::flush_extra_methods_cache(); } public function tearDown(SapphireTest $test) @@ -31,6 +40,8 @@ class ExtensionTestState implements TestState { // May be altered by another class $isAltered = false; + $this->extensionsToReapply = []; + $this->extensionsToRemove = []; /** @var string|SapphireTest $class */ /** @var string|DataObject $dataClass */ @@ -46,6 +57,10 @@ class ExtensionTestState implements TestState if (!class_exists($extension) || !$dataClass::has_extension($extension)) { continue; } + if (!isset($this->extensionsToReapply[$dataClass])) { + $this->extensionsToReapply[$dataClass] = []; + } + $this->extensionsToReapply[$dataClass][] = $extension; $dataClass::remove_extension($extension); $isAltered = true; } @@ -62,6 +77,10 @@ class ExtensionTestState implements TestState throw new LogicException("Test {$class} requires extension {$extension} which doesn't exist"); } if (!$dataClass::has_extension($extension)) { + if (!isset($this->extensionsToRemove[$dataClass])) { + $this->extensionsToRemove[$dataClass] = []; + } + $this->extensionsToRemove[$dataClass][] = $extension; $dataClass::add_extension($extension); $isAltered = true; } @@ -85,6 +104,23 @@ class ExtensionTestState implements TestState public function tearDownOnce($class) { - DataObject::flush_extra_methods_cache(); + // @todo: This isn't strictly necessary to restore extensions, but only to ensure that + // Object::$extra_methods is properly flushed. This should be replaced with a simple + // flush mechanism for each $class. + /** @var string|DataObject $dataClass */ + + // Remove extensions added for testing + foreach ($this->extensionsToRemove as $dataClass => $extensions) { + foreach ($extensions as $extension) { + $dataClass::remove_extension($extension); + } + } + + // Reapply ones removed + foreach ($this->extensionsToReapply as $dataClass => $extensions) { + foreach ($extensions as $extension) { + $dataClass::add_extension($extension); + } + } } } diff --git a/tests/php/Core/Injector/InjectorTest.php b/tests/php/Core/Injector/InjectorTest.php index b1dee54db..1e54cc1de 100644 --- a/tests/php/Core/Injector/InjectorTest.php +++ b/tests/php/Core/Injector/InjectorTest.php @@ -21,13 +21,12 @@ use SilverStripe\Core\Tests\Injector\InjectorTest\NeedsBothCirculars; use SilverStripe\Core\Tests\Injector\InjectorTest\NewRequirementsBackend; use SilverStripe\Core\Tests\Injector\InjectorTest\OriginalRequirementsBackend; use SilverStripe\Core\Tests\Injector\InjectorTest\OtherTestObject; -use SilverStripe\Core\Tests\Injector\InjectorTest\SomeCustomisedExtension; -use SilverStripe\Core\Tests\Injector\InjectorTest\SomeExtension; use SilverStripe\Core\Tests\Injector\InjectorTest\TestObject; use SilverStripe\Core\Tests\Injector\InjectorTest\TestSetterInjections; use SilverStripe\Core\Tests\Injector\InjectorTest\TestStaticInjections; use SilverStripe\Dev\SapphireTest; -use SilverStripe\Security\Member; +use SilverStripe\Dev\TestOnly; +use stdClass; define('TEST_SERVICES', __DIR__ . '/AopProxyServiceTest'); @@ -1048,24 +1047,4 @@ class InjectorTest extends SapphireTest Injector::unnest(); $this->nestingLevel--; } - - /** - * Tests that overloaded extensions work, see {@link Extensible::getExtensionInstance()} - */ - public function testExtendedExtensions() - { - Config::modify() - ->set(Injector::class, SomeExtension::class, [ - 'class' => SomeCustomisedExtension::class, - ]) - ->merge(Member::class, 'extensions', [ - SomeExtension::class, - ]); - - /** @var Member|SomeExtension $member */ - $member = new Member(); - $this->assertTrue($member->hasExtension(SomeExtension::class)); - $this->assertTrue($member->hasMethod('someMethod')); - $this->assertSame('bar', $member->someMethod()); - } } diff --git a/tests/php/Core/Injector/InjectorTest/SomeCustomisedExtension.php b/tests/php/Core/Injector/InjectorTest/SomeCustomisedExtension.php deleted file mode 100644 index 9641d7df1..000000000 --- a/tests/php/Core/Injector/InjectorTest/SomeCustomisedExtension.php +++ /dev/null @@ -1,13 +0,0 @@ -