Url helpers produces unexpected results when querystring and/or optional route params in stage

[edigu]

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.

[
    'name'            => 'user',
    'path'            => '/user[/{action:add|edit|show|list}[/{id}]]',
    'middleware'      => Action\UserActions::class,
    'allowed_methods' => ['GET','POST'],
],

And I want to generate a uri path with additional arguments using helper like below:

{{ path("user", { "page" : 1 } ) }}

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:

{{ path("user") }}

Expected output: /user Actual output: /user[/{act:add|edit|show|list}[/{id}]]

{{ path("user" { "act":null, "id":null } ) }}

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:

[
    'name'            => 'user',
    'path'            => '/user{/act,id}',
    'middleware'      => Action\UserActions::class,
    'allowed_methods' => ['GET'],
    'options'         => [
        'tokens' => [
            'act'    => '(add|edit|show|list)?',
            'id'     => '\d+',
            ]
    ],
],

Now, when I try to generate relative uri;

{{ path("user", { "page" : 1 } ) }}

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?

[kynx]

I've had the same issue with fast route and the optional parts of the route not being handled correctly - https://github.com/zendframework/zend-expressive-fastroute/pull/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.

[geerteltink]

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 {{ path("user", { "page" : 1 } ) }} should output as /user. The parameter page is part of the path, but since it is not there it shows only /user. There is no support for query parameters (I know, since I've build the Twig extension responsible for it). What you can do is use {{ path("user") }}?page=1 or change the path in the config to: 'path' => '/user/page{page}',. Again, don't use a catch all until this is improved.

[geerteltink]

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: /user[/{act:add|edit|show|list}[/{id}]] Twig: {{ path(user, {'act': 'edit', 'id': 1, 'foo': 'bar'}) }} Expected output: /user/edit/1?foo=bar

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.

[ricardofiorani]

+1 I'm using FastRoute and Plates.

[edigu]

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.

Exactly.

[geerteltink]

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.

[drakulitka]

/categories[/{action:select|insert|edit|delete}/[{id}]]', If you go to /categories/edit/5 And then in /categories/insert The URL remains: /categories/insert/5 ID does not disappear Use the links /categories/insert: {{ path('categories', {'action': 'insert', 'id':null }) }} to cleanse ID There are other options?

[edigu]

@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 unknown values as querystring args sounds reasonable to me. Since we use path(), serverurl() like helpers to build links, there should be a way to attach these arguments as querystring.

My current workaround to generate uri's in my templates as below and it looks bad:

`<a href="{{ path('foo', { bar: baz }) }}?page={{ pageNo }}">`

I would like to use helper as

<a href="{{ path('foo', { bar: baz, page:pageNo }) }}">

Should I roll my own helper or it should be solved in expressive?

[geerteltink]

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:

• Hack zend-expressive-fastroute
• Hack zend-expressive-router ???
• Write zend-expressive-symfonyrouter
• Or send PR's to Aura.Router and zend-router

[glen-84]

This would be very useful – it's awkward having to append ?page=123 to an existing URL, which may or may not already have a query string.

[geerteltink]

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 :)

[pine3ree]

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

[geerteltink]

@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.

[pine3ree]

Hello @xtreamwayz I remember I added a PR to slim v2 to do exactly the same ( a feature provieded by Yii - at least v 1.x). I wanted to pass parameters to the url builder and all leftovers were transformed to query parameters. This is a very convenient feature for the developer, and a natural feature in Yii because in Yii you could decide to use or not to use seo-url and if not the route itself was translated to a query param r.

The problem in my opinion is in term of responsibility: a router and a path generator should only deal with parameters that identify completely a single route and nothing more.

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, maks

PS: please let me know what's your opinion on this.

[geerteltink]

@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',
    ]

[pine3ree]

@xtreamwayz https://github.com/slimphp/Slim/commit/ad8b194ef04850c254da607e2c62252d358a586e :-) In slim 2 i did the automatic assignment to the query string, in slim 3 i added the $queryParams. At the time of my PR the roter method was still called urlFor, which is imho more proper since we use the slim router as a url-generator (i don't believe there is a separated url helper - twig renderer excluded ) not only as a path generator.

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 parse_url() via the php url helper constants: PHP_URL_PATH, PHP_URL_QUERY, PHP_URL_FRAGMENT

kind regards

[glen-84]

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;
}

[geerteltink]

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?

[michaelmoussa]

@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 ?foo=bar&baz=etc#comments to generated URLs (which is a lot shorter than something like ['query' => ['foo' => 'bar', 'baz' => 'etc'], 'fragment' => 'comments']).

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.

[geerteltink]

v3 is actually adding another extra option merged in this PR, so yet another might be a bit much.

@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 $routerOptions do exactly?

Is this really something that needs to be done by the helper, though?

I have the exact same question. My initial thought was it should be handled by the router.

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.

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 $routerOptions and let the router handle it if it's supported.

[pine3ree]

@xtreamwayz @michaelmoussa I wasn't aware of the extra paramater too. It's indeed starting to get real messy. :-)

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 The advantage of having the generator deal with query params is that we could just pass a simple array and it would normalize (url-encode values) it for us. As of now we need to do explicitly when building the query string.

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.

1. path (1.1 routeName + 1.2 params + 1.3 reuse). 2. query 3. fragment but the route part is needing a lot (3 vars), thus potentially ending in a messy signature

Personally i would drop the support for the reuse parameter in favour of an automatic reuse when $routeName is assigned the null value. the route params can be used to override already matched values.

$routeName, $routeParams, $queryParams, $fragment

(*** btw imho the nomenclature is also a little imprecise: the RouterInterface has the following method signature

public function generateUri($name, array $substitutions = []);

in my opinion generatePath would be more appropriate, since it deals with matching a route name and substitute parameters to buil a uri path. On the other side i find the name "$substitutions" very appropriate as it just reveal us its purpose. ***)

i would really like to know @weierophinney 's opinion on all this.

kind regards

[edigu]

Thanks for all replies. At first sight it looked to me like it can be solved by touching only zend-expressive-helpers and zend-expressive-router components. I also looked into path generation flows of all routers just after @xtreamwayz 's mention to FastRouter. Seems like some good solutions mentioned above requires creating different patches against multiple-repositories as @xtreamwayz' said. I was thinking on to create a PR on this after comments unfortunately I couldn't create spare time.

@pine3ree this sounds also good to me:

we can use the router to just build paths and use the helper to build complete urls leveraging the router for the path part.

and usage on templates (zend-view like) something like

$url = $this->url('catalog/product/index', ['page' => 123], ['query' => ['sort' => 'name.desc'], 'fragment' => 'pagination']);

or twig way

<a href="{{ path('user', { action: filter }, { query : { page:42}})}}">Link</a>

or

<a href="{{ path('user', {action: filter, status: 4 }, { page:42 }}}">  

In { query : { page: 42 } } scenario, imho query key turns into a reserved-word. At least in helper-level.

What happens if I pass current Uri instance into UrlHelper like achieved in ServerUrlMiddleware :

$this->helper->setUri($request->getUri());

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?

[glen-84]

@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.

[geerteltink]

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.

[edigu]

I just create a commit on my fork to demonstrate possible implementation by modifying only urlhelper:

https://github.com/edigu/zend-expressive-helpers/commit/ae70389d9a911467f79837a8993227c12c9d86f9

Advantages:

• It only requires small changes in helper-level.
• It uses underlying router to generate path, routers doesn't need to know anything about the query and fragment. Generates a complete uri with query and fragment if possible.

And two disadvantages at first sight:

1. My changes will be conflicted with the changes made here by @michaelmoussa
2. Existing querystring arguments will be lost if a new stack provided by developer. This needs passing current qs arguments into helper's itself to merge existing and provided ones. A psr UriInterface instance can return querystring arguments as string, and psr ServerRequest instance has getQueryParams() method. I'm not sure which one would be more appropriate.

[glen-84]

@edigu A router can generate a URL with a query string and/or a fragment, so your code might generate output like /path?param=1?another=5.

You'd need to use parse_url, parse_str, etc. (see my example)

[pine3ree]

@glen-84 , @edigu

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. Therefore it should be the same way for the reverse process, i.e. building JUST paths. If the router does not use query parameters to match a route, it should be not able to use them to build something it does not concern itself. Imo the router should be completely agnostic of query strings and full urls, unless it uses them to match different route/handler pairs, but that's not what happens.

zend-expressive provide us with a separate urlHelper, which could be used to build complete urls, using the router methods for the path part. otherwise the helper would be just a proxy to the router generation method.

router -> parse PATHs router-> build PATHs

urlHelper - >builds URLs (the path part being built by the router)

kind regards

[glen-84]

If the router does not use query parameters to match a route, it should be not able to use them to build something it does not concern itself.

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 = []
) {

reuseResultParams, query, and fragment would be in $options.

{{ path('routeName', {a: 123}, null, {reuseResultParams: false} }}

{{ path('routeName', {a: 123}, null, {query: {page: 5}}) }}

(I'm not really sure what routerOptions are for)

Edit: I just remembered that @xtreamwayz already suggested the same thing for the $options array. The only question is the routerOptions – should it be a separate parameter or just another $option?

[pine3ree]

@glen-84 hello,

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 {{path(route-with-substitute-route-params)}}?{{query}}#{{fragment}}

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....

[michaelmoussa]

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:

1. As @pine3ree mentioned, the router doesn't use query params to match a route. Add ?foo=bar to the /api/ping route in the Expressive Skeleton Application, and you'll find that the route no longer works - with or without the query params.
2. The fragment identifier is available to the browser only and is never sent to the server, so it's even less relevant to routing than query params are.

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 UrlHelper.

If nothing changes between now and the 3.0.0 release, the UrlHelper::__invoke(...) signature will be this:

public function __invoke(
    $routeName = null,
    array $params = [],
    $reuseResultParams = true,
    array $routerOptions = []
)

The $routeName and $params arguments are present in the current version, and their use is documented in the README, so I won't repeat them here. Regarding the new parameters, however...

$reuseResultParams comes into play when a RouteResult is composed in the helper and $reuseResultParams is set to false. This combination will result in the existing matched route parameters to be ignored and only consider the passed $params. The reason for this change is discussed in the PR. One suggestion was to remove the option entirely and have $routeName = null indicate that route params should not be reused. That option could work, but it would change how the helper has behaved up until this point. The question to consider I think is how often the various use cases for this helper are likely to be needed.

1. "Generate the current matched route and merge $params with the current params"
   • Accomplished with $routeName = null && $reuseResultParams = true
2. "Generate the current matched route, but don't merge $params with the current params"
   • Accomplished with $routeName = null && $reuseResultParams = false
3. "Generate the route named $routeName and merge $params with the current params"
   • Accomplished with $routeName != null && $reuseResultParams = true
4. "Generate the route named $routeName, but don't merge $params with the current params"
   • Accomplished with $routeName != null && $reuseResultParams = false

SO, if we make $routeName === null indicate "don't merge with the current route params", that will make cases (1) and (4) impossible. If we do the opposite and make, $routeName !== null, indicate "don't merge with the current route params", that will make case (2) and (3) impossible. Which is the more common use case? I don't know. However, maybe we can also treat $routeName === $result->getMatchedRouteName() as a replacement for what $routeName === null && $reuseRouteParams === false?

In other words, don't merge route params if the $routeName is specified, otherwise, do. That would make only option (3) in the list above impossible, but I think that might be the least common use case.

$routerOptions may not actually be necessary now that I look at it more closely. I'd like to get @weierophinney to give it a good look as well to confirm, but it seems to me that, of the routers we currently have, only Zend Router would ever have any use for $options? The other two seem to be able to get whatever they need from $params. zend-expressive-zend-router sets the name and only_return_path properties for $options automatically, and after digging a bit into Zend Router I see there are numerous other possible options that can be set like force_canonical, uri, has_child, locale, etc... I suppose generating a URL for a specific locale is a perfectly valid use case, and I don't really see any other way to communicate that to the underlying Zend Router without using $routerOptions. So, either this parameter has to stay, or we'll lose support for all that.

Perhaps UrlHelper should be limited to aspects that are common to all of the supported routers, and distinct cases like Zend Router $options should be handled by a specialized UrlHelper? If so, we could drop the $routerOptions parameter.

And, of course, we want to support query params and fragment identifiers. We originally were leaning away from doing it in UrlHelper due to all the extra arguments, but if we can find a way to get rid of the two new ones, adding another or repurposing one won't be that big of a deal.

Proposal

All that being said, I propose the following:

1. Revert the $routerOptions PR
   • Neither AuraRouter nor FastRoute would use them
   • ZendRouter does use them, but developers can achieve this by wrapping around UrlHelper in their code or we can add official support for it (which doesn't need to be done right now, as it wouldn't require a major version bump).
2. Revert the $reuseResultParams PR, but support the "don't merge params" behavior via non-null $routeName. I'm suggesting non-null $routeName instead of null $routeName because otherwise it'd be impossible to generate a URI for another route without merging params.
3. Add support for query params and fragment identifier via the existing $params argument OR via new optional $queryParams and $fragment arguments.

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 Option 2, I propose we expect $params to be structured as follows:

$params = [
    '_route' => [],
    '_query' => [],
    '_fragment' => '',
];

The _route key would contain what $params contains right now. _query and _fragment are for the query params and fragment identifier, respectively.

For the 3.0.0 release, we could retain support for the old way by first checking if the _route / _query / _fragment key(s) exist, if so, we know the developer has migrated to the new way of doing it. If not, we'll assume that the entire array should be treated as $params is currently treated, and we display a deprecation notice to indicate that it still works how they have it, but 3.1.0 will remove support for the old way of doing it. I don't think removing support for the old way should require a major version bump beyond 3.0.0 because we wouldn't be changing any of the interfaces, just the behavior.

That's it. What do you all think? @xtreamwayz @glen-84 @pine3ree @weierophinney ?

[harikt]

If you consider, I am for Option 1 here. It is more easy to remember than recalling the keys for the params.

Thank you.

[pine3ree]

Hello @michaelmoussa , thank you for taking the time to summarize all the opinions in un comment.

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:

$routeName === null should trigger => reuse current route parameters. I think it's easier to remember that null means "let the router take over".

There is another possibility (but this will add more automagic to remember): we could use this signature:

// Option 1.b
public function __invoke(
    $routeName = null,
    array $routeParams = null,
    array $queryParams = null,
    $fragmentIdentifier = ''
)

$routeParams === null => reuseRouteParams $routeParams === [] => ([] explicitly assigned by the developer) do not substitute route parameters (this of course will be only valid if the route pattern has only optional parameters placeholders). Checking identity with null is also fast, so if the argument is not null it has to be an array since the signature requires it. we could test identity with null and [] (or just empty() after the null test).

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 .

[glen-84]

_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 $routeName !== null:

{{ 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 $routeParams = null to indicate re-use, however we'd probably need an option to include the result params if needed:

{{ path(null, {}) }}

As routeParams are non-null, result params would not be included by default. To force them to be included, you'd use an option:

{{ 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:

• resultParams: true to include result params (before route params [i.e. routeParams override result params]) if you've specified route params.
• ...

The options parameter can be used for any future "extensions". It could also be used to pass router options to Zend Router or any other router implementation.

Would this work? Are there any use cases that are not covered?

[glen-84]

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 resultParams: false to force no re-use.

[geerteltink]

@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 _query and _fragment, I can change the twig renderer in anyway we like. You can even write your own twig extension. The only thing that would affect this is calling the UrlHelper directly, for example inside an action.

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}) }}

[glen-84]

@xtreamwayz,

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.

[pine3ree]

@glen-84 I believe that the url helper's signature should not depend on external libraries, but should be an internal design choice. I use twig, but i still prefer using php renderers such as zend-view / plates. Anyway, as @xtreamwayz poined out, we can let/leave the twig path extension function behave like it does now, and we can just change the url extension function for building complete urls.
twig/zend-view/plates are all very flexible, you can add all the helpers you need :-)

kind regards, maks

[glen-84]

@pine3ree,

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.

[pine3ree]

Hello @glen-84,

what i was suggesting is that:

since until now twig path function have dealt only with paths (not urls) building process we could leave path alone and rewrite the url function

{{ url('routeName', {rParam: 1}, {qParam: 2}, 'fragment', {resultParams: true}) }}

as of now the url function does the same of the path function see line 91/92. (*not true see comment update below)

If we do not modify the path function we will keep the older functionality untouched, while the url-building functionality would be implemented in the url functions.

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(...) }} need an url => use {{ url(...) }}

@glen-84 update 1: no sorry, they are not the same: the url function generate a full server url !!!! taking it back :-) this is why the names are so important in software development. sorry, again.

@glen-84 update 2: now that I have taken a deeper look, the twig function names defined in in zx-twigrenderer can be a little confusing:

• we have a url function generating an absolute path given a route name and params
• we have an absolute_url function generating an absolute path given a relative path
• we have a path function that is actually building what the name suggests a relative path for given route name and params.

[glen-84]

@pine3ree,

If I'm interpreting this correctly, then it's technically an absolute-path reference, but I think it's probably fine to just keep the tag name as path.

Also, url and absolute_url don't generate absolute paths, they generate full URLs or "absolute URLs".

[pine3ree]

@glen-84 Now that I have thought a little more about it the current names make perfect sense in the current situation (without the query/fragment part), while path can be a little confusing if we consider/add query string generation capabilities. But I am not suggesting to change the current names of the twig renderer functions :-) I agree on the fact that what they generate urls because the result is wrapped by serverUrlHelper. They generate absolute urls but not complete urls, since the underlying urlHelper still does not have this feature we are discussing here. But they names will still be valid names if/when they will be query/fragment aware. I am not sure if a further function named relative_url could be useful to distinguish between path and path + query + fragment generation. So far path can only generate paths using a route name and an array of parameters, so weather we add query string parameters into it or we add another template function current applications will not need to be changed.

kind regards

[glen-84]

@pine3ree If I'm not mistaken, you can already use path() to create an absolute-path reference, if your route URL includes a query string and/or fragment.

I don't really see the need for a separate function/tag.

[pine3ree]

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.

absolute_url instead is a wrapper around ServerUrlHelper::generate($path).

1. If the ServerUrlHelper instance has an $uri instance property it will generate an uri reference of type URI (with scheme and host provided by $uri). if the $path parameter contains a query/fragment parts those will parsed and if correctly recognize the return value would be the string representation of a cloned $uri with properties matching the parsed values... so they will be included unless $path is not formatted correclty.
2. Otherwise it will return an absolute-path reference. if the $path parameter contains a query/fragment parts those will be included.

so absolute_url can return both URIs and absolute-path references (I haven't tested if it can return network-path references with scheme-less $uri)

Since I haven't used twig with zx I cannot really help in choosing weather to use path to build both paths and relative-refs. My doubt (excluding my confusion about path, url implementation in on of my previous comments :-) ) is only about the convenience of having a function name closer to the results it provides since we can make that distinction in zend-expressive without introducing bc breaks. To me "url" sounds closer to the meaning of URI-reference than only the path part of it, but I believe it's just a matter of personal taste/habit. It's not eally such a big deal though. I used and still use slim path_for slim twig template function and it builds url with query strings.

kind regards and thank You for sharing your opinion.

[geerteltink]

@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: $reuseResultParams !== $routeName === null. I don't know when someone would use $routeName = null. In my templates I always have a routename, especially since my templates don't know the current route result. So I think the $reuseResultParams should stay. Having a null $routeName in the template also makes a partial template not reusable. But please correct me if I'm wrong.

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 may have this signature:

$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?

[glen-84]

// 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)

[michaelmoussa]

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 dev-3.0.0 branch and to allow for query params and fragment identifiers to be specified in the method call. All, ideally, without a method signature a mile long.

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 dev-3.0.0 release

• $routeName
  • Behaves exactly as it does now
• $params
  • Will have the following keys: route, query, fragment
  • The contents of route will behave exactly as $params does now
  • query and fragment will append query params and a fragment identifier, respectively
• $options
  • Will have the following keys: router, reuse_result_params
  • The contents of router will be used for what $routerOptions is used now
  • The contents of reuse_result_params will be used for what $reuseResultParams is used for now

SO...

• [x] Everything in current dev-3.0.0 is still supported
• [x] We can define query params
• [x] We can set a fragment identifier
• [x] Method signature is not a mile long with dozens of parameters

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 UrlHelper or wrap around it and have your __invoke($routeName = null, $routeParams = [], $queryParams = [], $fragmentIdentifier = null, $etc = ...) proxy to the canonical, supported version of the helper.

Finally (just in case), yes, I know that:

$urlHelper(..., ['route' => ['foo => 'bar'], 'query' => ['bar' => 'baz'], 'fragment' => 'whee')

is longer than:

$urlHelper(..., ['foo => 'bar'], ['bar' => 'baz'], 'whee')

BUT, both of them are longer than:

$urlHelper(..., ['foo' => 'bar']) . '?foo=bar#whee

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. :)

[pine3ree]

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 query, fragment keys to $options as well.... just an idea. In this way we would not have to add key checking for the transition/deprecation period.

So let's wait from other people's opinions and maybe @weierophinney's.

Thanks and kind regards

[michaelmoussa]

@pine3ree my reason for putting query and fragment in $params rather than adding them to $options was to create a separation - $params are for things whose values will appear in the resulting URL in some form (either as substitutions in the relative path, in the query string, or as a fragment identifier), whereas $options are for things that will affect how the URL is generated (are we reusing / merging with existing route params? are we modifying the behavior of the underlying router by passing some arguments to it? etc)

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.

[glen-84]

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?

[geerteltink]

@glen-84 Yes, I'll leave a message here once I created the PR. I'd like your feedback before merging it.