Use the standard route parser from FastRoute #32

geerteltink · 2017-03-22T18:21:49Z

Instead of using a custom regex, nikic suggests to use parsed results:
nikic/FastRoute#66 (comment)

This also throws errors for missing mandatory parameters (#30).

This is a work in progress and should be optimized and tested extensively.

Use FastRoute\RouteParser\Std for generating uri's
Remove regex related code
Add test for Returning empty path with missing mandatory route param #30
Throw an exception if value doesn't match the regex for a given parameter
Test in real life appplications

Instead of using a custom regex, nikic suggests to use parsed results: nikic/FastRoute#66 (comment) This also throws errors for missing mandatory parameters (#30).

weierophinney

This all makes sense to me! 👍

michaelmoussa · 2017-03-22T18:58:21Z

src/FastRouteRouter.php


-        return $path;
+        throw new LogicException('Too many parameters given');


We should add a test for this as well.

This is also not covered by a test, but it wasn't the result of any code you added. Might as well include a test for it though if it won't be much extra effort.

On second thought... if the format is /user/{id} and I call $this->url('user', ['id' => 123, 'extra' => 'extra']), then the helper has enough information to produce /user/123, but I'd get an exception because of extra - does having too many parameters necessarily need to be cause for an error?

Agreed with @michaelmoussa here. Passing parameters that will not be used should not be exceptional behavior, so long as the expected parameters were discovered.

(It may be interesting to raise an E_USER_NOTICE indicating what additional parameters were passed, but ignored, though I think such behavior should be toggleable, and disabled by default.)

We have to allow optional parameters since they are appended to the query (by the UrlHelper ???).

michaelmoussa · 2017-03-22T19:21:16Z

src/FastRouteRouter.php

+
+                // Placeholder in the route
+                if ($paramIdx === count($substitutions)) {
+                    throw new LogicException('Not enough parameters given');


I think we could make this exception more helpful by indicating how many parameters are required and which were missing.

Each $part that is an array appear to be tuples containing the key and a valid regex. If prior to beginning the substitutions we extract all the keys, we'll know the total number of parameters required and what their keys are. $substitutions already tells us what we've managed to match, so something along these lines would do the trick (not tested):

throw new LogicException(sprintf('Expected parameter values for [%s], but only found [%s]', implode(',', $needed), implode(',', array_keys($substitutions))));

... where $needed would be that list of all the required parameter names. So instead of Not enough parameters given, we'd see Expected parameter values for [id,foo,bar], but only found [id] or something like that.

Thoughts @xtreamwayz @weierophinney ?

Clearly I rushed through the review a bit hastily!

Yes, agreed with @michaelmoussa — it's far more helpful to the developer to know what was expected, and what was actually provided, as they can then figure out what they missed in their code.

michaelmoussa · 2017-03-22T19:28:03Z

src/FastRouteRouter.php

-        foreach ($segs as $n => $seg) {
-            if (strpos($seg, '{') !== false) {
-                if (isset($segs[$n - 1])) {
+                if (! isset($substitutions[$part[0]])) {
                    throw new Exception\InvalidArgumentException(
                        'Optional segments with unsubstituted parameters cannot '


The messaging here is a bit confusing as well. Assuming route format of /user/{id}/{foo}/{bar} named user, calling $this->url('user', ['id' => 123, 'foo' => 456, 'not_bar' => 789]) would produce this exception (correct number of parameters, but wrong keys). But none of those segments are optional, so "Optional segments with unsubstituted parameters cannot contain segments with substituted parameters when using FastRoute" doesn't really tell us what's wrong.

I think we should use a message similar to what @michaelmoussa proposed earlier: Expected parameter values for [id,foo,bar], but received [id,foo,not_bar]. Again, this would demonstrate to the developer what occurred so they have a reasonable chance of correcting the situation in the calling code.

michaelmoussa

This PR definitely addresses issue #30 but did prompt me to notice a few things that were concerning. Would like to hear you guys' opinions on it to see if the concerns are valid.

geerteltink · 2017-03-23T15:42:35Z

I'm working on this but I'm running into some issues to get optional parameters working. The problem is that the StandardParser is returning an array with possible routes.

Route '/test[/{name}[/{id:[0-9]+}]]' returns:

[
    ['/test'],
    ['/test/', ['name', '[^/]+']],
    ['/test/', ['name', '[^/]+'], '/', ['id', '[0-9]+']],
]

That code that I added loops through all returned routes until it has found a solution that has the exact parameters.

Thinking about this... What I could try is reverse that array (going from the most parameters up to the least. And compare the required parameters with the substitution parameters. Once they all exist, we have a match.

Also currently there isn't a regex check. So if only digits are allowed, even characters are accepted.

geerteltink · 2017-03-23T17:48:09Z

As explained before, this is a revamp of the logic to generate the uri. It goes in reverse order through the parsed routes, starting with the longest possible route to the shortest. It checks if all required parameters are available. If not, it skips to the next (shorter) route.

Major changes:

Tests now against all standard FastRoute tests
It will now throw an exception if a parameter value doesn't match it's regex.
It throws an exception if the minimum required parameters are not set and which needs to be set.
It doesn't throw an exception anymore for incomplete substitutions, it just skips the route and tries the next:
```
// `'/foo[/{param}[/optional-{extra}]]'`
$router->generateUri('foo', ['extra' => 'segment']);
```
Before this would throw an exception, now it would return /foo, since that is the minimum allowed route with no parameters.

Because of the removal of the incomplete substitutions exception and the new exception thrown for values not matching their regex, I would mark this as a BC break.

/cc @weierophinney @michaelmoussa

geerteltink · 2017-03-23T18:06:11Z

src/FastRouteRouter.php

-            );
-            $path = preg_replace($pattern, $value, $path);
-        }
+        $routeParser        = new RouteParser();


Should this be stored for re-use in a private property?

@xtreamwayz It appears to be stateless, so I think that'd be OK. Maybe do $routeParser = $this->getRouteParser() and add a private getRouteParser() for lazy instantiation and setting the private property?

geerteltink · 2017-03-23T18:08:11Z

src/FastRouteRouter.php

+        // No valid route was found; explain which minimal required parameters are needed
+        throw new Exception\InvalidArgumentException(sprintf(
+            'Expected parameter values for at least [%s], but received [%s]',
+            implode(',', $requiredParameters),


Enforce an array here? However $requiredParameters should always be an array, or a route is returned... I think.

Perhaps instead of using $requiredParameters in this way, keep a running list of the different valid parameter combinations? So in the case of your earlier example, given '/test[/{name}[/{id:[0-9]+}]]', the exception message would be:

"Expected any combination of: [{name,id}, {name}, {}], but received {foo,bar,baz}"

So as you'd go through the required parameters for the potential matching routes, you'd record them in a $validParameterSets (or something) array in order to report the exception if one arises.

Returning all possible combinations means that it always has to go through all possible combinations for that route. Plus is might also get confusing when return all combinations. Besides that, it's up to the developer to figure it out for complex routes. But what I can do is adding the route name to the exception so he can easily track what route is throwing the error.

That's a good balance I think... at least there'll be an indication of where to look.

michaelmoussa · 2017-03-25T01:36:13Z

src/FastRouteRouter.php

+     *
+     * @return array|bool
+     */
+    private function hasRequiredParameters(array $parts, array $substitutions)


@xtreamwayz What would you think of missingRequiredParameters(...) here instead?

In the calling method where the return value is saved in $requiredParameters, at first reading I expected that $requiredParameters would contain a list of the parameters that were required to match that route, when in fact it was either boolean true (which I thought to mean "yes, there are required parameters") or an array (which I thought to mean "here are all the required parameters").

I think $missingParameters = $this->missingParameters($parts, $substitutions) might be a little more obvious, as $missingParameters = false would be taken to mean "no, there are no parameters missing" and $missingParameters = ['foo', 'bar'] would be taken to mean "the parameters that are missing are 'foo' and 'bar'".

Done. The function is now called missingParameters() and always returns an array. If there are any parameters missing it will return those, otherwise it returns an empty array.

michaelmoussa · 2017-03-25T15:13:43Z

Because of the removal of the incomplete substitutions exception and the new exception thrown for values not matching their regex, I would mark this as a BC break.

I'm a little on the fence about that... changes in a method signature or in an interface are obvious BC breaks, but a change in behavior that wouldn't necessarily break existing implementations are a bit of a gray area. If we were doing it as an enhancement then I'd lean towards BC break, but it seems more like a bug fix to me.

"It will now throw an exception if a parameter value doesn't match it's regex" (it should have probably been doing so in the first place)
"It throws an exception if the minimum required parameters are not set and which needs to be set" (also should have probably been doing so in the first place)
"It doesn't throw an exception anymore for incomplete substitutions, it just skips the route and tries the next" (same as above)

geerteltink · 2017-03-25T15:16:47Z

@michaelmoussa makes sense. Bugfix works for me as well.

michaelmoussa · 2017-03-25T15:29:20Z

src/FastRouteRouter.php

@@ -304,7 +304,8 @@ public function generateUri($name, array $substitutions = [], array $options = [

        // No valid route was found: list minimal required parameters
        throw new Exception\InvalidArgumentException(sprintf(
-            'Expected parameter values for at least [%s], but received [%s]',
+            'Route `%s` expects al least parameter values for [%s], but received [%s]',


al -> at (typo)

michaelmoussa · 2017-03-25T15:31:42Z

Great work on the changes @xtreamwayz - they appear to cover all the concerns I had. I'll mark my review approved. Do you want to give it another once-over before it gets merged, @weierophinney ?

geerteltink · 2017-03-25T15:33:28Z

@michaelmoussa I'd like to test this in production applications as well before merge. I'll do that later today or Monday morning.

michaelmoussa · 2017-04-22T20:19:23Z

Do you think this is ready to merge and release, @xtreamwayz? Anything I can do to help move it along?

@michaelmoussa

per @michaelmoussa

geerteltink · 2017-04-28T09:20:09Z

@michaelmoussa I've tested this on two production sites and the tests are passing so I guess we can merge it.

Close #32, fixes #30

Use the standard route parser from FastRoute

6b0b7ec

Instead of using a custom regex, nikic suggests to use parsed results: nikic/FastRoute#66 (comment) This also throws errors for missing mandatory parameters (#30).

geerteltink requested review from weierophinney and michaelmoussa March 22, 2017 18:21

geerteltink added the enhancement label Mar 22, 2017

weierophinney mentioned this pull request Mar 22, 2017

Returning empty path with missing mandatory route param #30

Closed

weierophinney approved these changes Mar 22, 2017

View reviewed changes

michaelmoussa reviewed Mar 22, 2017

View reviewed changes

michaelmoussa suggested changes Mar 22, 2017

View reviewed changes

geerteltink added 4 commits March 23, 2017 18:28

Rework of integrating the standard route parser from FastRoute

73e330b

Add standard FastRoute tests for Uri generation

33403ba

Fix code standard violation

8edc7e5

Fix docheaders

a423933

geerteltink added 2 commits March 23, 2017 19:00

Update dockblocks and comments

7c04e03

Update dockblocks and comments for hasRequiredParameters

7dd04c2

geerteltink commented Mar 23, 2017

View reviewed changes

michaelmoussa reviewed Mar 25, 2017

View reviewed changes

geerteltink added 2 commits March 25, 2017 16:22

Rename required parameters to missing parameters for clarification

849873d

Add route name to the missing parameters exception

744be0a

michaelmoussa reviewed Mar 25, 2017

View reviewed changes

Fix typo

2479fb0

Fix all typos!!!

fc88301

michaelmoussa approved these changes Mar 25, 2017

View reviewed changes

geerteltink mentioned this pull request Mar 25, 2017

PSR-11 Container Interface #33

Merged

geerteltink modified the milestone: 2.1.0 Mar 31, 2017

geerteltink added 2 commits April 28, 2017 10:57

Fix typo and docblock formatting

b9fed35

Improve value substitute regex

1ac6fd7

per @michaelmoussa

geerteltink merged commit 1ac6fd7 into zendframework:develop Aug 11, 2017

geerteltink added a commit that referenced this pull request Aug 11, 2017

Add changelog for #32

bcc9437

geerteltink added a commit that referenced this pull request Aug 11, 2017

Merge branch 'pr/32' into develop

51d8f75

Close #32, fixes #30

geerteltink deleted the feature/std-url-generator branch August 11, 2017 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the standard route parser from FastRoute #32

Use the standard route parser from FastRoute #32

geerteltink commented Mar 22, 2017 •

edited

Loading

weierophinney left a comment

michaelmoussa Mar 22, 2017

michaelmoussa Mar 22, 2017

michaelmoussa Mar 22, 2017

weierophinney Mar 22, 2017

geerteltink Mar 23, 2017

michaelmoussa Mar 22, 2017

weierophinney Mar 22, 2017

geerteltink Mar 23, 2017

michaelmoussa Mar 22, 2017

weierophinney Mar 22, 2017

geerteltink Mar 23, 2017

michaelmoussa left a comment

geerteltink commented Mar 23, 2017

geerteltink commented Mar 23, 2017 •

edited

Loading

geerteltink Mar 23, 2017

michaelmoussa Mar 25, 2017

geerteltink Mar 23, 2017

michaelmoussa Mar 25, 2017

geerteltink Mar 25, 2017

michaelmoussa Mar 25, 2017

michaelmoussa Mar 25, 2017

geerteltink Mar 25, 2017

michaelmoussa commented Mar 25, 2017

geerteltink commented Mar 25, 2017 •

edited

Loading

michaelmoussa Mar 25, 2017

michaelmoussa commented Mar 25, 2017

geerteltink commented Mar 25, 2017

michaelmoussa commented Apr 22, 2017

geerteltink commented Apr 28, 2017


		return $path;
		throw new LogicException('Too many parameters given');

Use the standard route parser from FastRoute #32

Use the standard route parser from FastRoute #32

Conversation

geerteltink commented Mar 22, 2017 • edited Loading

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelmoussa left a comment

Choose a reason for hiding this comment

geerteltink commented Mar 23, 2017

geerteltink commented Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelmoussa commented Mar 25, 2017

geerteltink commented Mar 25, 2017 • edited Loading

Choose a reason for hiding this comment

michaelmoussa commented Mar 25, 2017

geerteltink commented Mar 25, 2017

michaelmoussa commented Apr 22, 2017

geerteltink commented Apr 28, 2017

geerteltink commented Mar 22, 2017 •

edited

Loading

geerteltink commented Mar 23, 2017 •

edited

Loading

geerteltink commented Mar 25, 2017 •

edited

Loading