[HttpKernel] Make ServiceValueResolver work if controller namespace starts with a backslash in routing #26773

mathieutu · 2018-04-03T13:49:43Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#26772
License	MIT

Hi folks,

This PR fixes #26772:

Controller "App\Controllers\Foo" requires that you provide a value for the "$foo" argument. Either the argument is nullable and no null value has been provided, no default value has been provided or because there is a non optional argument after this one.

Thank you for your work!

PS: This is my first (of many planned!) PR to Symfony, so please let me know if I did something wrong.

Meroje · 2018-04-03T14:02:38Z

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/ServiceValueResolver.php

@@ -41,6 +41,8 @@ public function supports(Request $request, ArgumentMetadata $argument)
            $controller = $controller[0].'::'.$controller[1];
        }

+        $controller = ltrim($controller, '\\');


trim takes a list of characters, thus having two backslashes is redundant. You may want to use \ltrim too but not sure about the convention here

Hi @Meroje if I don't use the two backslashes, it will consider that I'm escaping the last ' and never close the string:

ltrim($controller, '\')

And for the \ltrim, I've just done the same that in the rest of the code.

Ha nvm then, thought for a second it wouldn't work at the end of a string

I'm not sure it's useful for ltrim: PHP-CS-Fixer/PHP-CS-Fixer#3048

It would be great to put the ltrim in a condition.
Basically, I'd suggest doing this:

if (\is_array($controller) && \is_callable($controller, true) && \is_string($controller[0])) { $controller = $controller[0].'::'.$controller[1]; } elseif (!\is_string($controller) || '' === $controller) { return false; } if ('\\' === $controller[0]) { $controller = ltrim($controller, '\\'); } return \$this->container->has($controller) && $this->container->get($controller)->has($argument->getName());

nicolas-grekas

Congrats for your first PR!
Here are some nitpicking :P

nicolas-grekas · 2018-04-03T20:37:23Z

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/ServiceValueResolver.php

@@ -41,6 +41,8 @@ public function supports(Request $request, ArgumentMetadata $argument)
            $controller = $controller[0].'::'.$controller[1];
        }

+        $controller = ltrim($controller, '\\');


It would be great to put the ltrim in a condition.
Basically, I'd suggest doing this:

if (\is_array($controller) && \is_callable($controller, true) && \is_string($controller[0])) { $controller = $controller[0].'::'.$controller[1]; } elseif (!\is_string($controller) || '' === $controller) { return false; } if ('\\' === $controller[0]) { $controller = ltrim($controller, '\\'); } return \$this->container->has($controller) && $this->container->get($controller)->has($argument->getName());

nicolas-grekas · 2018-04-03T20:38:49Z

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/ServiceValueResolver.php

@@ -53,6 +55,8 @@ public function resolve(Request $request, ArgumentMetadata $argument)
            $controller = $controller[0].'::'.$controller[1];
        }

+        $controller = ltrim($controller, '\\');


consistently with the previous comment, wrap this call in if ('\\' === $controller[0]) {...}

Ok, thanks Nicolas.
One question, why the condition to format the controller if it's not an array is not the same in both supports and resolve methods?

Couldn't we extract this in its own method to make it consistent?

stof · 2018-04-04T08:33:55Z

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/ServiceValueResolver.php

+        }
+
+        if ('\\' === $controller[0]) {
+            $controller = ltrim($controller, '\\');
        }

        return \is_string($controller) && $this->container->has($controller) && $this->container->get($controller)->has($argument->getName());


is_string($controller) is useless here now, as it will always be true (due to the new check added above)

right, thx!

nicolas-grekas

(for 3.4)

fabpot · 2018-04-04T11:53:56Z

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/ServiceValueResolver.php

        }

-        return \is_string($controller) && $this->container->has($controller) && $this->container->get($controller)->has($argument->getName());
+        if ('\\' === $controller[0]) {


Do we really need to test first and then trim? Why not always trim?

Trim means triggering copy-on-write when the controller string usually comes from shared memory, which means also recomputing the hash of the string, even if it doesn't change.

stof · 2018-04-04T18:13:49Z

another solution could be to perform this trimming during the cache warmup of the router (we already have some logic there to resolve the bundle notation to class names there to optimize things). This way, we would have the controller class without leading backslash in the _controller key

mathieutu · 2018-04-04T20:03:11Z

Yeah could be a good idea, maybe a bit more consistent, and need less operations.
As you want guys, let me know and I'll try to do it on router!

nicolas-grekas · 2018-04-04T20:57:15Z

Worth a try for sure!

Tobion · 2018-04-07T20:09:59Z

another solution could be to perform this trimming during the cache warmup of the router (we already have some logic there to resolve the bundle notation to class names there to optimize things).

Since the bundle notation is deprecated, this part might not survive long. So I would either do it in the Route class itself, so it it always set in a normalized way for all consumers. Or do it like proposed here.

nicolas-grekas

let's do it this way: this responsibility on ServiceValueResolver looks legit to me

…tarts with a backslash in routing

nicolas-grekas · 2018-04-14T15:07:02Z

Thank you @mathieutu.

…namespace starts with a backslash in routing (mathieutu) This PR was submitted for the 4.0 branch but it was squashed and merged into the 3.4 branch instead (closes #26773). Discussion ---------- [HttpKernel] Make ServiceValueResolver work if controller namespace starts with a backslash in routing | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26772 | License | MIT Hi folks, This PR fixes #26772: >Controller "App\Controllers\Foo" requires that you provide a value for the "$foo" argument. Either the argument is nullable and no null value has been provided, no default value has been provided or because there is a non optional argument after this one. Thank you for your work! PS: This is my first (of many planned!) PR to Symfony, so please let me know if I did something wrong. Commits ------- 3b47441 [HttpKernel] Make ServiceValueResolver work if controller namespace starts with a backslash in routing

carsonbot added Status: Needs Review Bug labels Apr 3, 2018

mathieutu mentioned this pull request Apr 3, 2018

[HttpKernel][Bug] ServiceValueResolver fails if controller namespace starts with a backslash in routing. #26772

Closed

mathieutu changed the title ~~Make ServiceValueResolver working if controller has a backslash at first character in routing.~~ [HttpKernel][Fix] Make ServiceValueResolver working if controller namespace start with a backslash in routing. Apr 3, 2018

mathieutu changed the title ~~[HttpKernel][Fix] Make ServiceValueResolver working if controller namespace start with a backslash in routing.~~ [HttpKernel][Fix] Make ServiceValueResolver working if controller namespace starts with a backslash in routing. Apr 3, 2018

Meroje reviewed Apr 3, 2018

View reviewed changes

nicolas-grekas requested changes Apr 3, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 3, 2018

nicolas-grekas added this to the 3.4 milestone Apr 3, 2018

stof reviewed Apr 4, 2018

View reviewed changes

carsonbot removed the Status: Needs Work label Apr 4, 2018

nicolas-grekas approved these changes Apr 4, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Apr 4, 2018

fabpot reviewed Apr 4, 2018

View reviewed changes

nicolas-grekas changed the title ~~[HttpKernel][Fix] Make ServiceValueResolver working if controller namespace starts with a backslash in routing.~~ [HttpKernel][Fix] Make ServiceValueResolver work if controller namespace starts with a backslash in routing. Apr 5, 2018

nicolas-grekas changed the title ~~[HttpKernel][Fix] Make ServiceValueResolver work if controller namespace starts with a backslash in routing.~~ [HttpKernel][Fix] Make ServiceValueResolver work if controller namespace starts with a backslash in routing Apr 5, 2018

nicolas-grekas changed the title ~~[HttpKernel][Fix] Make ServiceValueResolver work if controller namespace starts with a backslash in routing~~ [HttpKernel] Make ServiceValueResolver work if controller namespace starts with a backslash in routing Apr 5, 2018

nicolas-grekas approved these changes Apr 9, 2018

View reviewed changes

chalasr added the HttpKernel label Apr 11, 2018

[HttpKernel] Make ServiceValueResolver work if controller namespace s…

3b47441

…tarts with a backslash in routing

nicolas-grekas changed the base branch from 4.0 to 3.4 April 14, 2018 15:06

nicolas-grekas force-pushed the fixes/route-trailing-slash branch from 52ef950 to 3b47441 Compare April 14, 2018 15:06

nicolas-grekas merged commit 3b47441 into symfony:3.4 Apr 14, 2018

This was referenced Apr 30, 2018

Release v3.4.9 #27102

Merged

Release v4.0.9 #27103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpKernel] Make ServiceValueResolver work if controller namespace starts with a backslash in routing #26773

[HttpKernel] Make ServiceValueResolver work if controller namespace starts with a backslash in routing #26773

mathieutu commented Apr 3, 2018 •

edited by chalasr

Loading

Meroje Apr 3, 2018

mathieutu Apr 3, 2018 •

edited

Loading

Meroje Apr 3, 2018

linaori Apr 3, 2018

nicolas-grekas Apr 3, 2018

nicolas-grekas left a comment

nicolas-grekas Apr 3, 2018

nicolas-grekas Apr 3, 2018

mathieutu Apr 4, 2018 •

edited

Loading

stof Apr 4, 2018

mathieutu Apr 4, 2018

nicolas-grekas left a comment

fabpot Apr 4, 2018

nicolas-grekas Apr 4, 2018

stof commented Apr 4, 2018

mathieutu commented Apr 4, 2018

nicolas-grekas commented Apr 4, 2018

Tobion commented Apr 7, 2018

nicolas-grekas left a comment

nicolas-grekas commented Apr 14, 2018

[HttpKernel] Make ServiceValueResolver work if controller namespace starts with a backslash in routing #26773

[HttpKernel] Make ServiceValueResolver work if controller namespace starts with a backslash in routing #26773

Conversation

mathieutu commented Apr 3, 2018 • edited by chalasr Loading

Choose a reason for hiding this comment

mathieutu Apr 3, 2018 • 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

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathieutu Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Apr 4, 2018

mathieutu commented Apr 4, 2018

nicolas-grekas commented Apr 4, 2018

Tobion commented Apr 7, 2018

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 14, 2018

mathieutu commented Apr 3, 2018 •

edited by chalasr

Loading

mathieutu Apr 3, 2018 •

edited

Loading

mathieutu Apr 4, 2018 •

edited

Loading