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

Allow tagging handlers per bus #29

Merged
merged 21 commits into from Nov 16, 2016

Conversation

tyx
Copy link
Contributor

@tyx tyx commented Sep 27, 2016

Following #20

Just let the check of tactician.middleware.command_handler as I'm not sure to get what is wrong with it.

Let me know what we need to go forward.

@tyx
Copy link
Contributor Author

tyx commented Oct 6, 2016

ping @rosstuck

I definitively need this feature ;)

@rosstuck
Copy link
Member

rosstuck commented Oct 6, 2016

Trying to make time, laid low with stomach flu, work and conf org. I'll get to it Saturday at the latest, really sorry. :(

@tyx
Copy link
Contributor Author

tyx commented Oct 6, 2016

Oh ! Take care of yourself, no problem ;)

I will start to use my fork for now.

@rosstuck
Copy link
Member

rosstuck commented Oct 6, 2016

Okay, made another review and tried to dive back into this code (the bundle was never really my work in the first place 😄).

First off, the bad news: I've made your job slightly harder by merging another PR that's been pending a while and adds a new security middleware. I'm felt it should go first but I am sorry about that.

Good news: if you can add docs for the multi-bus features, I'll merge it. I still need to complete a test with the security features before tagging a release but we'll be one step closer.

Regarding the big issue in the old PR, if dynamic command handler middleware names are a good idea, I'm not sure there's any good fix so let's write good docs there and ship it.

Sorry for the extra hassle, it's not fair to you and I really appreciate your effort!

@tyx
Copy link
Contributor Author

tyx commented Oct 7, 2016

no problem, as we say in french : "c'est le jeu ma pauvre lucette" ! ;)

@tyx tyx force-pushed the allow_tagging_handlers_per_bus branch from 3b7abfd to c4873f5 Compare October 27, 2016 14:57
@tyx
Copy link
Contributor Author

tyx commented Oct 27, 2016

I'm back on this PR !

I just rebased and add some doc. Let me know if it's enough or if I miss anything else.

@rosstuck
Copy link
Member

Broken build aside, looks pretty good to me. The build failure looks like this: sebastianbergmann/phpunit#2015

I'm going to ping @rdohms and @boekkooi because they had some input on the initial version. I'm organizing DomCode this week so my availability will....vary. 😅

@tyx
Copy link
Contributor Author

tyx commented Nov 16, 2016

Hi,

hope we can move forward on this issue soon if you get some extra time !

@rosstuck rosstuck merged commit 8a00e7f into thephpleague:master Nov 16, 2016
@tyx
Copy link
Contributor Author

tyx commented Nov 17, 2016

Thanks @rosstuck \o/

I guess we can plan to tag a new release (0.5 regarding the new feature security) : v0.4.1...master what do you think ? No hurry, I can rely to the dev-master on my side, but I like tag :p

@tyx tyx deleted the allow_tagging_handlers_per_bus branch November 17, 2016 10:09
@rosstuck
Copy link
Member

Well, that was the plan after fixing the build last night but then I started running into #31 on a fresh install. Sooooo now I've got to try and fix those issues this weekend maybe? I don't feel right tagging a release where the config from the readme immediately results in implementation errors with no useful debugging messages for the user.

In the meantime, dev-master is easier than your own fork at least so small steps forward? 😅

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.

None yet

2 participants