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

[4.x] Add base path to `$request` and use `RouteContext` to read #2860

Merged
merged 5 commits into from Oct 5, 2019

Conversation

@adriansuter
Copy link
Contributor

adriansuter commented Oct 4, 2019

Another attempt for #2837. This is an alternative solution to #2859.

It uses the route runner to bring the base path to the $request object and then the route context helper class to read that attribute from the $request.

@adriansuter adriansuter changed the title Patch route context base path [4.x] Patch route context base path Oct 4, 2019
@adriansuter adriansuter changed the title [4.x] Patch route context base path [4.x] Add base path to `$request` and use `RouteContext` to read Oct 4, 2019
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 4, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling de91cf0 on adriansuter:patch-route-context-base-path into 80a52f7 on slimphp:4.x.

@l0gicgate l0gicgate added the Slim 4 label Oct 5, 2019
@l0gicgate l0gicgate added this to the 4.3.0 milestone Oct 5, 2019
@l0gicgate l0gicgate merged commit 3224271 into slimphp:4.x Oct 5, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@piotr-cz

This comment has been minimized.

Copy link
Contributor

piotr-cz commented Oct 6, 2019

Thank you!

@piotr-cz

This comment has been minimized.

Copy link
Contributor

piotr-cz commented Oct 6, 2019

As the basePath attribute is set in RouteRunner, request passed to middlewares and indirectly to error handlers doesn't have have it set.

Only place is the Route callable.

@adriansuter

This comment has been minimized.

Copy link
Contributor Author

adriansuter commented Oct 6, 2019

@piotr-cz Should we add the basePath attribute directly in the App->run() method? If someone is overriding this method, then it is important to make sure that this attribute will be written to the request. Otherwise we can't access it.

@l0gicgate Do you think we should/could undo the RouteRunner modification?

@adriansuter

This comment has been minimized.

Copy link
Contributor Author

adriansuter commented Oct 6, 2019

A middleware run before the routing middleware would not have access to any route context instance (route parser, etc), would it? So this solution would only work in a route callback.

@piotr-cz

This comment has been minimized.

Copy link
Contributor

piotr-cz commented Oct 6, 2019

After this PR attributes route, routingResults and basePath are set on the request by the RouteRunner which is available in route callable.

As I understand it, if you want to get route attributes in the middleware, you have to add RoutingMiddleware (see docs). But this sets only route and routingResults attributes.

Another option could be to set the basePath request attribute in App::handle method.

To be honest, I'm not sure what is the best way out of this.
I've decided to set base path on the view renderer during dependecy initialization.

@adriansuter

This comment has been minimized.

Copy link
Contributor Author

adriansuter commented Oct 7, 2019

@piotr-cz Injecting the base path into the middleware, error handlers and route callables is the preferred way to go. At least if you use dependency injection. The solution to use request attributes should IMO only be used in small projects in case of absence of dependency injection.

@piotr-cz

This comment has been minimized.

Copy link
Contributor

piotr-cz commented Oct 7, 2019

okay, thanks for explanation

@adriansuter adriansuter deleted the adriansuter:patch-route-context-base-path branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.