-
Notifications
You must be signed in to change notification settings - Fork 450
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
removal of unused function in AuthorizeController #470
base: master
Are you sure you want to change the base?
Conversation
Since the controller is public, it should rather be deprecated first. |
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.
Except my last comments 👍
Controller/AuthorizeController.php
Outdated
*/ | ||
protected function getRedirectionUrl(UserInterface $user) | ||
{ | ||
@trigger_error('deprecated', E_USER_DEPRECATED); |
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.
@trigger_error(sprintf('%s() is deprecated since version 1.6 and will be removed in 2.0', __METHOD__))
Controller/AuthorizeController.php
Outdated
@@ -242,9 +242,13 @@ protected function processSuccess(UserInterface $user, AuthorizeFormHandler $for | |||
* @param UserInterface $user | |||
* | |||
* @return string | |||
* | |||
* @deprecated since 1.5.2, to be removed in 2.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.
Since 1.6
…ndle into unused-code # Conflicts: # Controller/AuthorizeController.php
Controller/AuthorizeController.php
Outdated
*/ | ||
protected function getRedirectionUrl(UserInterface $user) | ||
{ | ||
@trigger_error(sprintf('%s() is deprecated since version 1.6 and will be removed in 2.0', __METHOD__)); | ||
|
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.
Please remove spaces, it makes the tests fail
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.
LGTM, wdyt @dkarlovi?
It should target 1.6, no? |
No description provided.