8000 [Routing] Allow inline definition of requirements and defaults by nicolas-grekas · Pull Request #26518 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing] Allow inline definition of requirements and defaults #26518

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 19, 2018
Merged

[Routing] Allow inline definition of requirements and defaults #26518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/Symfony/Component/Routing/Route.php
8000
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ class Route implements \Serializable
public function __construct(string $path, array $defaults = array(), array $requirements = array(), array $options = array(), ?string $host = '', $schemes = array(), $methods = array(), ?string $condition = '')
{
$this->setPath($path);
$this->setDefaults($defaults);
$this->setRequirements($requirements);
$this->addDefaults($defaults);
$this->addRequirements($requirements);
$this->setOptions($options);
$this->setHost($host);
$this->setSchemes($schemes);
Expand Down Expand Up @@ -123,6 +123,19 @@ public function getPath()
*/
public function setPath($pattern)
{
if (false !== strpbrk($pattern, '?<')) {
$pattern = preg_replace_callback('#\{(\w++)(<.*?>)?(\?[^\}]*+)?\}#', function ($m) {
Copy link
Contributor
@ro0NL ro0NL Mar 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does {bar<>} exactly do?

ah that's sanitized. never mind.. perhaps a test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already tested elsewhere, nothing specific to this feature

if (isset($m[3][0])) {
$this->setDefault($m[1], '?' !== $m[3] ? substr($m[3], 1) : null);
}
if (isset($m[2][0])) {
$this->setRequirement($m[1], substr($m[2], 1, -1));
}

return '{'.$m[1].'}';
}, $pattern);
}

// A pattern must start with a slash and must not have multiple slashes at the beginning because the
// generated path for this route would be confused with a network path, e.g. '//domain.com/path'.
$this->path = '/'.ltrim(trim($pattern), '/');
Expand Down
16 changes: 16 additions & 0 deletions src/Symfony/Component/Routing/Tests/RouteTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,22 @@ public function testSerialize()
$this->assertNotSame($route, $unserialized);
}

public function testInlineDefaultAndRequirement()
{
$this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', null), new Route('/foo/{bar?}'));
$this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', 'baz'), new Route('/foo/{bar?baz}'));
$this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', 'baz<buz>'), new Route('/foo/{bar?baz<buz>}'));
Copy link
Contributor
@Tobion Tobion Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we make it so that it does not matter if you specify a default or regex first?
So '/foo/{bar?baz<.*>}' == '/foo/{bar<.*>?baz}'

This seems more user-friendly as you don't need to remember the order. Also this syntax is not feature-complete anyway due to missing escaping. So if you really need to specify a default like baz<buz> then just use the explicit config. But having such a default is much less likely than a user mixing up the order.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #26481 (comment)

Copy link
Member Author
@nicolas-grekas nicolas-grekas Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having two ways to express things just creates more confusion.
Having only one order means everyone will write these in exactly the same way, thus less WTF when switching projects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally I agree consistency is better. But I also want to prevent bugs in the first place. So when we do not accept both ways, it would be cleaner to throw an exception if you do it wrong. I cannot believe somewhat actually uses a default like baz<buz> in a URL.

Copy link
Member Author
@nicolas-grekas nicolas-grekas Mar 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot believe somewhat actually uses a default like baz in a URL

This is not a reason to technically forbid it. Symfony devs have been proved surprising many time :)

Copy link
Contributor
@Tobion Tobion Mar 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not forbidden. You just need to use the explicit syntax.

Symfony devs have been proved surprising many time

Yes they will be surprised with unexpected behavior.

$this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', 'baz'), new Route('/foo/{bar?}', array('bar' => 'baz')));

$this->assertEquals((new Route('/foo/{bar}'))->setRequirement('bar', '.*'), new Route('/foo/{bar<.*>}'));
$this->assertEquals((new Route('/foo/{bar}'))->setRequirement('bar', '>'), new Route('/foo/{bar<>>}'));
$this->assertEquals((new Route('/foo/{bar}'))->setRequirement('bar', '\d+'), new Route('/foo/{bar<.*>}', array(), array('bar' => '\d+')));
$this->assertEquals((new Route('/foo/{bar}'))->setRequirement('bar', '[a-z]{2}'), new Route('/foo/{bar<[a-z]{2}>}'));

$this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', null)->setRequirement('bar', '.*'), new Route('/foo/{bar<.*>?}'));
$this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', '<>')->setRequirement('bar', '>'), new Route('/foo/{bar<>>?<>}'));
}

/**
* Tests that the compiled version is also serialized to prevent the overhead
* of compiling it again after unserialize.
Expand Down
0