thephpleague / tactician-bundle

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

Create CommandHandlerInterface for autowiring-configure of CommandHandlers in Symfony #104

Closed mnavarrocarter closed 4 years ago

mnavarrocarter commented 6 years ago

Hey! I love this bundle, and I'm starting to discover the power of the command bus pattern. It would be really cool to include a CommandHandlerInterface whose sole purpose is going to be signaling a concrete command handler for autowiring and autoconfigure. I've implemented this in my project manually, but I think it could be a very neat feature to have! :)

Najki commented 6 years ago

That would be a great feature!

rosstuck commented 6 years ago

Hi folks :) This topic has come up a few times before and the general thought from my side is that Tactician is meant to be used behind a project specific bridge interface (so you can tweak/replace it easily if you want). Having a direct interface from Tactician itself would break that a bit, so I'd prefer to see folks use their own Handler interface (if that's what they want) and wire it up that way.

If someone would like to contribute docs explaining how to do this as a new section in the README, I'd be happy to merge those though!

Najki commented 6 years ago

Hi folks :) This topic has come up a few times before and the general thought from my side is that Tactician is meant to be used behind a project specific bridge interface (so you can tweak/replace it easily if you want). Having a direct interface from Tactician itself would break that a bit, so I'd prefer to see folks use their own Handler interface (if that's what they want) and wire it up that way.

From my standpoint, TacticianBundle is this bridge which allows to ease and speed up implementation of Tactician in Symfony-based projects. It already introduces a few implementation methods (eg. strict or by using typehints) so an autowiring interface could be an another method of speeding up implementation of this bundle and widening its user base.

If someone would like to contribute docs explaining how to do this as a new section in the README, I'd be happy to merge those though!

And if someone (maybe @mnavarrocarter as I understand that he already has a working solution) would create a pull request with the mentioned interface? :wink:

rosstuck commented 6 years ago

The role of the bundle is to ease integration into the Symfony side of things but not to encourage direct coupling within your application.

Existing methods like autowiring and direct tagging all work externally to your code, they're limited to outside introspection. Adding an interface is not and from what I've seen in earlier experiments, most users will not realize it's optional.

Considering the wiring of the feature you're requesting is limited to:

I think it's reasonable to add this as (prominent) documentation, since I'm sure it'll be popular but it'll also encourage folks to use a cheap best practice (creating a Bridge interface for an external dependency rather than directly relying on it).

If I receive a PR with an interface, I will with all courtesy reject it. I really don't have time to go in and check best practices and variations for this right now, so if someone else can pick it up, I will happily merge it.

On Fri, Jul 20, 2018, 8:14 AM Nikodem Ośmiałowski notifications@github.com wrote:

Hi folks :) This topic has come up a few times before and the general thought from my side is that Tactician is meant to be used behind a project specific bridge interface (so you can tweak/replace it easily if you want). Having a direct interface from Tactician itself would break that a bit, so I'd prefer to see folks use their own Handler interface (if that's what they want) and wire it up that way.

From my standpoint, TacticianBundle is this bridge which allows to ease and speed up implementation of Tactician in Symfony-based projects. It already introduces a few implementation methods (eg. strict or by using typehints) so an autowiring interface could be an another method of speeding up implementation of this bundle and widening its user base.

If someone would like to contribute docs explaining how to do this as a new section in the README, I'd be happy to merge those though!

And if someone (maybe @mnavarrocarter https://github.com/mnavarrocarter as I understand that he already has a working solution) would create a pull request with the mentioned interface? 😉

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/thephpleague/tactician-bundle/issues/104#issuecomment-406499620, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI9TrFFqvbxIPfK6FzDBkfmsua6DaGuks5uIXVegaJpZM4VEbYZ .

Najki commented 6 years ago

OK, thanks for a detailed reply.

rosstuck commented 6 years ago

No worries! I think this whole thing can be done with an "_instanceof" in the DI config but I don't have time to investigate what the best practice is, sorry.

mnavarrocarter commented 6 years ago

After giving it some thought I'm inclined to agree with @rosstuck. Specially, because of the coupling of the bundle (in the infrastructure layer) with the application layer.

I can work in a PR for the docs that addresses these points:

Is that cool?

rosstuck commented 6 years ago

Sounds great, thanks a ton!

On Mon, Jul 23, 2018, 2:44 PM Matias Navarro Carter < notifications@github.com> wrote:

After giving it some thought I'm inclined to agree with @rosstuck https://github.com/rosstuck. Specially, because of the coupling of the bundle (in the infrastructure layer) with the application layer.

I can work in a PR for the docs that addresses these points:

  • Why we don't have an Interface for Symfony Autowiring
  • How can you create your own handler interface
  • How can you configure autowiring (for SF4)

Is that cool?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/thephpleague/tactician-bundle/issues/104#issuecomment-407045300, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI9Tn6BAPtuoAwnOIS0S0-oRDrDObdqks5uJcVFgaJpZM4VEbYZ .

maryo commented 6 years ago

It is also possible to use just a naming convention for autoconfiguration.

Domain\Command\:
    resource: "%kernel.project_dir%/src/Domain/Command/**/*Handler.php"
    tags: [{name: tactician.handler, typehints: true}]
magnetik commented 5 years ago

I just started using this bundle a few days ago, and I was quite puzzled by the fact that the manually mapping is the first mapping solution displayed : because it's the first it looks like it's the one recommended, while the typehint way is more the "symfony DX way of life" (with additional auto configuration from @maryo command).

Should I do a PR to move the typehint way (with a note about autoconfiguration) as the first one?

rosstuck commented 4 years ago

Sounds good if you like, I did that originally just because it seemed the simplest and the oldest.

Regardless, will close this old PR due to age and inactivity :)