container-interop / definition-interop

[EXPERIMENTAL] Promoting container interoperability through standard definitions
MIT License
19 stars 2 forks source link

Should references resolving be recursive? #18

Closed Anahkiasen closed 8 years ago

Anahkiasen commented 8 years ago

This is a bit of an odd question but it's something I wondered when writing definitions. Consider Tactician as example, you create an instance of it by passing it an array of middlewares.

Now with the implementations we have, if you use Assembly per example, you would be unable to do this:

class TacticianDefinition implement DefinitionProviderInterface
{
    public function getDefinitions()
    {
        $bus = new ObjectDefinition(CommandBus::class, CommandBus::class);
        $bus->setConstructorArguments([new Reference(Middleware::class)]);

        return [
            Middleware::class => new CommandHandlerMiddleware(...),
            CommandBus::class => $bus,
        ];
    }
}

Because it would see just an array and wouldn't look into it for possible references to resolve.

Now my question is should it? To me the current behavior makes sense, because doing a recursive resolving of references could a) be a serious performance hit b) cause unforeseen errors. But in this case, should we possibly update the getArguments docblock (or something) to specify it? I know I tripped on it at first and likely won't be the only one.

moufmouf commented 8 years ago

Hey @Anahkiasen ,

I'm not sure I understand... It seems to me the code you propose above should be ok. It does not work in Assembly? We might link this issue to #19 that proposes a set of common tests for containers implementing definition-interop.

Anahkiasen commented 8 years ago

It does not work in Assembly?

No because to resolve references it just does an instanceof ReferenceInterface check, which the array would fail, even if it contains a ReferenceInterface

mnapoli commented 8 years ago

I think the problematic line here is:

$bus->setConstructorArguments([new Reference(Middleware::class)]);

It's not a reference, it's an array containing a reference. This problem can be solved using a factory btw, but I see why you raise the point. I've been there in PHP-DI and crawling arrays recursively to resolve such nested references is trouble… I'd rather stay of up that.

moufmouf commented 8 years ago

Ha ok. I'd really go the other way around. It is a powerful feature, and it is actually not that difficult to implement. I've done that in Yaco, it is only a few lines of code.

I'll submit a PR to Assembly now.

moufmouf commented 8 years ago

Recursive reference support pushed here: https://github.com/mnapoli/assembly/pull/13

mnapoli commented 8 years ago

OK after giving it some time I don't have compelling arguments to why not, so let's do it! Thanks @moufmouf I'll merge the PR.

moufmouf commented 8 years ago

:+1: