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

Adding RouteParser::getBasePath method #2852

Closed
wants to merge 2 commits into from

Conversation

piotr-cz
Copy link
Contributor

@piotr-cz piotr-cz commented Oct 1, 2019

See #2837

At this moment it's not trivial to retrieve basePath from code that doesn't have access to App or RouteCollector instances (and in my opinion code responsible for rendering responses shouldn't have).

RouteParser class seems like good place for it as

  • it contains other related methods (relativeUrlFor, urlFor, fullUrlFor)
  • it can be available as a route attribute (and indirectly in middleware) when using App::addRoutingMiddleware

I haven't implemented in this PR other methods that could be helpful like baseUrl by purpose as I'm not sure if it should be implemented in Slim or view classes.

@coveralls
Copy link

coveralls commented Oct 1, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling fd3449f on piotr-cz:feature/routeparser-basepath into a24fac2 on slimphp:4.x.

@odan
Copy link

odan commented Oct 1, 2019

IMHO: For the sake of "symmetry", the method name getBasePath would be more appropriate and meaningful.

@piotr-cz
Copy link
Contributor Author

piotr-cz commented Oct 1, 2019

@odan This is I was not sure about, as there also are methods like relativeUrlFor and not getRelativeUrlFor.

@odan
Copy link

odan commented Oct 1, 2019

@piotr-cz Ah yes, that's right.

@piotr-cz piotr-cz changed the title Adding RouteParser::basePath method Adding RouteParser::getBasePath method Oct 2, 2019
@piotr-cz
Copy link
Contributor Author

piotr-cz commented Oct 2, 2019

@odan
I've renamed method to getBasePath like you suggested.
It is used in other places in application and different one could confuse people.

@l0gicgate
Copy link
Member

This is a BC change since we’re adding onto the interface. @akrabat correct?

@akrabat
Copy link
Member

akrabat commented Oct 3, 2019

This is a BC change since we’re adding onto the interface. @akrabat correct?

Yes. Ouch.

@l0gicgate l0gicgate closed this Oct 3, 2019
@piotr-cz
Copy link
Contributor Author

piotr-cz commented Oct 3, 2019

What about keeping method in implementation without adding it to interface?
We could add it to interface next major version

@piotr-cz
Copy link
Contributor Author

piotr-cz commented Oct 4, 2019

Superseeded by #2859

@piotr-cz piotr-cz deleted the feature/routeparser-basepath branch October 4, 2019 08:19
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

6 participants