Add query parameters and fragment support

[geerteltink]

This PR adds support for query parameters and fragments as discussed in zendframework/zend-expressive#325.

The invoke function has now this signature:

public function __invoke($routeName = null, $params = [], $options = []);

• $routeName
  Behaves exactly as it does now
• $params
  Can have the following keys: route, query, fragment.
  • route is an array containing the route parameters
  • query appends query params
  • fragment appends a fragment identifier, respectively
• $options
  Can have the following keys: router, reuse_result_params.
  • router must be an array containing the router options
  • reuse_result_params is a boolean to indicate if the current RouteResult parameters will be used, defaults to true.

[geerteltink]

@michaelmoussa I have added the PR. I have some mixed feeling about this. I know it's already discussed too much and don't want to cause more headaches. I had some doubts while implementing this.

Right now it has a fallback for the v1 and v2 behavior. So basically you can upgrade to v3 without many issues. However since we using the $params also for query and fragment, the route, query and fragment should become reserved keywords and can't be used in v1 and v2 routes if you install version 3.

I can only think of some solutions:

1. remove backwards compatibility but that involves a lot of rewrites for people who used the UrlHelper directly.
2. Move the query and fragment to the options array. This way $params behaves like it always did, and it feels a bit cleaner.
   3, Leave it as it is and make them reserved keywords.

Ok, back to the PR...

The tests are in place but I'm not sure if there are more tests needed.

Finally, what to do if a path contains already a query? I don't know if this is even possible with a router, or we should even care about this and make it a user responsibility. The question was raised here.

[pine3ree]

@xtreamwayz @michaelmoussa
The same doubt (2) came to my mind (zendframework/zend-expressive#325 (comment)).

On one side it's seem natural to have route, query, fragment inside a common $params array as @michaelmoussa suggested.
On the other side leaving the $params array as it is has a few benefits: matching the router generateUri signature, no bc break and shorter calls for simpler urls.
If the generateUri is going to be changed to generateUri(($name, array $substitutions = [], array $options = []) this will still match the new UrlHelper signature where the router intance won't be passed query/ fragment with $options as they belong to the helper.

sorry guys for bring it up again, but i believe that it's better to expose any doubt (they usually come for a reason).

about the other question:
why should we pass a complete url (with query string) inside the UrlHelper. Isn't it purpose to build a path (and from now on a complete url with qeury string etc). The $routeName should (I'd dare say MUST) be chosen from the null value and one of the user define routes' names in configuration.
Or did I just completely miss the subject of your question?

kind regards

[geerteltink]

@pine3ree

Isn't it purpose to build a path (and from now on a complete url with qeury string etc). ... Or did I just completely miss the subject of your question?

This is what I was thinking as well. This PR adds the option to add the query and fragment in a proper way. So there is no need to support any hackisch way to accomplish the same.

[glen-84]

I tried adding a query string to a FastRoute path the other day, and there were no errors. This means that someone might have a path like /a/b/c?fixed=value, and then try to append or merge an additional parameter, and end up with /a/b/c?fixed=value?page=1.

The same is probably true for the fragment, and conceivably even other parts of the URL, which is why I used parse_url and parse_str in my example.

Again, I don't currently have this requirement, but others may.

---

FWIW, I think moving query and fragment to the options array might make sense.

[pine3ree]

Hello @glen-84,
I am not sure if you are referring to path used as $routeName parameter in UrlHelper. If so:
the purpose of the UrlHelper is to build a path given a defined route-name and parameter substitutions.

You can define route names like this:

• user.list
• user.add

Route names are merely string identifiers (array keys) and using this representation (dots instead of forward slashes) makes it more clear that $routeName is not a path (i also use forward slashes as a habit)

UrlHelper so far is a proxy to RouterInterface::generateUri($name, array $substitutions = []) with a few addons features like prepending a basePath and reuse current matched route/params.

ServerUrlHelper instead is a completely different helper: it generates a full url from a given path.

Pls let me know if you are referring to something else.

kind regards
maks

[glen-84]

@pine3ree,

I'm not referring to the route name, I'm referring to the route path. For example:

        [
            'name' => 'api.ping',
            'path' => '/api/ping?fixed=value', // *** NOTE THE QUERY STRING ***
            'middleware' => App\Action\PingAction::class,
            'allowed_methods' => ['GET'],
        ]

The above is valid IIRC, but will not play nicely with the code in this PR.

[pine3ree]

@glen-84
Oh, i see what you mean now.
But router should not deal with query string, they match paths and build paths without query string. In particular, from the Usage section of FastRoute

//...
// Fetch method and URI from somewhere
$httpMethod = $_SERVER['REQUEST_METHOD'];
$uri = $_SERVER['REQUEST_URI'];

// Strip query string (?foo=bar) and decode URI
if (false !== $pos = strpos($uri, '?')) {
    $uri = substr($uri, 0, $pos);
}
$uri = rawurldecode($uri);

$routeInfo = $dispatcher->dispatch($httpMethod, $uri);
//...

you can see that the query string is stripped away before the matching happens.

kind regards

added later:

when you define a route in the config file the path spec is supposed to be composed of segments with or without optional placeholders that wil be parsed (if matched) into psr-7 request attributes. So a route is not supposed to be defined with a path containing a query string.
request query params are supposed to be handled inside web controllers' code (middleware, controller methods etc)

[pine3ree]

@glen-84
using more technical terminology

the router deals in parsing and building the absolute-path of a relative-reference of type absolute-path reference, so it parses and builds paths like:

path-pattern: /user[/{action}[/{id:\d+}]]

handled absolute-paths

/user (with a default implicit action index)
/user/read/123
/user/update/456

the absolute-path is considered related to the application. the real absolute-path could have a base path prepended, but this is handled outside the router so that the application can be moved easilty inside directories without the need of redefining all the routes.

kind regards

[glen-84]

@pine3ree,

From my understanding, a router is just something that looks at the request to determine which code to run. The ZF documentation confirms this:

Routing can utilize other portions of the request URI or environment as well – for example, the host or scheme, query parameters, headers, request method, and more.

The ZF router also allows you to specify a query and fragment part in the options passed to the assemble method. Whether or not ZE would allow passing query params in this way is besides the point – the fact is that it is possible that one of the router implementations might return a URI with a query string or fragment.

I'm not concerned with what is correct or what is intended (I don't have query strings in my own route paths), I'm only pointing out that the current code may return invalid URIs as it does not currently account for this possibility.

[pine3ree]

Hello @glen-84
I agree on the fact the a perfect router should examine the whole request and I am working on a project that does just that.
But beside the fact zend-router Query segment was deprecated since v.2.3 and removed in v3, the zx-zendrouter implementation uses a TreeRouteStack and the path parameter used in ze routes definitions would be what in zend-router routes definitions is under the route key. I have never seen passing query strings inside the route key: the query parameters were retrieved from the Request instance.

The old zf Query route also was used as a child route under a Literal or Segment route and when assembling the url you needed to add an ending "/query" part to the route name.

So why should we use query string inside path in a route definition in the first place?
As far as I know all 3 current implementations supports route definitions with path (route/pattern or whatever is called) similar to the Literal and Segment route in zend framework.

(All other features such as hostname matching if available are implemented differently, so we need to have a common ze way of passing them properly to the used implementation via an the options key
for instance but keeping a simple standard route definition)

[glen-84]

You're missing the point.

Add this to your routes:

[
    'name' => 'test',
    'path' => '/xxx?a=b',
    'middleware' => /* insert something here */,
    'allowed_methods' => ['GET']
]

Then generate a URI (example with Twig):

{{ path('test') }}

It will output (at least with FastRoute):

/xxx?a=b

Is it right to do this? I don't care (I don't do it).
Is it possible to do this? Yes.
Should it be handled? That's a decision for @xtreamwayz and @michaelmoussa to make.

[pine3ree]

Hello @glen-84 ,

I believe that the term path to be self-explanatory to the developer, but the only way i can think to make the developer avoid that is by checking the format of the path via regex.

As of now I think this is possible only in the router implementation (the injectRoute() method at least this is the case for zendrouter and fastroute implementation).
Handling this in the UrlHelper would be not right from my point of view since any part of a route definition should not be altered after it's injected.

kind regards

[michaelmoussa]

I have some mixed feeling about this. I know it's already discussed too much and don't want to cause more headaches. I had some doubts while implementing this.

A lot of good points have been raised, and the more I look at the continued discussion, the more I think that the approach I encouraged might not be the best one.

Specifically:

since we using the $params also for query and fragment, the route, query and fragment should become reserved keywords and can't be used in v1 and v2 routes if you install version 3.

and

leaving the $params array as it is has a few benefits: matching the router generateUri signature, no bc break and shorter calls for simpler urls.

The motivation to put these items into the $params array was to reduce the number of parameters in the method signature. Other than length, are there any strong arguments against this signature?

public function __invoke(
    $routeName = null,
    array $routeParams = [],
    array $queryParams = [],
    $fragmentIdentifier = '',
    array $options = []
)

I mean yeah it's ugly and long etc, but it was one of the first options that were discussed and all of the arguments are optional, so it's only as ugly as you need it to be for the URL you're trying to generate.

If we bite the bullet on appearance and go with the above, the benefits are as follows:

1. Usage of the first two parameters - $routeName and $routeParams (previously $params) does not change at all.
2. Zero BC break for anybody who is simply using the existing UrlHelper to generate routes - only folks who have extended the class will have minor changes to make.
3. No need for reserved keys in a $params array.
4. The signature shouldn't have to grow further, since any new potential parameters in the future that don't have anything to do with the URL or route itself can just be crammed into $options.

As is the case now, the first two parameters for $options would be router for additional router-specific configuration and reuse_result_params to replace $reuseResultParams currently in dev-3.0.0.

What do you think?

[michaelmoussa]

Is it right to do this? I don't care (I don't do it).
Is it possible to do this? Yes.
Should it be handled?

I'm generally of the opinion that one should take reasonable measures to protect developers from making common mistakes, but as @pine3ree said, the term path should be self-explanatory to a developer. /paths/look/like/this and shouldn't have query strings in them. I don't think that would be a common mistake.

The worst that would happen if we left things alone and someone generated a URL for a route containing a query string in the path, and appended query params of their own, would be that they'd get a goofy looking URL with two ?s... but at least the application wouldn't 500 on the user.

If we wanted to prevent this anyway, it'd probably be something to discuss in one of the router repos, as it's not really in scope for the UrlHelper, so if any of you feel it is warranted, please open an issue and we can continue that part of the discussion there. Otherwise, we could probably leave it alone for now.

[geerteltink]

public function __invoke(
    $routeName = null,
    array $routeParams = [],
    array $queryParams = [],
    $fragmentIdentifier = '',
    array $options = []
)

Given that there can only be route parameters, query parameters and a fragment and everything else goes in options, this signature shouldn't need to change in the future. This signature is the same I have used in zendframework/zend-expressive-twigrenderer#18: It felt the best way when coding it.

@michaelmoussa I'll update this PR today.

[glen-84]

The only issue that I saw with this signature was if you just wanted a "current page" route without reusing params, it would be something like:

$url(null, [], [], null, ['reuse_result_params' => false])

That's why I was trying to make use of the 2nd parameter, but perhaps it's not the end of the world.

I'm not really against it.

[geerteltink]

@glen-84 There are always edge cases :)

@michaelmoussa PR is updated.

[pine3ree]

Hello @xtreamwayz @michaelmoussa
As I said in a related discussion I always felt the last signature suggested to be more natural and simple to read as it reflect the url parts order. No matter which the final choice was going to be i think that keeping the compatibility with the older signature and putting everything else in newer optional parameters could be a smart move.

About reusing the current route and current route parameters I feel that they can be convenient and useful features, but in some way of less importance in comparison to the other params.
Over the years in most of my projects I've needed to have controller/middleware variables for the current route name/path and for route parameters (current pageNumber in items pagination, ID of current item). I almost always passed them as template variables too (for example I commonly use a normalized routeName as css class for the body in a html response). So most of the time i explicitly set the parameters when building the url for the current route as well, because i already have them in place and makes me use the helper in the same way for all urls.
In js SPA applications where php acts as a simpler end point (dataProvider, dataManager) most of the time the routes (client app routes) are generated by javascript used the same params passed via JSON responses.
but this of course is my personal experience...

kind regards

[michaelmoussa]

@xtreamwayz I made a couple of minor tweaks - look OK to you? If so I'll merge this today.

[michaelmoussa]

@xtreamwayz ah - the test failures raise a question, actually. The tests were passing null, which worked with the plain if ($fragmentIdentifier) check, but failed when I added !== ''. Maybe if (!empty($fragmentIdentifier) || (string) $fragmentIdentifier === '0') would be best for covering the edge case without breaking the tests?

[pine3ree]

@michaelmoussa

why not just:

$reuseResultParams = !isset($options['reuse_result_params']) || (bool) $options['reuse_result_params'];

kind regards

[geerteltink]

@michaelmoussa The tests fail on this: 8753d79...feature/27#diff-c051d24dcf87b2f1bfb6a96ed36220a3R97

if ($fragmentIdentifier !== '') {
    $path .= '#' . $fragmentIdentifier;
}

I've used if ($fragmentIdentifier) { so that it doesn't add # if the fragment identifier is null or an empty string. With your change it fails for null in the tests.

[geerteltink]

@michaelmoussa The other change you made: It tested for $result first, if there is no result, it doesn't need to go through the other tests. With your change it has to do all tests always. It's a minor speed difference.

Also, I had a question about the identifier but didn't ask before. Should it be $identifier = '' or $identifier = null in the signature? For some reason I like null more than ''.
Anyway, it's the reason I tested for both.

[michaelmoussa]

@pine3ree updated - thanks! Silly oversight on my part.

[michaelmoussa]

With your change it has to do all tests always. It's a minor speed difference.

@xtreamwayz I ran a quick test locally to check (php7, xdebug turned off):

<?php

$foo = [];

$start = microtime(true);

for ($i = 0; $i < 40000; $i++) {
    !isset($foo['bar']) || (bool) $foo['bar'];
}

echo (microtime(true) - $start) * 1000;

I ended up needing around 40k executions in the loop in order to add ~1ms to the execution time (~20k on php56), so in this case I don't think we should worry about it because generating that many URLs in a single request is a far edge case.

I usually lean towards readability over tiny performance optimizations, but if you have strong feelings about it I don't mind changing it since the original version isn't totally unreadable or anything like that.

Regarding $fragmentIdentifier, I like = null in the signature as well (null meaning "there is no fragment identifier" versus '' meaning "there is a fragment identifier, but it's an empty string, and yes I know that makes no sense").

If we change the signature to say $fragmentIdentifier = null and change the condition to if ($fragmentIdentifier !== null), that fixes the broken tests as well as covering the $fragmentIdentifier = '0' edge case; however, it breaks:

1) ZendTest\Expressive\Helper\UrlHelperTest::testQueryParametersAndFragment with data set "empty-fragment" (array(), '', '')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'/foo/baz'
+'/foo/baz#'

Changing the condition to if ((string) $fragmentIdentifier !== '') would preserve all current tests and would allow the weird case of /path/to/something#0. What do you think?

[pine3ree]

Hello @michaelmoussa,
as we are dealing with testing code, wouldn't this:

if (!empty($fragmentIdentifier) || is_string($fragmentIdentifier)) { 

be less cryptic? :-)

In my opinion (string) $fragmentIdentifier === '0' should be used only if we support a $fragment parameter of type object with an implemented __toString() method, but i don't think this is the case.

@xtreamwayz @michaelmoussa
I also like a default null value for $fragment, since we cannot type-hint strings a developer could even inject it on purpose and the value null nicely represents an unset parameter.

In the implementation i would add type checking (pseudo-code)

if (null !== $fragmentIdentifier) { // quick test to check if fragment is set, most of the cases
    if (!is_string($fragmentIdentifier)) {
        // throw exception
    }
    if ('' !== $fragmentIdentifier) {
        $path .= '#' . $fragmentIdentifier;
    }
}

kind regards

[pine3ree]

@xtreamwayz @michaelmoussa
as a note to my last comment:

I am not sure this has been discussed previously
should we allow a '#' value for the $fragmentIdentifier?
otherwise we could add a

$fragmentIdentifier = ltrim($fragmentIdentifier, '#'); // updated left-trim obviously....

before checking for empty string (last if in my pseudo code)

[geerteltink]

Changing the condition to if ((string) $fragmentIdentifier !== '') would preserve all current tests and would allow the weird case of /path/to/something#0. What do you think?

Looks good to me.

I ended up needing around 40k executions in the loop in order to add ~1ms to the execution time (~20k on php56), so in this case I don't think we should worry about it because generating that many URLs in a single request is a far edge case.

I agree. It's just unneeded micro optimization.

should we allow a '#' value for the fragment?

According to the specs it must have this format: fragment = *( pchar / "/" / "?" ). I'm not sure what a pchar is but given that / and ? are specifically mentioned and # is not, I guess it's not allowed to have it in the fragment itself.

[geerteltink]

@michaelmoussa Once this is merged I'll finish the Twig helper and the tests and after that I'll add the same for zend-view and plates.

[pine3ree]

@xtreamwayz
I feel the same way about the fragment value and as you highlighted it's also in the specs.
Without type-checking in the implementation, the test condition with string type-casting would be just fine.

kind regards

[glen-84]

I'm not sure what a pchar is

fragment      = *( pchar / "/" / "?" )
pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"
unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
pct-encoded   = "%" HEXDIG HEXDIG
sub-delims    = "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "="

[michaelmoussa]

@xtreamwayz @glen-84 here is a more thorough explanation of what's allowed.

So... this? -> /^([!$&'()*+,;=._~:@\/?]|%[0-9a-fA-F]{2}|[a-zA-Z0-9])+$/

I can make a class constant out of that and throw an InvalidArgumentException if the fragment identifier is no good. We can probably leave $queryParams alone since http_build_query(...) will handle encoding, and I guess we can leave $routeParams to the routers to decide what to do with.

[geerteltink]

@michaelmoussa As you are saying the router is taking care of the path, http_build_query does the query parameters. It might be a nice addition to build a complete fail safe package.

[michaelmoussa]

@xtreamwayz done. Anything else you can think of, or are we pretty much 👍 ?

[geerteltink]

LGTM

[michaelmoussa]

Ok @xtreamwayz this has been merged into the dev-3.0.0 branch. You should be able to use "zendframework/zend-expressive-helpers": "^3.0.0-dev" for now to work on your other PRs. I need to hold off on a proper 3.0.0 release for a bit longer in order to look over a few other things and also to do some additional housekeeping in this repo, but hopefully within the next few days.

[pine3ree]

@michaelmoussa (@xtreamwayz)
I stille believe we should use is_string and allow a valid (edge-case) fragment expression like #0.
kind regards

No sorry, i mis-read the code, it's actually allowing it.

[michaelmoussa]

@pine3ree correct, it is allowing it, but when I looked I realized there's a simpler way to do it, so I've pushed an update. /cc @xtreamwayz

[pine3ree]

@michaelmoussa
right, but now it does not handle non string values. I believe we should throw an exception if the value is not a string as in #27 (comment) before the regex match.

I am not sure if we should also pre-test for the empy string with an early return or just not allow it at all.

kind regards

[geerteltink]

@pine3ree If the fragment === null it skips the fragment and if the fragment doesn't validate the regex it throws an error. The regex says that it needs to contain one or more characters from that list. So the fragment must be null or valid and there is no need to check if it's an empty string as it would classify as an invalid fragment.

[pine3ree]

Hello @xtreamwayz
But the fragment (if !== null) and the 2nd parameter of preg_match still need to be a string. preg_match will send its PHP warning if not. Maybe a (custom or std) exception would be preferrable.

update:
and of course if we want to be that thorough.... :-)

[michaelmoussa]

@pine3ree I think leaving it as-is is fine. The phpDoc says the value must be a string. If it's saying to pass a string and somebody passes an integer or an object, they shouldn't be surprised when it doesn't behave properly.

The exception that's there now is useful I think because it communicates why the parameter is not valid even though it satisfies the type requirement. The warning will tell them to fix their code, and preg_match(...) will return false, so the value won't be appended.

[pine3ree]

@michaelmoussa
ok, that's fine with me. sorry for my late answer ...the flu cought me :-)
kind regards

[glen-84]

@michaelmoussa,

Your regex is missing the hyphen character.

[michaelmoussa]

Thanks @glen-84 - fixed in dev-3.0.0