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

Change route context attribute names #2889

Merged
merged 3 commits into from Nov 29, 2019

Conversation

adriansuter
Copy link
Contributor

PR for #2883

The attribute names are even configurable - although I am not sure if that should be an option. If not, let me know - I will remove all the unnecessary calls to static methods and instead hard code the attribute names.

If yes, maybe we could find another solution than using static methods and properties in the RouteContext class - I am not totally happy with this static solution.

@coveralls
Copy link

coveralls commented Nov 28, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 2c6d2be on adriansuter:patch-route-reserved-keywords into 4360365 on slimphp:4.x.

@adriansuter
Copy link
Contributor Author

Proposition

Pass an optional RouteContextFactoryInterface to the App. In App::handle() use that factory to create a route context and attach it to the request.

So we need

RouteContextFactoryInterface::createRouteContext():RouteContextInterface;
RouteContext::attach(ServerRequestInterface, RouteContextInterface):ServerRequestInterface;

Then, whenever we need that route context, we can grab it from the $request.

RouteContext::fromRequest(ServerRequestInterface):RouteContextInterface

The RouteContextInterface has methods to set and get route parser, routing results, base path and route.

Warning: A lot of phpunit test cases would need rewrites.

Copy link
Member

@l0gicgate l0gicgate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the _ATTRIBUTE_NAME suffix from the constants, then it's good to go!

@adriansuter
Copy link
Contributor Author

Good to go.

@l0gicgate l0gicgate added this to the 4.4.0 milestone Nov 29, 2019
@l0gicgate l0gicgate merged commit ea75cf4 into slimphp:4.x Nov 29, 2019
@adriansuter adriansuter deleted the patch-route-reserved-keywords branch November 29, 2019 18:10
@jacekkarczmarczyk
Copy link

jacekkarczmarczyk commented Nov 30, 2019

I'm not familar with v4 yet so I wasn't sure if this attribute names where a public this, but apparently they are (#2892), so changing them is definitely a breaking change.

I'd rather see these names as configurable thing (with default names unchanged, 'route' etc) in RouteContextConfig or something like this, which could be injected to classes that use the attributes. Probably there could be also RouteContextFactory that would have RouteContextConfig injected in the constructor, otherwise you should pass the config in RouteContext::fromRequest arguments

@N1ghteyes
Copy link

can confirm that this is a breaking change as per my issue - referencing the docs is one thing (my original code came from v4 docs - one of the basic examples i think) but if possible i would like to see Slim avoid breaking changes in minor releases.

@l0gicgate
Copy link
Member

l0gicgate commented Nov 30, 2019

@N1ghteyes

can confirm that this is a breaking change as per my issue

Your issue is unrelated to this. As I mentioned, pattern matching is done via FastRoute, which isn't code that was touched by this PR.

i would like to see Slim avoid breaking changes in minor releases.

We do too however using the request attributes directly isn't the correct way to access the route/routing results/base path/etc. While it was originally stated that way in the earlier version of the v4 docs, this was a mistake and has since been changed. Using the RouteContext object which was in the original 4.0.0 release would have avoided any potential breaks.

@l0gicgate l0gicgate mentioned this pull request Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants