Remove AbstractPluginManager#validate

[boesing]

RFC

Q	A
Proposed Version(s)	4.0.0
BC Break?	Yes

Goal

Due to the current implementation, implementors of the AbstractPluginManager can easily "oversee" that they have to provide $instanceOf property to provide any validation of their plugin manager. This leads to implementations with no validation at all:

https://psalm.dev/r/d12af359e2

Background

Even tho, the current implementation within AbstractPluginManager also allows adding complex generics to their plugin manager without adding any validation. Might be even the case that implementors think that laminas got their back do the fact that there is already an AbstractPluginManager#validate.

Given the example of https://psalm.dev/r/d12af359e2, it is quite easy to implement a PluginManager which can return mixed while stating to only return int|string.

Considerations

Even tho, that in laminas-servicemanager v3 the validatePlugin method was removed (which had to be implemented in implementing classes) - for convenience - I'd require it to be implemented again with v4 while providing a trait which can be implemented in case that a PluginManager did not need more than that.
In order to have more type-stability, this change would be mandatory tho.

Proposal(s)

• Remove AbstractPluginManager#validate and AbstractPluginManager#$instanceOf
• Provide an SingleInstanceOfPluginManagerTrait which provides that functionality so it can be re-implemented conveniently

Appendix

/** 
 * @template InstanceType of object
 * @psalm-require-implements PluginManagerInterface
 */
trait Foo
{
    /** @var class-string<InstanceType> */
    protected string $instanceOf;
    
    /** @psalm-assert InstanceType $instance */
    public function validate(mixed $instance): void
    {
        if($instance instanceOf $this->instanceOf) {
            return;
        }
        throw new InvalidArgumentException('Not the expected instance of...');
    }
}

/** 
 * @implements PluginManagerInterface<stdClass> 
 */
final class Bar implements PluginManagerInterface
{
    /** @use Foo<stdClass> */
    use Foo;
    
    protected string $instanceOf = stdClass::class;
}

https://psalm.dev/r/9a75a42fda

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/d12af359e2

<?php

namespace Laminas {
    /**
     * @template InstanceType
     */
    interface PluginManagerInterface
    {
        /**
         * @psalm-assert InstanceType $instance
         * @throws \InvalidArgumentException In case the value does not match expectations.
         */
        public function validate(mixed $instance): void;
        
        /** @return InstanceType */
        public function get(string $id): mixed;
    }

    /**
     * @template InstanceType
     * @template-implements PluginManagerInterface<InstanceType>
     */
    abstract class AbstractPluginManager implements PluginManagerInterface
    {
        /**
         * @var class-string<InstanceType>|null
         */
        protected string|null $instanceOf = null;
        
        /**
         * @psalm-assert InstanceType $instance
         */
        public function validate(mixed $instance): void
        {
            if ($this->instanceOf === null) {
                return;
            }

            if ($instance instanceOf $this->instanceOf) {
                return;
            }

            throw new \InvalidArgumentException('booyah');
        }
        
        /**
         * @return InstanceType
         */
        public function get(string $id): mixed
        {
            /** @var mixed $instance */
        	$instance = null; // instance from factory or whatever
            $this->validate($instance);
            
            return $instance;
        }
    }
}

namespace UpstreamProject {
    use Laminas\AbstractPluginManager;
    
    /**
     * @extends AbstractPluginManager<int|string>
     */
    final class MyWhateverPluginManager extends AbstractPluginManager
    {
    }
    
    $plugins = new MyWhateverPluginManager();
    
    /** @psalm-trace $plugin */
    $plugin = $plugins->get('foo');
}

Psalm output (using commit c82191b):

INFO: Trace - 73:5 - $plugin: int|string

https://psalm.dev/r/9a75a42fda

<?php

/** @template InstanceType */
interface PluginManagerInterface
{}

/** 
 * @template InstanceType of object
 * @psalm-require-implements PluginManagerInterface
 */
trait Foo
{
    /** @var class-string<InstanceType> */
    protected string $instanceOf;
    
    /** @psalm-assert InstanceType $instance */
    public function validate(mixed $instance): void
    {
        
    }
}

/** 
 * @implements PluginManagerInterface<stdClass> 
 */
final class Bar implements PluginManagerInterface
{
    /** @use Foo<stdClass> */
    use Foo;
    
    protected string $instanceOf = stdClass::class;
}

Psalm output (using commit c82191b):

No issues!

[Ocramius]

Few things:

1. OK to remove validate() and $instanceOf
2. I really don't want traits to be added: they are the squishiest and hardest to maintain concepts in PHP.
3. we can implement (and we discussed it already) validation with type inference:

/** @template TItem */
final class ValidatedPluginManager {
    /** @param callable(mixed): TItem $validator */
    public function __construct(
        private readonly ContainerInterface $c,
        private readonly $validator
    ) {
    }

    /** @return TItem */
    function get(string $name): mixed {
        return ($this->validator)($this->c->get($name));
    }
}

Here's an example of the inference working: https://psalm.dev/r/be1a96c20b

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/be1a96c20b

<?php

interface ContainerInterface { function get(string $name): mixed; }

/** @template TItem */
final class ValidatedPluginManager {
    /** @param callable(mixed): TItem $validator */
    public function __construct(
        private readonly ContainerInterface $c,
        private readonly $validator
    ) {
    }

    /** @return TItem */
    function get(string $name): mixed {
        return ($this->validator)($this->c->get($name));
    }
}

interface SomethingA {}

/** @var ContainerInterface $c */

$pm = new ValidatedPluginManager($c, function (mixed $v) { assert($v instanceof SomethingA); return $v; });

$service = $pm->get('stuff');

/** @psalm-trace $service */

return $service;

Psalm output (using commit c82191b):

INFO: Trace - 30:1 - $service: SomethingA

[boesing]

I already mentioned else where that this is not how pluginmanagers are used. They are fetched from the container and not manually instantiated in upstream.

Can you please provide an example on how to use that ValidatedPluginManager while fetching it from the container?

[Ocramius]

I would still go with the validator approach then:

/** @template-extends BasePluginManager<Foo> */
class MyPluginManager extends BasePluginManager { // yes, this is still better than a trait, to have different class names
    public function __construct($c) {
        parent::__construct(
            $c,
            function (mixed $v): Foo {
                assert($v instanceof Foo);
                return $v;
            }
        );
    }
}

[Xerkus]

Injected validation callback adds complexity for no reason. Concrete plugin managers do not change what is allowed at runtime. They can implement method directly just fine.

[Ocramius]

To be crystal-clear, I'm not looking forward to maintaining compatibility with the trait: the trait is an absolute no-go from a design perspective.

[Ocramius]

Fine by me with remaining with $instanceOf = class-string, if you believe that's simpler, but then we need to remove final and put it on all non-private methods

[boesing]

The trait can be added to the documentation as a BC layer. I dont have a hard need for it to be shipped. But I also dont see hilarious maintenance effort here tbh, its one method and a property.

[Ocramius]

If all we do here is removing #validate() then it's fine: we don't need to push it further.

[boesing]

The method is required by the Interface and with this proposal has to be added to the various plugin managers out there.

I do not want to remove it, just want to drop the flawed implementation in the abstract plugin manager.

[Ocramius]

I don't understand this issue then, sorry 😓

I suggest sending a patch directly, so perhaps it's clearer there?

[boesing]

Can we outline the exact thing you are not understanding?
This is an RFC, I just want to get some feedback on:

Are we fine with removing the concrete method implementation for PluginManagerInterface#validate in AbstractPluginManager so that all the plugin managers out there have to implement that method on their own?

For all those plugin managers out there which do only provide a single instance of a specific interface, I wanted to add either a trait or an AbstractSingleInstancePluginManager which implements the validate method in the way it exists as of now (without the nullable property $instanceOf tho). That would reduce the amount of migration effort in those projects.

So once one of those plugin managers have to get migrated:

• either use the trait (as the $instanceOf property most likely is already configured) or extend the AbstractSingleInstancePluginManager if we want to have that rather than a trait
• let them implement an empty validate method as they return mixed and thus do not need any validation

[Xerkus]

What if... validate() was dropped entirely?

class ViewHelperPluginManager extends AbstractPluginManager implements ViewHelperPluginManagerInterface
{
    public function get(string $id): ViewHelperPlugin
    {
        return parent::get($id);
    }
}

[boesing]

Would work from a raw php perspective but psalm will tell you that mixed might be returned. the validate is to ensure that the return typ fits expectations for static analysis without the need to execute and handle runtime issues.

[boesing]

I've created that AbstractSingleInstancePluginManager in #179 and moved the validate method from the AbstractPluginManager to that abstraction.

Migration path would be to replace AbstractPluginManager with AbstractSingleInstancePluginManager in case the plugin manager is also setting $instanceOf property to a class-string.

Every other plugin manager (i.e. those which do return callables) have to implement an own validate method and either do nothing or somehow verify that the callable is matching the expectations. For initial migration that would be simply to keep it empty as it reflects servicemanager v3 behavior which did nothing once $instanceOf was null.

[boesing]

Closed by #179