-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Rewrite Router->urlFor() to generate complete Urls #2493
Conversation
Slim/Router.php
Outdated
return $this->pathFor($name, $data, $queryParams); | ||
// Check if container['request'] is initialized. | ||
if (!$this->container->has('request')) { | ||
throw new RuntimeException('Request object is not initialized yet.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For one of the use cases (creating email templates) it might very well happen that this mechanism is called without a request context when emails are generated from a command line script or from a task queue.
There is no way to automatically build the base URL without a Request object. So instead of just throwing an Exception here, it would probably be helpful to be able to configure the base URL from a config setting and fall back to it if no Request is available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's treat this as a separate issue for a subsequent PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be obsolete now, since the $uri
gets passed as argument and $request
ist not needed anymore
There's a potential problem here in that this is not backwards compatible. |
Refs #1316. Actually, the name I'd suggest:
As a result, in Slim 4 we would have |
I agree. Let's call this |
@akrabat I can add this PR as part of the router refactoring process going on at the moment for the Slim 4 branch. It’ll probably be last though. |
Okay. Separate PR :) |
Next step for this one is for @Jmp00000001 to update this PR. |
Hello, I will have a look after work. This should be updated?
|
I have
Let me know if I missed something :) |
Just a nitpick: we could also reverse the Slim 3 BC alias, i.e. make |
Here you go :) |
Please remove the |
Slim/Router.php
Outdated
|
||
/** @var Uri $uri */ | ||
$uri = $this->container->get('request')->getUri(); | ||
if (is_string($uri)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test case for this condition as well please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the check that $uri
is a string is not required at all.
There is no way Request::getUri()
returns something different than a UriInterface
object:
- Only the Request constructor sets the
$uri
and expects anUriInterface
. The constructor will throw otherwise (at latest when calling class methods likeUri::getPort()
) - The docblock of
Request::getUri()
also mentions:This method MUST return a UriInterface instance
No clue what I was thinking..
I simply remove that check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this brings me to question the method signature in the first place. Look at the way I implemented it in Slim 4:
https://github.com/slimphp/Slim/blob/4.x/Slim/Routing/RouteParser.php#L129-L136
Since this is a new method, we should force a UriInterface
being passed in by the user instead of trying to do gymnastics to get to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test coverage for the to aforementioned conditions and I'll merge. Thanks!
I will have a look today/tomorrow. |
I'd suggest renaming
And here are the correspondences:
|
@vlakoff we cannot change However, I'm open to that for Slim 4 since we're not even in alpha yet. |
Same as the urlFor() change, keeping the old name as a BC alias in Slim 3. |
@vlakoff I'm going to PR that for Slim 4 on the
I also want to point out that there's not going to be any deprecation warning. Since As for Slim 3, I don't think any further changes are warranted since we're so deep in the release cycle. The functionality is there and is usable, that's what matters most. |
Thanks for merging! |
Considering that I chose these names trying to apply the best possible the principle of least astonishment. But on second thought, an other acceptable naming scheme could be " This is really worrying me and it's still time to change if necessary, please provide your feedback on this matter! |
Wouldn’t the term “Uri” instead of path apply here then @vlakoff?
A URL should be with protocol. A URI should be part of a url much like a path is. So |
That's quite a headache! I have done some reading, and it (probably arguably) seems that what we are dealing with are really URLs, as they are resources' locations; for the value to be useful, the scheme and domain have to be known, i.e. specified but could also be deduced. In our case, the browser does the latter, by completing the provided URL using the current location. Further reading: What is the difference between a URI, a URL and a URN? Also worth noting:
Finally, using "uri" surely would be very confusing for the users! |
By that logic it's totally fine to use |
Agree with the above. So, personally I'd bend staying on To summarize:
So, shall we settle on this? |
We are going to leave the terminology as in in Slim 3. But for Slim 4 the
So I think this is settled! Thanks for your input. |
I apologize for the extended chat… I really wanted to be sure the new naming convention was good. I would have felt so bad if I had introduced a bad naming scheme in Slim 4! |
Hello!
Like @splitbrain mentioned in slimphp/Twig-View#101 there is actually no way to generate a complete url while slim runs in a sub directory.
This pull request refactors slimphp/Twig-View#102 TwigExtension->urlFor() to Router->urlFor()
Is there any chance you can merge this?
Let me know if this pull request need improvement.