Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Make ServiceValueResolver work if controller namespace starts with a backslash in routing#26773
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $controller =$controller[0].'::'.$controller[1]; | ||
| } | ||
| $controller =ltrim($controller,'\\'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ha nvm then, thought for a second it wouldn't work at the end of a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure it's useful for ltrim:PHP-CS-Fixer/PHP-CS-Fixer#3048
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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) {returnfalse;}if ('\\' ===$controller[0]) {$controller =ltrim($controller,'\\');}return \$this->container->has($controller) &&$this->container->get($controller)->has($argument->getName());
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Congrats for your first PR!
Here are some nitpicking :P
| $controller =$controller[0].'::'.$controller[1]; | ||
| } | ||
| $controller =ltrim($controller,'\\'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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) {returnfalse;}if ('\\' ===$controller[0]) {$controller =ltrim($controller,'\\');}return \$this->container->has($controller) &&$this->container->get($controller)->has($argument->getName());
| $controller =$controller[0].'::'.$controller[1]; | ||
| } | ||
| $controller =ltrim($controller,'\\'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
consistently with the previous comment, wrap this call inif ('\\' === $controller[0]) {...}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, thanks Nicolas.
One question, why the condition to format the controller if it's not an array is not the same in bothsupports andresolve methods?
Couldn't we extract this in its own method to make it consistent?
| $controller =ltrim($controller,'\\'); | ||
| } | ||
| return\is_string($controller) &&$this->container->has($controller) &&$this->container->get($controller)->has($argument->getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is_string($controller) is useless here now, as it will always betrue (due to the new check added above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
right, thx!
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(for 3.4)
| } | ||
| return\is_string($controller) &&$this->container->has($controller) &&$this->container->get($controller)->has($argument->getName()); | ||
| if ('\\' ===$controller[0]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we really need to test first and then trim? Why not always trim?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedApr 4, 2018
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 |
mathieutu commentedApr 4, 2018
Yeah could be a good idea, maybe a bit more consistent, and need less operations. |
nicolas-grekas commentedApr 4, 2018
Worth a try for sure! |
Tobion commentedApr 7, 2018
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 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
let's do it this way: this responsibility on ServiceValueResolver looks legit to me
…tarts with a backslash in routing
52ef950 to3b47441Comparenicolas-grekas commentedApr 14, 2018
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 | MITHi folks,This PRfixes#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
Uh oh!
There was an error while loading.Please reload this page.
Hi folks,
This PRfixes#26772:
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.