Extensions behave inconsistent

[jkondela]

Affected Version

Tested on current 4.2.2

Description

Extensions behave weird and inconsistent.

After removing extension by calling remove_extension, it will not preserve inherited extensions. Affected method is get_extensions which returns before calling remove_extension all extensions (including inherited), but after removing single extension, it will probably save wrong configuration. The result is only extensions on called class.

I was able to discover this issue by getting error in CMS. Error was that Hierarchy extension is not added to Page. There was also opened issue in CMS repo, but it cannot be reproduced due to not calling remove_extension.

A picture is worth a thousand words:

Steps to Reproduce

For instance try to add extension on Page class (which extends SiteTree) and then remove_extension.

[robbieaverill]

Reproduced on SS 4.3.x-dev, as well as the other extensions that are added in SiteTree::$extensions:

Page::add_extension(FooExtension::class);
Page::has_extension(Hierarchy::class); // boolean true
Page::has_extension(Versioned::class); // boolean true
Page::has_extension(InheritedPermissionsExtension::class); // boolean true

Page::remove_extension(FooExtension::class);
Page::has_extension(Hierarchy::class); // boolean false
Page::has_extension(Versioned::class); // boolean false
Page::has_extension(InheritedPermissionsExtension::class); // boolean false

Wherever possible you should try to apply (and remove) extensions with YAML configuration, which wouldn't be susceptible to this problem

[robbieaverill]

Testing on various versions:

Version	Bug exists
3.7.1	No
4.0.x-dev	Yes
4.1.x-dev	Yes
4.2.x-dev	Yes
4.3.x-dev	Yes

Extensible::add_extension() debugging:

var_dump(Config::inst()->get($class, 'extensions')); // contains all extensions, including those applied to SiteTree
Config::modify()->set($class, 'extensions', $config); // should only be removing one extension
var_dump(Config::inst()->get($class, 'extensions')); // contains no extensions

The bug is in Config::modify()->set($class, 'extensions', $config), which overwrites any inherited extensions aswell somehow.

cc @tractorcow - you might have a better idea of why this is happening

[robbieaverill]

From what I can tell in debugging DeltaConfigCollection and DeltaMiddleware from silverstripe/config, the point at which the delta to remove FooExtension from the config is executed, the class config has been normalised and includes parent extensions as well.

I'm going to make a PR to include inherited extensions when getting the list of extensions to check in Extensible::remove_exension(), which will ensure that the rest of the list of persisted when the one we want to remove is removed.

[robbieaverill]

PR at https://github.com/silverstripe/silverstripe-framework/pull/8624

@tractorcow if you have time it'd be great to get you across this change

[tractorcow]

Left a comment on the PR. I agree the behaviour is inconsistent, but it should be an error, and not us try to support the inconsistency.

[robbieaverill]

Thanks, just the kind of feedback I was looking for!

So the bug here is that the initial checks for whethe Page has the versioned extension are incorrectly reporting true? Isn’t that a dangerous precedent to set? The example code and te test in the PR are both innocently setting and removing an extension to Page, while the broken behaviour is what happens to the other extensions applied to SiteTree when you do things to Page

[tractorcow]

You could extend has_extension(). We already have $strict to toggle between matching subclasses of the extension. You could augment this arg to be a set of flags that also allow matching of extensions on the parent of the class being checked. Or, well, just add another arg. ;P

In any case, the problem isn't with has_extension(), but rather with remove_extension(). I'm not sure that has_extension() being true automatically implies that the extension can be removed, so I wouldn't assume it's a direct relationship.

[robbieaverill]

Yeah - the tests in my PR do test removing parent extensions from subclasses (which I can remove) but the original issue again is this: https://github.com/silverstripe/silverstripe-framework/issues/8613#issuecomment-439403383

[axllent]

Is this solution making and headway? We're still experiencing what appears to be a related issue (#8630) meaning we cannot upgrade beyond 4.2.1.

[tractorcow]

Hi @axllent I've added a note to https://github.com/silverstripe/silverstripe-framework/issues/8630 and re-opened, since I have a feeling these issues are similar but actually different. If we only address this issue my concern is that the other issue may not be addressed either.

[tractorcow]

@robbieaverill done a bit of digging, and I agree that config::set is actually to blame.

Because config is a big chain of middleware, by the time the DeltaConfig middleware is processing the extensions for this class, both parent class extensions and self-class extensions are merged in before applying deltas.

If you want to modify a config on a class without modifying parent classes it can be a problem. The only way to do it safely is with a merge, since set will forcibly replace everything.

The reason inherited configs are being removed is that we are placing the entire inherited config via set, with a config that was generated in a non-inherited way! My bad. :D

In YML, the only way to remove an extension is with a keyed-value, and replacing the value with a null.

E.g.

MyClass:
  extensions:
    someKey: null

The PHP solution is actually exactly the same! In order to remove an extension safely, we MUST key the extension, and remove it with the same key, but via ->merge($class, 'extensions', ['thekey' => null]) and not via ->set

Obviously this is a huge problem, since users haven't got into a habit of adding keyed extensions, and it's not really a convention, more of a suggestion at this point.

So, suggested solution:

• 5.x make keyed extensions mandatory
• 4.x we could add a special config mutation. E.g. Config::modify()->removeArrayValue($class, 'extensions', 'theextension'). This will act kind of like ->merge(['key' => null]) but will only be good to replace a single key in a config by value, instead of key.

Annoying but what do you think @robbieaverill ?

[tractorcow]

    /**
     * Apply a single delta to a class config
     *
     * @param array $config
     * @param array $delta
     * @return array
     */
    protected function applyDelta($config, $delta)
    {
        switch ($delta['type']) {
            case DeltaConfigCollection::SET:
                return array_merge($config, $delta['config']);
+            case DeltaConfigCollection::REMOVE_VALUE:
+                while (isset($config[$delta['name']]) && ($key = array_search($delta['value'], $config[$delta['name']])) !== false) {
+                    unset($config[$delta['name']][$key]);
+                }
+                return $config;
            case DeltaConfigCollection::MERGE:
                return Priority::mergeArray($delta['config'], $config);
            case DeltaConfigCollection::CLEAR:
                return [];
            case DeltaConfigCollection::REPLACE:
                return $delta['config'];
            case DeltaConfigCollection::REMOVE:
                return array_diff_key($config, $delta['config']);
            default:
                throw new InvalidArgumentException("Invalid delta " . $delta['type']);
        }
    }

[tractorcow]

Note, I still think remove_extension() should throw an error / warning if it fails to remove an extension on the class instead of NO-OP.

[robbieaverill]

Will that only work if the extension is defined with a keyed syntax?

[tractorcow]

My code is designed to work if you DON'T use a keyed syntax.

If you use a keyed syntax as standard you can use it without any new config API.

In 5.x you should probably encourage keyed syntax anyway, but maybe support the by value not key removal.

[tristan-mastrodicasa]

Bump

[tristan-mastrodicasa]

we had in app/_config.php (SS4.4):

Page::remove_extension(SomeExtensionWeDidNotWant::class);

This breaks things because the following happens:

• extension is removed (good)
• in removing it, all of the extensions added to SiteTree are also removed (BAD)

The culprit line:

https://github.com/silverstripe/silverstripe-framework/blob/4/src/Core/Extensible.php#L242

$config = Config::inst()->get($class, 'extensions', Config::EXCLUDE_EXTRA_SOURCES | Config::UNINHERITED) ?: [];

When we change the line to:

$config = Config::inst()->get($class, 'extensions', Config::EXCLUDE_EXTRA_SOURCES) ?: [];

it works!

This should be fixed urgently because a seemingly innocuous method breaks basically everything.

The way we fixed this (HACK) was to add the following method to Page.php:

    /**
     * NB. COPY FROM SilverStripe\Core\Extensible
     * @param string $extension class name of an {@link Extension} subclass, without parameters
     */
    public static function remove_extension_fix($extension)
    {
        $class = get_called_class();

        // Build filtered extension list
        $found = false;
        $config = Config::inst()->get($class, 'extensions', Config::EXCLUDE_EXTRA_SOURCES) ?: [];
        foreach ($config as $key => $candidate) {
            // extensions with parameters will be stored in config as ExtensionName("Param").
            if (strcasecmp($candidate, $extension) === 0 ||
                stripos($candidate, $extension . '(') === 0
            ) {
                $found = true;
                unset($config[$key]);
            }
        }
        // Don't dirty cache if no changes
        if (!$found) {
            return;
        }
        Config::modify()->set($class, 'extensions', $config);

        // Unset singletons
        Injector::inst()->unregisterObjects($class);

        // unset some caches
        $subclasses = ClassInfo::subclassesFor($class);
        $subclasses[] = $class;
        if ($subclasses) {
            foreach ($subclasses as $subclass) {
                unset(self::$extra_methods[$subclass]);
            }
        }
    }

[purplespider]

@tractorcow:

In YML, the only way to remove an extension is with a keyed-value, and replacing the value with a null.

E.g.

MyClass:
  extensions:
    someKey: null

So is this the resolution here? To push people to key extensions and deprecate the PHP remove_extension?

As I've just tried this, but it doesn't seem to like null being there:

[Emergency] Uncaught InvalidArgumentException: SilverStripe\CMS\Model\VirtualPage references nonexistent in 'extensions'