Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swap arguments order in ServiceProviderInterface::getExtension callables? #50

Open
romm opened this issue Apr 12, 2020 · 2 comments
Open

Comments

@romm
Copy link

romm commented Apr 12, 2020

It seems to me that the order of arguments given in ServiceProviderInterface::getExtension callables would make more sense if they were swapped.

I've been using this kind of extensions quite often:

public function getExtensions(): array
{
    return [
        MyServiceInterface::class => static function (ContainerInterface $container, MyServiceInterface $myService): MyServiceInterface {
            return new AggregateOfMyService(
                $myService,
                new SomeOtherImplementationOfMyService()
            );
        },
    ];
}

In this example, the instance of ContainerInterface is not used, leading to an unused variable and making static analysis tools (including IDE) complain about it (which is right IMO).

When I think about it, I believe that in extensions, the former value of the service should always be present in parameters (even if it's null) because the callback should extend this value, whereas the container is not always needed (see example above).

What about changing the callable signature to:

function(?$previous, ContainerInterface $container) : mixed

Some would maybe argue that this should be a valid use-case:

public function getExtensions(): array
{
    return [
        MyServiceInterface::class => static function (ContainerInterface $container, ?MyServiceInterface $myService = null): MyServiceInterface {
            return new SomeOtherImplementationOfMyService(
                $container->get('some_dependency')
            );
        },
    ];
}

But I don't agree: in this case, a factory should be prefered over an extension.

WDYT?

@antonpresn
Copy link

Also, it's quite hard to implement (and maybe even impossible) with concrete container (such as https://php-di.org/), which uses decoration parameters as suggested here (when decorated value goes first)

@mindplay-dk
Copy link
Collaborator

When I think about it, I believe that in extensions, the former value of the service should always be present in parameters (even if it's null) because the callback should extend this value, whereas the container is not always needed (see example above).

My pattern-loving programmer brain screams "NOOOOO", but I do see the point.

Technically, the factory and extension callables are unrelated types, so although consistency would feel good here, it's not technically required, I think.

Also, it's quite hard to implement (and maybe even impossible) with concrete container (such as https://php-di.org/), which uses decoration parameters as suggested here (when decorated value goes first)

@antonpresn I don't know what is meant by "decoration parameters" here? Why would the signature of callable types in a PSR service provider affect the concrete container?

I'm leaving this issue open for discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants