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

* DOCS Add docs for extending extensions, and upgrade guide note to 4.3 to avoid using PHP config to do so

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;
+    }
+}
+```
+
+<div class="notice" markdown="1">
+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.
+</div>
+
+## Related Lessons
+
+* [DataExtensions and SiteConfig](https://www.silverstripe.org/learn/lessons/v4/data-extensions-and-siteconfig-1)
 
 ## Related Documentaion
 

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

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

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);
+            }
+        }
     }
 }

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());
-    }
 }

tests/php/Core/Injector/InjectorTest/SomeCustomisedExtension.php

@@ -1,13 +0,0 @@
-<?php
-
-namespace SilverStripe\Core\Tests\Injector\InjectorTest;
-
-use SilverStripe\Dev\TestOnly;
-
-class SomeCustomisedExtension extends SomeExtension implements TestOnly
-{
-    public function someMethod()
-    {
-        return 'bar';
-    }
-}

tests/php/Core/Injector/InjectorTest/SomeExtension.php

@@ -1,14 +0,0 @@
-<?php
-
-namespace SilverStripe\Core\Tests\Injector\InjectorTest;
-
-use SilverStripe\Dev\TestOnly;
-use SilverStripe\ORM\DataExtension;
-
-class SomeExtension extends DataExtension implements TestOnly
-{
-    public function someMethod()
-    {
-        return 'foo';
-    }
-}