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.

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

[Routing] Make important parameters required when matching #29770

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@vudaltsov
Copy link
Contributor

vudaltsov commented Jan 4, 2019

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29763
License MIT
Doc PR n/a

Today I started working on #29763.

  • Added a test: /index should not match /index.{!_format?xml}).
  • This test was supposed to fail, but it didn't!
  • The reason is that at
    if ('variable' === $token[0] && $route->hasDefault($token[3])) {
    $token[3] equals !_format, not _format, so $route->hasDefault('!_format') returns false.

I think that current implementation of important variables is not correct. Route parameter name should be _format, not !_format. It is expected to be so inRoute::isDefault(), CompiledRoute::getVariables() and other places across the component. So when creating a token we should keep $token[3] (variable name) clean without !.

I suggest adding a new offset in the token array for (bool) $important. Since [4] is reserved for (bool) $needsUtf8 (see), we can take [5].

  • Refactor RoutingCompiler and its tests accordingly.
  • Prevent short urls from matching.

@nicolas-grekas nicolas-grekas added this to the next milestone Jan 4, 2019

@vudaltsov

This comment has been minimized.

Copy link
Contributor

vudaltsov commented Jan 4, 2019

As discussed with @nicolas-grekas , #29763 will be implemented in this PR as well :)

@vudaltsov vudaltsov changed the title [Routing] Refactor important route parameters [Routing] Make important parameters required when matching Jan 4, 2019

@vudaltsov vudaltsov force-pushed the vudaltsov:routing-important-parameter-refactoring branch from 7ae21c9 to 098ab7b Jan 4, 2019

@@ -199,7 +197,7 @@ private static function compilePattern(Route $route, $pattern, $isHost)
if (!$isHost) {
for ($i = \count($tokens) - 1; $i >= 0; --$i) {
$token = $tokens[$i];
if ('variable' === $token[0] && $route->hasDefault($token[3])) {
if ('variable' === $token[0] && !$token[5] && $route->hasDefault($token[3])) {

This comment has been minimized.

@vudaltsov

vudaltsov Jan 4, 2019

Contributor

Here is the change that prevents matching

@vudaltsov

This comment has been minimized.

Copy link
Contributor

vudaltsov commented Jan 4, 2019

@nicolas-grekas , ready!

@vudaltsov

This comment has been minimized.

Copy link
Contributor

vudaltsov commented Jan 4, 2019

CI fails are not relevant (I see some Intl component errors), Routing is green locally :)

@vudaltsov vudaltsov force-pushed the vudaltsov:routing-important-parameter-refactoring branch 2 times, most recently from 00f6760 to f18a6e1 Jan 9, 2019

@vudaltsov vudaltsov force-pushed the vudaltsov:routing-important-parameter-refactoring branch from f18a6e1 to ca35103 Jan 15, 2019

@vudaltsov

This comment has been minimized.

Copy link
Contributor

vudaltsov commented Jan 15, 2019

Finally green and ready!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment