thephpleague / tactician-bundle

Bundle to integrate Tactician with Symfony projects
MIT License
245 stars 43 forks source link

RFC: Command class guessing during compilation #60

Closed linaori closed 7 years ago

linaori commented 7 years ago

Currently it's required to pass a command value to the tag in your service configuration, this seems redundant to me. If the chosen strategy supports guessing the method (e.g. handle or __invoke), it can also guess which command it should use as it's defined in the method signature.

During container compilation, the compiler pass knows the class of the handler and it knows the strategy. Based on this (if the tag doesn't define the command), reflection could be used to guess the actual command class and fill it in.

Advantage of this method is that it won't rely on auto-wiring.

Slightly related to #44

rosstuck commented 7 years ago

Hi @iltar, thanks for opening this RFC!

I've thought about this a bit lately as well. My thought is that I wouldn't want to tie this into the __invoke or handle naming style because we don't know if this works (or not) with custom naming conventions. I don't want to exclude those people or add even more complex config arguments.

What we could do, though, is

This could all be enabled/disabled by a single flag on the tag itself (mapping=auto or similar).

This also lets us have our 🍰 and eat it too with autowiring: you can use a single tag with mapping=auto in your autowiring definition, but you're not reliant on autowiring to get this autoscanning feature.

Thoughts? :)

tyx commented 7 years ago

Any idea to support the bus parameter of the tag through these magic solutions ?

linaori commented 7 years ago

I think it's a good step in the right direction!

From what I've seen, all the handlers are pretty much stateless (besides of 1). That means they could be instantiated during the compilation process to finish the map for the ContainerLocator. This would be 100% in line with the configuration option rather than wild guessing.

rosstuck commented 7 years ago

@tyx Since the configuration would go onto the tag, my thought is that if you put a "bus" attribute on there, the auto scanning would be limited to just that bus. If you wanted it differently on another bus, you could specify a different tag.

@iltar Hmm, not sure what the benefit of instantiating them but I might be missing something. My preference is to try and keep the ContainerLocator as lazy as possible for performance reasons.

linaori commented 7 years ago

@rosstuck During the compiler pass the existing strategy can be used to resolve the method to be called upon a specific handler, that's what I mean.

rosstuck commented 7 years ago

Ah gotcha. The existing strategies use the command object instance to guess the method to call, so we'd have to know the command class name up-front (which we don't, unfortunately). It would require either tagging the commands in some way or reflection based on typehints.

linaori commented 7 years ago
services:
    _defaults:
        tags: [tactician.handler]
    Handler\One: ~
    Handler\Two: ~

If this would be possible, I'd be happy!

rosstuck commented 7 years ago

FYI, I've started working on something that would support this (along with a couple other refactors). Still in the "copy/pasting code around" phase, will try to post a POC next week.

linaori commented 7 years ago

Awesome!

kbond commented 7 years ago

We did something similar in simplebus - only works with __invoke though.

rosstuck commented 7 years ago

Quick update, still working on this, refactoring/cleanup is going alright but it's turning out to be more complex than expected since we want to support user defined wiring strategies as well. We'll probably need to move from only having a compiler pass to also having a cache warmer.

rosstuck commented 7 years ago

There's now a work in progress PR for this, #67.

rosstuck commented 7 years ago

Closing this now that #67 is headed for master.