Description
Symfony version(s) affected
7.3.0
Description
I found an odd behavior (in my opinion) while looking through the Routing component.
Normal behavior
Supposed you have a route to /test/{foo}, with the following requirement : 'foo' => '\w'.
Related code : $route = new Route('/test/{foo}', [], ['foo' => '\w+']);
In that case:
/test/fooworks/test/foo-fails, because the dash is not permitted by the requirement./testfails because no default value is set
All good here.
Odd behavior
Now, let's tweak the route definition, and add a default value : 'foo' => 'foo-'. Notice that the default value does not match the requirement.
Code : $route = new Route('/test/{foo}', ['foo' => 'foo-'], ['foo' => '\w+'], []);
With this route definition:
/test/fooworks/test/foo-fails, because the dash is not permitted by the requirement./testworks now, that's the point of the default value. BUT : the value of the parameterfoois set to its default value 'foo-', event though it does not meet the requirement!
What do we do with this?
Is this last behaviour (authorizing the default value to not respect the requirement) really what we want ?
- If so: I'll submit a PR in order to add testing to enforce this behavior
- If not: there's going to be some refactoring needed, and honestly I'll need some help for it 😄
Let's discuss !
How to reproduce
Add these tests to UrlMatcherTest.php in order to reproduce the behavior / the issue:
public function testParameterWithRequirementWithoutDefault()
{
$collection = new RouteCollection();
// Route with requirement for 'foo' parameter and without default value
$route = new Route('/test/{foo}', [], ['foo' => '\w+']);
$collection->add('test', $route);
$matcher = $this->getUrlMatcher($collection);
// Case 1 : Route to '/test/foo' : works as expected
$result = $matcher->match('/test/foo');
$this->assertSame('test', $result['_route']);
$this->assertSame('foo', $result['foo']);
// Case 2 : Route to '/test/foo-' : fails as expected
try {
$matcher->match('/test/foo-');
} catch (ResourceNotFoundException $e) {
$this->assertStringContainsString("No routes found", $e->getMessage());
}
// Case 3 : Route to '/test' : fails as expected
try {
$matcher->match('/test');
} catch (ResourceNotFoundException $e) {
$this->assertStringContainsString("No routes found", $e->getMessage());
}
}
public function testParameterWithRequirementWithDefault()
{
$collection = new RouteCollection();
// Route with requirement for 'foo' parameter and with invalid default value
$route = new Route('/test/{foo}', ['foo' => 'foo-'], ['foo' => '\w+']);
$collection->add('test', $route);
$matcher = $this->getUrlMatcher($collection);
// Case 1 : Route to '/test/foo' : works as expected
$result = $matcher->match('/test/foo');
$this->assertSame('test', $result['_route']);
$this->assertSame('foo', $result['foo']);
// Case 2 : Route to '/test/foo-' : fails as expected
try {
$matcher->match('/test/foo-');
} catch (ResourceNotFoundException $e) {
$this->assertStringContainsString("No routes found", $e->getMessage());
}
// Case 3 : Route to '/test' : works - is this expected ?
$result = $matcher->match('/test');
$this->assertSame('test', $result['_route']);
$this->assertSame('foo-', $result['foo']); // Foo is set to 'foo-' : do we allow this ?
}Possible Solution
No solution proposed, only a discussion for now:
- Is this the behavior we want? In that case, there is nothing more to do here, except for me to submit a PR with the related tests to ensure this behavior is always enforced. I'll also add this to the docs with another PR there.
- Does this behavior seem odd? Then, there's some refactoring to do ...
Additional Context
Recent activity related to this subject: