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

Tactician v2 #115

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft

Tactician v2 #115

wants to merge 14 commits into from

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Oct 7, 2019

I started working on v2

  • How to do debug command? With v2 the command handler map is not known in advance, mapping is done in runtime through locators. We would need inverse locator for this.
  • Tests
  • Update readme
  • Tag based mapping/handling stuff
  • should DoctrineMiddlewarePass, SecurityMiddlewarePass, ValidatorMiddlewarePass be part of this?
  • Let's only support modern versions of Symfony
  • Maybe we can move more of this out of the config.yml and more into the services.yml. Most of the work around the config.yml is just doing multiple command buses and adding middleware to them. These are tasks that I think would be better off in the services.yml where it's much simpler for folks to manage and work with them, instead of guessing at the autogenerated names (which are a repeatedly filed bug). The only services we need to auto generate then are the command locators, so maybe there's no need for the config.yml work at all.

@rosstuck
Copy link
Member

Hey, thanks for getting the jump on this. I confess I'm not using Symfony anymore so I was dreading this a bit. 😅

To answer some of your questions:

  • My thought with the debug command is to use MapByStaticList instead. As I recall (and it's been a bit), the compiler passes built a static list of all the mappings ahead of time and then built it into a a ContainerBasedHandlerLocator. So, the MapByStaticList does basically the same.

  • The compiler passes for Doctrine, Validator and Security middleware could all be split out into separate packages (splitting out the Validator stuff into a separate package has been requested a few times). That said, I honestly didn't have the energy or patience for the additional package management work that would entail.

  • I see the tag based mapping/handling stuff was deleted in this PR but I think it might still be needed. Don't forget, the bundle has some other features base tactician doesn't have (yet) like detecting mapping based on typehints.

Also because I haven't written these down anywhere, here's my paper sketch ideas for Bundle 2.0

  • Let's only support modern versions of Symfony

  • Maybe we can move more of this out of the config.yml and more into the services.yml. Most of the work around the config.yml is just doing multiple command buses and adding middleware to them. These are tasks that I think would be better off in the services.yml where it's much simpler for folks to manage and work with them, instead of guessing at the autogenerated names (which are a repeatedly filed bug). The only services we need to auto generate then are the command locators, so maybe there's no need for the config.yml work at all.

Anyways, none of that second part is a hard contract, just something to keep in mind. :)

@simPod
Copy link
Contributor Author

simPod commented Nov 23, 2019

@rosstuck The problem with debug command is, that we know only handlers on container build.

The mapping happens in CommandHandlerMiddleware by running $this->mapping->getClassName($commandClassName).

We would have to implement reverse mapping and run handlers through it to generate mapping up front. Currently, I don't see a way to do it 🤔

@simPod
Copy link
Contributor Author

simPod commented Nov 23, 2019

I see the tag based mapping/handling stuff was deleted in this PR but I think it might still be needed. Don't forget, the bundle has some other features base tactician doesn't have (yet) like detecting mapping based on typehints.

I decided to revisit it when the rest is done. After looking into it for a while, does not seem that trivial.

@simPod
Copy link
Contributor Author

simPod commented Nov 23, 2019

Just to be sure, it's correct to drop service locator, amirite?

@rosstuck rosstuck mentioned this pull request Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants