Skip to content

Raise notice when controller is defined with a starting back slash #30045

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Raise notice when controller is defined with a starting back slash #30045

wants to merge 1 commit into from

Conversation

itscaro
Copy link

@itscaro itscaro commented Jan 30, 2019 •
edited
Loading

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

@itscaro itscaro closed this Jan 30, 2019
@itscaro itscaro deleted the quan/issue-29945 branch January 30, 2019 16:31
@itscaro itscaro changed the title Raise deprecation notice when the service id starts with \ wrong one Jan 30, 2019
@itscaro itscaro reopened this Jan 30, 2019
@itscaro itscaro changed the title wrong one Raise notice when controller or service is defined with a starting back slash Jan 30, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone Jan 30, 2019
@nicolas-grekas
Copy link
Member

The PR table looks fine thanks.
But I don't think we should deprecate services starting with a \ - at least that wouldn't help the linked issue at all.
And I also don't think we should hardcode the _controller key in the routing component.
ControllerResolver look like a better place to achieve what is described in #29945

@itscaro
Raise deprecation notice when the controller definition starts with \
2ea10b4 
Signed-off-by: Minh-Quan TRAN <account@itscaro.me>
@itscaro
Copy link
Author

itscaro commented Jan 30, 2019 •
edited
Loading

@nicolas-grekas like this? I put it in catch so that it will not be evaluated each time, but maybe it's preferable to put it at the start of the function?

about the services, if the controller is defined with \, the service must be with \ so that the $this->container->has() can find the service. That why in the linked issue I propose to raise notice when controller or service is define with \, so that the only good naming is without starting \.

Implementing only the notice for controller declaration seem insufficient and might make confusion too.

@itscaro itscaro changed the title Raise notice when controller or service is defined with a starting back slash Raise notice when controller is defined with a starting back slash Jan 30, 2019
OskarStark
OskarStark reviewed Jan 30, 2019
View reviewed changes
src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php
@@ -54,6 +54,9 @@ protected function instantiateController($class)
try {
return parent::instantiateController($class);
} catch (\Error $e) {
if (0 === strpos($class, '\\')) {
@trigger_error(sprintf('The controller definition "%s" must not begin with "\"', $class), E_USER_DEPRECATED);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe: must not start with a slash

Instead of

“\” ?

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH it's backslash actually ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @OskarStark and you. What about

Suggested change
@trigger_error(sprintf('The controller definition "%s" must not begin with "\"', $class), E_USER_DEPRECATED);
@trigger_error(sprintf('The controller definition "%s" must not start with a backslash.', $class), E_USER_DEPRECATED);

It looks like start with smth is the correct wording: https://www.thefreedictionary.com/words-that-start-with-x .
We should also add a dot, because it's a sentence.

ismail1432
ismail1432 reviewed Feb 1, 2019
View reviewed changes
src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php
@@ -54,6 +54,9 @@ protected function instantiateController($class)
try {
return parent::instantiateController($class);
} catch (\Error $e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's a notice IMO we should not trigger only on Excpetion

vudaltsov
vudaltsov suggested changes Feb 12, 2019
View reviewed changes
src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php
@@ -54,6 +54,9 @@ protected function instantiateController($class)
try {
return parent::instantiateController($class);
} catch (\Error $e) {
if (0 === strpos($class, '\\')) {
@trigger_error(sprintf('The controller definition "%s" must not begin with "\"', $class), E_USER_DEPRECATED);
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @OskarStark and you. What about

Suggested change
@trigger_error(sprintf('The controller definition "%s" must not begin with "\"', $class), E_USER_DEPRECATED);
@trigger_error(sprintf('The controller definition "%s" must not start with a backslash.', $class), E_USER_DEPRECATED);

It looks like start with smth is the correct wording: https://www.thefreedictionary.com/words-that-start-with-x .
We should also add a dot, because it's a sentence.

fabpot
fabpot requested changes Apr 3, 2019
View reviewed changes
src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php
@@ -54,6 +54,9 @@ protected function instantiateController($class)
try {
return parent::instantiateController($class);
} catch (\Error $e) {
if (0 === strpos($class, '\\')) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of deprecating this possibility, what about trimming the starting backslash in the instantiateController method?

@Simperfit
Copy link
Contributor

@itscaro Doyou have time to finish this one with what @fabpot suggested ?

@fabpot
Copy link
Member

fabpot commented Jul 12, 2019

Closing as there is no more feedback and I think the approach is not the right one.

@fabpot fabpot closed this Jul 12, 2019
@Simperfit Simperfit mentioned this pull request Jul 14, 2019
Merged
fabpot added a commit that referenced this pull request Aug 9, 2019
@fabpot
bug #32541 [HttpKernel] trim the leading backslash in the controller …
1ff2ab3 
…init (Simperfit, fabpot)

This PR was submitted for the 4.4 branch but it was merged into the 4.3 branch instead (closes #32541).

Discussion
----------

[HttpKernel] trim the leading backslash in the controller init

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #29945
| License       | MIT
| Doc PR        | none <!-- required for new features -->

<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against branch 4.4.
 - Legacy code removals go to the master branch.
-->

This fixes an error where the classes exists when using a leading backslash in the controller, it's not invalid to do so.

see #30045 (comment)

Commits
-------

3c8d395 [HttpKernel] fixed class having a leading \ in a route controller
6fdf252 [HttpKernel] trim the leading backslash in the controller init
@nicolas-grekas nicolas-grekas removed this from the next milestone Oct 27, 2019
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eXtreme eXtreme eXtreme left review comments

@OskarStark OskarStark OskarStark left review comments

@ismail1432 ismail1432 ismail1432 left review comments

@fabpot fabpot fabpot requested changes

@vudaltsov vudaltsov vudaltsov requested changes

Assignees
No one assigned
Labels
Deprecation Status: Needs Work
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
9 participants
@itscaro @nicolas-grekas @Simperfit @fabpot @eXtreme @OskarStark @vudaltsov @ismail1432 @carsonbot