Remove ServiceManager inheritance for AbstractPluginManager

[boesing]

Q	A
Documentation	no
BC Break	yes

Description

This is a PoC of how the plugin manager could work without inheriting from the ServiceManager.
With this PoC, I've also modified ServiceManager to return mixed as thats actually the case already. Nothing in this component verifies that something is object. We do have soft-types for factories to only return object but in fact, they can return mixed anyways.
Since the AbstractPluginManager does not require an $instanceOf value and thus, can also return mixed in case it is not set, I think this change makes sense until we decide to go the same route as symfony does. Since that decision has not yet been made, I'd prefer to skip that. I'm open to create an RFC for this and bring it to the next TSC if some1 wants to further discuss this.

Please NOTE that I also:

• removed some useless tests which verified exceptions for passing invalid value sto either ServiceManager and/or AbstractPluginManager
• added plenty of type-hints (even to interfaces) - but not to every interface (yet) - will definitely do that before finally creating a v4 (@samsonasik would you mind updating the laminas/laminas-servicemanager-migration rector rules once we finished with adding all the missing types?)
• slightly modified the ServiceManagerConfigurationType (still compatible but more strict)
• with a psalm version containing this patch, we can provide even better psalm-types regarding callable-objects and class-string

TODOS:

• Modify container-config-test repository so that some tests are being dropped
• Mark ConfigInterface and Config as deprecated in v3
• Add psalm-types from ConfigInterface to ServiceManager in a redundant way (in v3) so that upstream projects can already switch
• Mark ServiceManager as @final in v3
• Get Introduce callable object intersection vimeo/psalm#9599 merged so that we can provide even better psalm-types for class-string<callable(ContainerInterface,string,array|null):mixed>

[boesing]

Moved this test to the ServiceManager specific tests as this is used for both AbstractPluginManager AND ServiceManager.

I could've checked the container instance and marked as skipped but since there is already a dedicated test class, I moved it.

[samsonasik]

@boesing ok , I can do that 👍 , I may need summary which part that need update

[boesing]

Partly solves #44

[Ocramius]

My only major problem with this patch is the addition of #configure() on interfaces that are designed for container consumption, rather than configuration.

Configuration of a container can happen on concrete instances, while the ContainerInterface, ServiceLocatorInterface and PluginManagerInterface interfaces should be read-only, without any assumption that they might be configurable.

Heck, they may be dumped/generated code.

[Xerkus]

configure() is/was part of mutable container. I remember there was a push to make container immutable and configurable at creation. It got ultimately dropped unmerged or reverted before release.

Its use case might have been related to how container or various plugin managers were configured after creation in module manager/mvc events.

I am for making it internal implementation detail and removing from public interface.

[Xerkus]

configure() is indeed mvc late configuration related. Context could be found here zendframework/zend-servicemanager#47

[boesing]

@Xerkus @Ocramius I'd be up for a read-only container with v5. Given the fact that v4 to the end of Q1 is already tough with all the migration paths, I'd prefer not being too complex here.

Keeping the old methods is mostly due to BC compat but I am fine with dropping configure from the Interface. WDYT?

[Ocramius]

Dropping from the interface(s) is sufficient for me, for now 👍

[boesing]

• needs to be switched back once psalm releases the next minor

[weierophinney]

Overall,I like this approach. Composing a ServiceManager inside the AbstractPluginManager removes a lot of duplication, while simultaneously removing the inheritance. Having an interface means we can use that as the type instead of an abstract class, which also makes me happy. And the psalm shapes in the ServiceManager provide valuable documentation of expectations.

My only concern/issue is that I don't recall the the rationale for deprecating/removing the ConfigInterface and Config class. I recognize that we can use Psalm annotations to provide shapes for valid data, but that seems more likely to lead to issues at runtime where the shapes cannot be verified/validated. That said, I won't block approval on that; just wish I could remember what the arguments were around this.

[weierophinney]

Since ServiceLocatorInterface already extends ContainerInterface, should these really be defined here?

I can understand defining get() so we can add the additional annotations, but at the very least, has() feels superfluous.

[boesing]

Yah, has is superfluous indeed.
I guess I've added it to also add some psalm-assert on it to provide information of what will be returned from get. But since has actually does no validation at all, it would provide false information and thus we can safely drop it here.

[boesing]

 [...] rationale for deprecating/removing the ConfigInterface and Config [...]

I don't see any value for that interface or the Config. The interface itself was not used anywhere and the Config was almost always empty.
We (in my company) configured our project container like this in mezzio:

container.php

<?php

use Laminas\ServiceManager\Config;
use Laminas\ServiceManager\ServiceManager;

require_once __DIR__ . '/../vendor/autoload.php';

// Load configuration
$config = require __DIR__ . '/config.php';

// Build container
$container = new ServiceManager();
(new Config($config['dependencies']))->configureServiceManager($container);

// Inject config
$container->setService('Config', $config);

// Make container writable since we want to overwrite even existing alias for config
$container->setAllowOverride(true);
$container->setAlias('config', 'Config');
$container->setAllowOverride(false);

return $container;

The laminas-modulemanager seems to provide support for the ConfigInterface and maybe some projects out there might have used that. I guess that was to avoid having to provide the array-structure from Module#getConfig. But since that is the usual way of how it is returned from ConfigProvider#__invoke in mezzio applications, the plan for laminas-mvc v4 is to use the config-aggregator for modules as well (hopefully, that would be huge, can be even done as of now by using laminas/laminas-config-aggregator-modulemanager) and due to the fact that we have psalm-types for a period of time now gives me the feeling that the ConfigInterface along with its implementation is adding unnecessary code-paths to applications (as in the end, we end up having an array anyways which is passed to ServiceManager#configure or AbstractPluginManager#configure.

So its just a dangling class mostly used to have a value-object of the config which wasnt properly validated at runtime and didn't provide any type until 3.9.0 which was 11 years after the initial creation of the interface.

---

Thats why I think we can safely drop it and let upstream projects actually return the array structure while using psalm + psalm-type to have proper type validation:

before

<?php

namespace MyModule;

use Laminas\ModuleManager\Feature\ServiceProviderInterface;
use Laminas\ServiceManager\Config;
use Laminas\ServiceManager\Factory\InvokableFactory;

final class Module implements ServiceProviderInterface
{

    public function getServiceConfig(): Config
    {
        return new Config([
            'factories' => [
                MyModuleService::class => InvokableFactory::class,
            ],
        ]);
    }
}

after

<?php

namespace MyModule;

use Laminas\ModuleManager\Feature\ServiceProviderInterface;
use Laminas\ServiceManager\Factory\InvokableFactory;
use Laminas\ServiceManager\ServiceManager;

/** @psalm-import-type ServiceManagerConfiguration from ServiceManager */
final class Module implements ServiceProviderInterface
{

    /**
     * @return ServiceManagerConfiguration
     */
    public function getServiceConfig(): array
    {
        return [
            'factories' => [
                MyModuleService::class => InvokableFactory::class,
            ],
        ];
    }
}

---

However, I am open to keep the Config object if we prefer object > array but then I would actually prefer the config being injected as an object to the SM/PluginManager instead of allowing arrays. But then we have ConfigInterface#toArray again which does not really change anything.

Should we create an RFC for this? I can restore the ConfigInterface + Config here for now.
But we would have to rename ConfigInterface#configureServiceManager to ConfigInterface#configure and then allow both ServiceManager + AbstractPluginManager due to the removal of inheritance here.

[weierophinney]

@boesing Thanks for that summary!

When I consider that the majority of the time, we're passing an array to either the ServiceManager or a plugin manager, and that it has to do validations internally anyways due to this, it doesn't make sense to keep it. The only reason to keep it is if the SM and/or PM required an instance, and pulled configuration from it, as it would then provide validations. That would just be shifting responsibilities, and would introduce duplications regardless, as we have the various set*() methods, and they have to do validations, too.

[boesing]

@boesing ok , I can do that 👍 , I may need summary which part that need update

@samsonasik I guess I've finished the work here, so I can provide a lil summary:

1. all classes extending AbstractPluginManager should either
   1.1 extend AbstractSingleInstancePluginManager in case the $instanceOf property is declared and should adapt $instanceOf value in @template-extends annotation
   1.2 implement its own validate method (can be created as an empty method from rector in case it is missing)
2. We could search for classes implementing the modulemanager ServiceProviderInterface#getServiceConfig and in case of a return value of Config it should return array (basically the __construct($arg) which is being passed to the Config. Refer to Remove ServiceManager inheritance for AbstractPluginManager #179 (comment) to see how it needs to be replaced.
3. adding property types of AbstractPluginManager/ AbstractSingleInstancePluginManager (not sure if rector can modify code which is not valid as per linting due to mismatching property types?)
4. container.php as in Remove ServiceManager inheritance for AbstractPluginManager #179 (comment) can be modified to $container = new ServiceManager($config['dependencies']); instead

Not 100% sure if that is already everything but I guess at least for this PR that should reflect almost all changes.

[samsonasik]

@boesing Thank you for the summary, I will try what I can do 👍

[weierophinney]

🚢