-
Notifications
You must be signed in to change notification settings - Fork 195
Url helpers produces unexpected results when querystring and/or optional route params in stage #325
Comments
I've had the same issue with fast route and the optional parts of the route not being handled correctly - zendframework/zend-expressive-fastroute#7 seems to fix it. I don't think there is any functionality for handling the query string in the URI generator - the router isn't responsible for that. I just append http_build_query($query) to the helper output. |
Same issues here, using zendframework/zend-expressive-fastroute#7 fixed some issues. The other issue I had was that a route always seems to use the parameters from the routeresult even if you don't supply parameters: zendframework/zend-expressive-helpers#10. Until it is fixed, not using a catch all route might help or swap to ZendRouter or Aura.Router. EDIT: Doing this |
I've just been looking at how Symfony handles the path extension for Twig. It looks like it appends all unknown parameters in the query string. I guess that's what you mean. Path: I need to have a look at this if this is possible. It would be a nice addition though. EDIT: Appending unknown parameters to the query string is not possible in an easy way. This has to be build into the routers. |
+1 |
Exactly. |
Can this issue be closed? The first part has been fixed with zendframework/zend-expressive-fastroute#4. Parameter overloading to the query is something that has to be implemented in the underlying routers. Unless I missed something, the zend-expressive-router can't (and shouldn't) handle this. |
|
@xtreamwayz yup it solves the first part. I'm still not sure about missing/lost query string arguments in helper. They are definitely components of a Uri, from a router's perspective, they are not part of the routes. Appending My current workaround to generate uri's in my templates as below and it looks bad:
I would like to use helper as
Should I roll my own helper or it should be solved in expressive? |
Appending unknown parameters is something that needs be handled by the router. Maybe you can get it to work with fastroute since route generation is done in zend-expressive-fastroute and not in fastroute itself. For Aura.Router and zend-router it should be implemented at the router side. An other option would be to write a zend-expressive-symfonyrouter since that router already handles it. The problem is that the zend-expressive-router isn't smart enough to tell what route parameters are known and which are unknown. Thinking of this, it could be done if you store all routes in zend-expressive-router and add some code to detect unknown parameters so they can be appended. To sum it up:
|
This would be very useful – it's awkward having to append |
Any one wants to test zendframework/zend-expressive-fastroute#21? It appends extra parameters to the query. It works with FastRoute only. Patches to Aura.router and zend-view are needed to achieve the same, which I happily leave to someone else :) |
Hello, i like the idea of adding extra query parameters, but in my opinion this should be implemented in the urlHelper. Routing related classes should only deal with route parameter substitutions. kind regards |
@pine3ree But how would you want to do this then? The url helper doesn't have knowledge of the routes and it doesn't know what parameters belong to a specific route. In other words, it can't tell what the extra parameters are and which should be appended to the query. Only the router knows this. That's why I said that Aura.Router and zend-view need PR's to support this. Fortunately FastRoute only does the routing, it doesn't handle generating routes, that's done in zend-expressive-fastroute and that's why I could create that PR. If you can tell me how I can add the same functionality to the url-helper without taking over the job of the router I'll make a PR for all routers. |
Hello @xtreamwayz The problem in my opinion is in term of responsibility: An url generator (such as the UrlHelper) should instead be allow edto build query string as well, so my option would be changing the url helper signature to smt like: __invoke ($routeName = null, array $routeParams = null, array $queryParams = null); it's up to the developer decide which parameter belongs to which area (routing or query string), and it should be a conscious decision not an automatic leftover query parameter, as I used to like not so long ago. kind regards, PS: please let me know what's your opinion on this. |
@pine3ree Thank you for the explanation. Now I understand what you mean. It's less "magical" and it might prevent unwanted side effects. For the record, slim 3 uses this: public function relativePathFor($name, array $data = [], array $queryParams = [])
{
// ...
if ($queryParams) {
$url .= '?' . http_build_query($queryParams);
}
// ...
} It would be easy to integrate into the url helper. And the template url generation helpers would need an update. ... I just did some research. zend-view and zend-router already seems to support this: https://docs.zendframework.com/zend-view/helpers/url/#fragments. So when messing with the UrlHelper, we might break this for zend-view. If you want to add this to the url-helper, the trick is to copy the same behavior from the zend-view and zend-router combo. So the 3rd parameter must accept an optional array like this: [
'query' => [
'commentPage' => 3,
],
'fragment' => 'comments',
] |
@xtreamwayz in zend-expressive/zend-components the roles are nicely separated, we can use the router to just build paths and use the helper to build complete urls leveraging the router for the path part. About the signature, as i forgot about the fragment component, i would prefer this one: public function __invoke($routeName = null, array $routeParams = null, array $queryParams = null, $fragment=null); for nothing less than convenience: adding nested layers adds visual complexity, just an example: $this->url = $container->get(My\Url\Helper::class);
// zend-view-like
$url = $this->url('catalog/product/index', ['page' => 123], ['query' => ['sort' => 'name.desc'], 'fragment' => 'pagination']);
//simpler signature
$url = $this->url('catalog/product/index', ['page' => 123], ['sort' => 'name.desc'], 'pagination'); using url helpers with the first signature inside php view templates can get things messy pretty soon. this also reflects the parts extracted by kind regards |
I guess the question is which of the routers support query strings and fragments? If any of them don't, then @pine3ree's suggestion makes sense, and would also mean that you wouldn't need to make changes to any of the routers. It might look something like this: public function __invoke($routeName = null, array $routeParams = null, array $queryParams = null, $fragment = null)
{
$route = $router->generate($routeName, $routeParams); // IDK what this actually looks like.
if ($queryParams !== null || $fragment !== null) {
$parts = parse_url($route);
if ($queryParams !== null) {
if (isset($parts['query'])) {
parse_str($parts['query'], $params);
} else {
$params = [];
}
$params = array_merge($params, $queryParams);
}
if ($fragment === null) {
$fragment = (isset($parts['fragment']) ? $parts['fragment'] : null);
}
$route = $parts['path']; // Will the path component always exist?
if (count($params) !== 0) {
$route .= sprintf('?%s', http_build_query($params));
}
if ($fragment !== null) {
$route .= sprintf('#%s', $fragment);
}
}
return $route;
} |
I think I prefer something like this: /**
* @param string $routeName
* @param array $params
* @param array $options ['query' => [], 'fragment' => 'foo', 'reuseResultParams' = true]
*/
public function __invoke($routeName = null, array $params = [], array $options = []) The reason is that in v3 of the UrlHelper an extra option is already set. It might be easier to just add all options in 1 array so we don't get too many parameters. zend-expressive-helpers v3 hasn't been released yet so it can still be changed. /cc @michaelmoussa any thoughts on this? |
@xtreamwayz v3 is actually adding another extra option merged in this PR, so yet another might be a bit much. Is this really something that needs to be done by the helper, though? The helper is meant to assist with generating paths based on defined routes, but neither query params nor a fragment have anything to do with route definition. Changing or omitting a query param or fragment isn't going to the change the route we end up matching. I've always just appended I'm not strictly opposed to adding support for the query params and fragment in the UrlHelper, but I'd like to see if we can find a better way than adding another parameter or cramming a lot of options into a single parameter array before going that route first, though. |
@michaelmoussa This is why I cc'd you as I have the same feeling about adding more and more options. I completely missed there was even a 4th parameter already. So in version 3 this is the current function: public function __invoke(
$routeName = null,
array $params = [],
$reuseResultParams = true,
array $routerOptions = []
) { Adding 2 more ($fragment and $queryParameters) will make it a mess. I'm just wondering, what does the
I have the exact same question. My initial thought was it should be handled by the router.
Another option is to do it the Symfony way and let the router handle this and append non-used parameters to the query. I try to mimic that here: zendframework/zend-expressive-fastroute#21 A cleaner option might be to use the |
@xtreamwayz @michaelmoussa Maybe it's more safe to leave the UrlHelper to act just as a path builder (a simple router method proxy) , despite the name. @michaelmoussa In my mind i like the idea of having the helper parameters sorted like they are in the real url as it's easier to remember.
Personally i would drop the support for the reuse parameter in favour of an automatic reuse when $routeName is assigned the $routeName, $routeParams, $queryParams, $fragment (*** public function generateUri($name, array $substitutions = []); in my opinion i would really like to know @weierophinney 's opinion on all this. kind regards |
Thanks for all replies. At first sight it looked to me like it can be solved by touching only @pine3ree this sounds also good to me:
and usage on templates (zend-view like) something like
or twig way
or
In What happens if I pass current
Since we have a PSR $request instance near everywhere and querystring arguments should not be part of route or router but part of Uri, imho view helper may easily read existing querystring arguments from it. is there any drawback of this? |
@xtreamwayz Do you know which of the 3 routers support query strings (a) As a first-class feature, (b) By allowing a route path to contain a query string? I know that FastRoute at least supports (b). I think that we need this information before making a decision on whether it belongs in the router or the helper. |
zend-router seems to support it in ZF3 (https://docs.zendframework.com/zend-view/helpers/url/#fragments), but it's not ported to it's expressive version. |
I just create a commit on my fork to demonstrate possible implementation by modifying only urlhelper: edigu/zend-expressive-helpers@ae70389 Advantages:
And two disadvantages at first sight:
|
@edigu A router can generate a URL with a query string and/or a fragment, so your code might generate output like You'd need to use |
This issue is starting to become very interesting :-). A router obviously can offer a method to build a complete url, and a few implementations do just that and I used and still use them in my daily work. In my opinion the real question is : SHOULD a router be able to generate a full url? In some frameworks there is no url building helper to do that, so it's natural to implement the feature in the router itself. BUT in general a router's task is to map a path to an handler (middleware, controller action other callables etc etc). Different query parameters are handled by the same handler attached to the path. zend-expressive provide us with a separate urlHelper, which could be used to build complete urls, using the router methods for the router -> parse PATHs urlHelper - >builds URLs (the path part being built by the router) kind regards |
This is actually a very good point. I'm certainly leaning towards the idea of this being helper functionality rather than router functionality. What about this signature? public function __invoke(
$routeName = null,
array $routeParams = [],
array $routerOptions = [],
array $options = []
) {
{{ path('routeName', {a: 123}, null, {reuseResultParams: false} }} {{ path('routeName', {a: 123}, null, {query: {page: 5}}) }} (I'm not really sure what Edit: I just remembered that @xtreamwayz already suggested the same thing for the |
@glen-84 my ideal signature would be: public function __invoke(
$routeName = null,
array $routeParams = [],
array $queryParams = [],
$fragment = null
); the main reason is that i can read the parameters in the same order of the real url i would use the null $routeName to automatically reuse current route parameters (which we can partally override via $routeParams) but of course different developers may have different opinions and needs so at the end it will be up to the repo reviewers to decide which signature to adopt. In the current (3-dev)implementation the signature is presenting the helper as a mere proxy for the router capabilities. public function __invoke(
$routeName = null,
array $params = [],
$reuseResultParams = true,
array $routerOptions = []
) but i am very curious about the final decision....I will adapt anyway.... |
Thank you everyone for your comments. Since there's been a lot of discussion, let me try to summarize the main points that have been touched on thus far. This is pretty long, so I'll recap at the end with a specific proposal for comments. One option is to add this functionality to the routers that Expressive supports. Most of you seem to be leaning away from that, and I agree, mainly for two reasons:
As such, I think any further suggestions on how to best implement this behavior should not involve the router classes at all. That leaves us with If nothing changes between now and the public function __invoke(
$routeName = null,
array $params = [],
$reuseResultParams = true,
array $routerOptions = []
) The
SO, if we make In other words, don't merge route params if the
Perhaps And, of course, we want to support query params and fragment identifiers. We originally were leaning away from doing it in Proposal All that being said, I propose the following:
The new method signature would be one of the following, depending on the option we choose for #3. // Option 1
public function __invoke(
$routeName = null,
array $routeParams = [],
array $queryParams = [],
$fragmentIdentifier = ''
) OR // Option 2
public function __invoke(
$routeName = null,
array $params = []
) For $params = [
'_route' => [],
'_query' => [],
'_fragment' => '',
]; The For the That's it. What do you all think? @xtreamwayz @glen-84 @pine3ree @weierophinney ? |
If you consider, I am for Option 1 here. It is more easy to remember than recalling the keys for the params. Thank you. |
Hello @michaelmoussa , I am obviously biased towards option 1 since I proposed it as a possible signature for the reasons i won't repeat (don't want to bother people to death :-) ) Option 1 will also provide easier transitions from the older signature public __invoke($routeName = null, array $params = []); since the meaning of the 1st 2 parameters are basically unchanged. I will be easily adapt to option2 anyway, but in this case I would prefer keys without underscores (I know you added it to avoid name clashes in existing applications). The transition phase will also have a little impact on performance, since we have to check for 3 keys per url, I am thinking about pages with hundreds or even thousands of links (xml sitemaps, feeds, data feeds to other remote social crawlers) I am not sure If my english was clear enough (I apologize if it was not) but about the reuseRouteParams matter i believe that:
There is another possibility (but this will add more automagic to remember): // Option 1.b
public function __invoke(
$routeName = null,
array $routeParams = null,
array $queryParams = null,
$fragmentIdentifier = ''
)
But in the latest years I've tried to avoid automagics so i doubt i will ever use these addons. It is nice to have them though, but as @harikt said, they are not immediatly clear from the signature alone., we need to peek at the doc-block of the ide code-hints. kind regards to eveyone and thanks again for your time @michaelmoussa . |
_Signature_ I really don't like option 2. This would not look good: {{ path('routeName', {_route: {a: 123}, _query: {page: 456}}) }} vs: {{ path('routeName', {a: 123}, {page: 456}) }} That's a big difference. So I would definitely vote for option 1. _$reuseResultParams_ Regarding the {{ path('routeName') }} {{ path() }} It would mean that you could never have result params merged in when specifying a route name, and it's also a bit "magical". I had the same thought as @pine3ree regarding the use of {{ path(null, {}) }} As {{ path(null, {a: 123}, null, null, {resultParams: true}) }} ... but this might not be needed that often (?). The final signature would be: public function __invoke(
$routeName = null, // null means currently-matched route
array $routeParams = null, // null means use result params
array $queryParams = null, // null means no query params to merge.
$fragmentIdentifier = null, // null means no fragment/don't change fragment in route
array $options = [] // (see below)
) $options:
The Would this work? Are there any use cases that are not covered? |
As for the requirements: Generate the current matched route and merge $params with the current params {{ path(null, {a: 123}, null, null, {resultParams: true}) }} Generate the current matched route, but don't merge $params with the current params {{ path(null, {}) }} Generate the route named $routeName and merge $params with the current params {{ path('routeName', {a: 123}, null, null, {resultParams: true}) }} OR (if you don't have params): {{ path('routeName') }} Generate the route named $routeName, but don't merge $params with the current params {{ path('routeName', {a: 123}) }} OR (if you don't have params): {{ path('routeName', {}) }} You could perhaps also make use of |
@glen-84 You are mixing two things here. We are talking about how the UrlHelper has to look like. How it looks like for the twig renderer is something different. Even though the url helper might require an array with Also _route is not what you think it is. _route would be the option to reuse the current route result. That is not something what would be called from twig directly since you don't have / don't need access the route result in your template. Eventually it might something like this for twig: {{ path('routename'), {<parameters>}, {<options like query, fragment, reuseRouteResult>}) }}
{{ path('routeName', {foo: bar}, {query: {param: 1}, fragment: test, reuseResult: false}) }} |
I just used Twig as a way of visualizing the differences. At the moment, the PHP signature is the same as the Twig signature, is it not? (routeName + routeParams) I don't really see any reason to make them different. |
@glen-84 kind regards, |
I wasn't suggesting any dependency between the Twig tag and the URL helper. I was just saying that it would make sense if they were the same or similar (for consistency/familiarity): public function __invoke(
$routeName = null, // null means currently-matched route
array $routeParams = null, // null means use result params
array $queryParams = null, // null means no query params to merge.
$fragmentIdentifier = null, // null means no fragment/don't change fragment in route
array $options = [] // (see below)
) {{ path('routeName', {rParam: 1}, {qParam: 2}, 'fragment', {resultParams: true}) }} What's wrong with that? I'm not sure why you're suggesting leaving the Twig tag as it is – that would defeat the purpose of this enhancement request (see the OP, which uses Twig examples). It makes no sense to expect every developer to create their own tag/function for this, when it can be provided by the framework. |
Hello @glen-84, what i was suggesting is that: since until now twig {{ url('routeName', {rParam: 1}, {qParam: 2}, 'fragment', {resultParams: true}) }} as of now the If we do not modify the In this way we have the possibility to use the most semantically appropriate function in twig templates and we do not need to modify code based on the present behaviour: need a path => use {{ path(...) }} @glen-84 @glen-84
|
@glen-84 kind regards |
@pine3ree If I'm not mistaken, you can already use I don't really see the need for a separate function/tag. |
Hello @glen-84, i haven't checked in dev, but from the code in master twig path is a wrapper around UrlHelper::generate(...), which in turn is (a bit more than) a wrapper around the RouterInterface::generateUri(...), which in master only makes substitutions in the route pattern.
so Since I haven't used twig with zx I cannot really help in choosing weather to use kind regards and thank You for sharing your opinion. |
@michaelmoussa Coming back on your last message: You speak of 2 options: // Option 1
public function __invoke(
$routeName = null,
array $routeParams = [],
array $queryParams = [],
$fragmentIdentifier = ''
) OR // Option 2
public function __invoke(
$routeName = null,
array $params = []
) Option 2 would not work: And then there is a 3rd option: // Option 3
public function __invoke(
$routeName = null,
array $routeParams = [],
array $queryParams = [],
$fragmentIdentifier = '',
$reuseResultParams = true
) But that there are too many parameters already and we loose the $routerOptions. Also what happens if something else needs to be added in the future? So what's left is this early proposal: // Option 4
public function __invoke(
$routeName = null,
array $routeParams = [],
array $options = []
) Where $options = [
'query' => [
'parameter' => 'foo',
],
'fragment' => 'foobar',
'reuse_result_params' => false,
'any other options' => 'that needs to be passed to the router'
] Or if you want to take this into consideration you would get option 5: // Option 5
public function __invoke(
$routeName = null,
array $routeParams = [],
array $helperOptions = [],
array $routerOptions = []
) I've read all the comments and understand the concerns about how beautiful something looks, but this is about readability. I'm think this reads a lot better: // $helper('routeName', [routeParam], [options]);
$helper('foobar', ['foo' => bar], [
'query' => [
'bar' => 'baz'
],
'fragment' => 'qux',
'reuseRouteResult' => true,
'some' => 'option'
]); than this: // $helper('routeName', [routeParam], [queryParam], 'fragement', reuseRouteResult, [routerOptions]);
$helper('foobar', ['foo' => bar], ['bar' => 'baz'], 'qux', true, ['some' => 'router option']);
// or this:
$helper('foobar', ['foo' => bar], [], '', true);
$helper('foobar', ['foo' => bar], [], '', true, ['some' => 'router option']);
// What's the empty array and string?
// What does true do?
|
You can answer that with an IDE – the fact that PHP doesn't support named arguments is a language limitation. I'm not sure if it should affect the decision regarding the signature. With that said, I don't have very strong opinions regarding the helper signature, as long as the resulting Twig tag doesn't end up looking messy (and yes, I know that it can have a different signature). (I would use null instead of an empty string for fragment) |
OK folks, this has been discussed quite a bit and I think a decision just has to be made because there's no obvious signature that has overwhelming support from everybody. The goal is to continue to support everything that's currently supported in the pending Unless @weierophinney wants to veto or there is strong opposition (meaning "this is horrible and wrong. do not do it!", not "it's not my favorite but it's ok I guess"), I'm going to go with this: public function __invoke($routeName = null, $params = [], $options = []); Explanation below. Note that when I say "behaves as it does now", I'm referring to the pending
SO...
I know there will probably be some contention regarding listing out all the parameters individually vs. using keyed arrays, but at this point I think it's a matter of personal preference / style. If you do not like that particular style, it is very easy to either extend Finally (just in case), yes, I know that:
is longer than:
BUT, both of them are longer than:
which is what we'd have to do now, so hopefully we can agree there's not a perfect solution and that this one is at least not bad. :) |
Hello @michaelmoussa , it's fine with me, and as you suggested it's easy to override it with your own wrapper's signature. One final thing that came to my mind about BC break: since we will always use key-value pairs for $options, we could also keep the current use of $params as route subsitutions and move the So let's wait from other people's opinions and maybe @weierophinney's. Thanks and kind regards |
@pine3ree my reason for putting The transition period won't be a big deal at all - I'll likely have the deprecation notice in the 3.0.0 release and add a commit for the removal, but it just won't be released immediately. Given the fact that it's a major version bump, it'll be documented, and there will be a brief transition period, I'm confident that anybody with a sound upgrade / test / deployment process will not have surprise breakage. |
I guess it's fine ... verbose ... but fine. =) @xtreamwayz Will you be updating the Twig functions based on this work? If so, will it have a similar signature, or a less verbose alternative? |
@glen-84 Yes, I'll leave a message here once I created the PR. I'd like your feedback before merging it. |
I'll work on the UrlHelper updates sometime next week, as my spare time is pretty full up until then, but if anybody wants to jump on the changes before then and fill their Hacktoberfest quota, please feel free. :) |
I've got time for this tomorrow morning. |
In case you haven't seen it yet, there is a PR for the twig renderer: zendframework/zend-expressive-twigrenderer#18 |
Closing, as the related work is being done in the relevant repositories (which are not this one!), and previous comments have links to the issues/pull requests to track that work. |
I hit this problem today when trying to implement a pagination template using Twig as template renderer.
I have a catch-all route for user-related operations and trying to handle multiple actions in a single class like documented in cookbook.
And I want to generate a uri path with additional arguments using helper like below:
Expected output:
/user?page=1
Actual output:
/user[/{action:add|edit|show|list}[/{id}]]
Problem(s) : Helper exposing the original route in view layer and
page
argument is lost.Other examples:
Expected output:
/user
Actual output:
/user[/{act:add|edit|show|list}[/{id}]]
Expected output:
/user
Actual output:
/location[/[/]]
I have not tried yet with the the zend-router. I believe there should be a way to create routes with querystring arguments.
Seems like
generateUri()
method of the Zend\Expressive\Router\FastrouteRouter is responsible but I'm not sure that.I have also tried switching the router from FastRouter to AuraRouter:
Now, when I try to generate relative uri;
Expected output:
/user?page=1
Actual output:
/user
Result : My route definitions are not exposed but page argument is still lost.
Am i missing something? what is the proper way to create URL's with querystring arguments in expressive?
The text was updated successfully, but these errors were encountered: