-
-
Notifications
You must be signed in to change notification settings - Fork 126
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
Question/Poll: Should we bump to v3 for per route and per group middleware? #113
Comments
Is it perhaps possible to make a different strategy and deprecate the old one? Then you'd be fine with a new minor. I'm OK with a v3 as well, though. |
If it's a small change, could you provide a branch to showcase the changes and discuss further based on that? |
The discussion is going to dictate where I go with the code. The breaking change would be here https://github.com/thephpleague/route/blob/master/src/Strategy/JsonStrategy.php#L19 where I would be adding the response object as the second parameter on this call. |
That's it? Then I'd lean more towards my first proposal: deprecate |
And just have the original strategy ignore the new middleware functionality? |
Maybe, does that seem plausible to you? |
Yeah, I think that works for me.
|
Maybe actually have it as That way we can continue to introduce strategies and it has a format. As in |
Deprecating JsonStrategy isn't necessary, no. But you'd want that to avoid confusion why there are 2 json related strategy classes, and if you want to remove it in v3. |
Well there would be value in keeping both, the difference would just have to be explained thoroughly.
|
Route 2.x is not compatible with things like League\Relay right? |
@CMCDragonkai I guess you meant https://github.com/relayphp/Relay.Relay ? League\Relay doesn't exist, afaik. if you have a |
Yes that's what I meant. Oh that's nice. Hopefully this can be clearer in the docs. |
So, I've been looking at bringing in a PSR-7 middleware implementation, the front runner is Relay at the moment. With version two though I feel I've made a glaring mistake with the
JsonStrategy
in not passing the response object off to the controller.What I'd like to do is bump to version 3 and implement in a way that I'd be happy with, allowing per route and per group middleware on any strategy. As things stand, because of the issue with the
JsonStrategy
I would probably have to provide a new middleware specific strategy to keep BC.I suppose one of the benefits of bumping to v3 now is that the install base for v2 is still quite small so support would only have to be provided for a small amount of time rather than waiting and having to support it for much longer, and it would be a relatively simply upgrade guide.
Thoughts? @hannesvdvreken @localheinz @kayladnls
The text was updated successfully, but these errors were encountered: