-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Trigger deprecation notices when inherited class calls parent method but misses adding new arguments #28316
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
Trigger deprecation notices when inherited class calls parent method but misses adding new arguments #28316
Conversation
@@ -28,6 +28,10 @@ class Logger extends BaseLogger implements DebugLoggerInterface, ResetInterface | |||
*/ | |||
public function getLogs(/* Request $request = null */) | |||
{ | |||
if ((func_num_args() < 1) && (__CLASS__ !== \get_class($this)) && (__CLASS__ !== (new \ReflectionMethod($this, __FUNCTION__))->getDeclaringClass()->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.
please remove all extra brackets (same below)
@@ -28,7 +28,7 @@ class Logger extends BaseLogger implements DebugLoggerInterface, ResetInterface | |||
*/ | |||
public function getLogs(/* Request $request = null */) | |||
{ | |||
if ((func_num_args() < 1) && (__CLASS__ !== \get_class($this)) && (__CLASS__ !== (new \ReflectionMethod($this, __FUNCTION__))->getDeclaringClass()->getName())) { | |||
if (func_num_args() < 1 && __CLASS__ !== \get_class($this) && __CLASS__ !== (new \ReflectionMethod($this, __FUNCTION__))->getDeclaringClass()->getName()) { | |||
@trigger_error(sprintf('The "%s()" method will have one `Request $request = null` argument in version 5.0 and higher.Not defining it is deprecated since Symfony 4.1.', __METHOD__), 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.
..., not defining it
... (same below
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 Symfony 4.2.
UPGRADE-4.2.md
Outdated
@@ -75,6 +75,8 @@ Process | |||
// alternatively, when a shell wrapper is required | |||
$process = Process::fromShellCommandline('ls -l'); | |||
``` | |||
* Add trigger_error messages with E_USER_DEPRECATED to functions in components Bridge/Monolog, BrowserKit, DomCrawler, Finder and Serializer when class is inherited and |
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.
there should be one line per affected component, telling roughly the same as what deprecation messages tell (same in UPGRADE-5.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.
the changelog should tell that we are adding a new argument, not that we are adding a deprecation warning
@@ -5,6 +5,8 @@ CHANGELOG | |||
----- | |||
|
|||
* added `ProcessorInterface`: an optional interface to allow autoconfiguration of Monolog processors | |||
* [DEPRECATION] add trigger_error in case of inherit class using getLogs and countErrors |
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.
no need for the [...]
label
also the message should be less technical and more useful to the reader (stating roughly the same as the deprecation message)
4.2.0 | ||
----- | ||
|
||
* [DEPRECATION] add trigger_error in case of inherit class using submit |
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.
(same as above)
@@ -54,10 +54,8 @@ public function __construct(TokenStorageInterface $tokenStorage, EventDispatcher | |||
* | |||
* @param string $providerKey The name of the provider/firewall being used for authentication | |||
*/ | |||
public function authenticateWithToken(TokenInterface $token, Request $request/*, string $providerKey */) | |||
public function authenticateWithToken(TokenInterface $token, Request $request, string $providerKey) |
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.
hum actually we should add null as a default value here, so not break BC at least (and thus revert the changes on the test cases)
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.
No problem to make this change, I just observed in the project Symfony that all the features using this function implement the providerKey argument and as the class is annotate final I believed it will be OK, only the unit tests was needed to be changed
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.
if anyone else calls the function, their code will break. since the method is not internal, that's should remain supported.
@@ -28,6 +28,10 @@ class Logger extends BaseLogger implements DebugLoggerInterface, ResetInterface | |||
*/ | |||
public function getLogs(/* Request $request = null */) | |||
{ | |||
if (\func_num_args() < 1 && __CLASS__ !== \get_class($this) && __CLASS__ !== (new \ReflectionMethod($this, __FUNCTION__))->getDeclaringClass()->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.
I would add && !$this instanceof \PHPUnit\Framework\MockObject\MockObject && !$this instanceof \Prophecy\Prophecy\ProphecySubjectInterface
to avoid triggering this when mocking the class with PHPUnit or Prophecy (the mock class will get its signature from the actual signature, so the argument won't be added until we add it for real, but that's fine as it will then be added automatically)
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.
that applies to all places of course
@@ -43,7 +43,7 @@ public function acquire($blocking = false); | |||
* @throws LockConflictedException If the lock is acquired by someone else | |||
* @throws LockAcquiringException If the lock can not be refreshed | |||
*/ | |||
public function refresh(/* $ttl = null */); | |||
public function refresh($ttl = null); |
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.
that change is a BC break. We cannot uncomment it until 5.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.
Argh I didn't see that thanks
@@ -54,10 +54,8 @@ public function __construct(TokenStorageInterface $tokenStorage, EventDispatcher | |||
* | |||
* @param string $providerKey The name of the provider/firewall being used for authentication | |||
*/ | |||
public function authenticateWithToken(TokenInterface $token, Request $request/*, string $providerKey */) | |||
public function authenticateWithToken(TokenInterface $token, Request $request, string $providerKey = null) |
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.
this is also a BC break
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.
This arguments is commented since Symfony 3.4, and the class passed final since 4.0, this particular evolution was skipped in the newest version, you are right it's need to be made in 5.0, not now
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.
The class has been made final in 3.4, which means we can do this change now. We should have done it earlier in fact.
Most of these deprecations won't be triggered if the child method does not call the parent method, so they might not be the best place to trigger them (sometimes, we might not have a better way though) |
True. We can't do better I think, unless we create a way to declare such added arguments and trigger a deprecation in e.g. |
Thanks @nicolas-grekas and @stof, I fixed Changelog and functions according to your feedbacks. |
UPGRADE-4.2.md
Outdated
* In `Component/BrowserKit`, the `Client::submit()` method will have a new `$serverParameters` argument in version 5.0, not defining it is deprecated since version 4.2. | ||
* In `Component/DomCrawler`, the `Crawler::children()` method will have one `$selector` argument in version 5.0, not defining it is deprecated since version 4.2. | ||
* In `Component/Finder`, the `Finder::sortByName()` method will have one `$useNaturalSort` argument in version 5.0, not defining it is deprecated since version 4.2. | ||
* In `Component/Serializer`, the `AbstractNormalizer::handleCircularReference()` method will have two more arguments : `$format` and `$context` in version 5.0, not defining it is deprecated since version 4.2. |
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 now, all these lines are in the "Process" component section. They should be split each in their component's section instead. (same in UPGRADE-5.0.md)
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, didn't understand the chapters, make more sense now :)
It's fixed
4.2.0 | ||
----- | ||
|
||
* The method `Client::submit()` will have one `$serverParameters` argument in version 5.0, not definig it |
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.
defining
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.
(and double space :) )
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.
missed it sorry, done :)
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, here are some remaining comments.
UPGRADE-4.2.md
Outdated
@@ -149,3 +169,4 @@ Serializer | |||
|
|||
* Relying on the default value (false) of the "as_collection" option is deprecated since 4.2. | |||
You should set it to false explicitly instead as true will be the default value in 5.0. | |||
* The `AbstractNormalizer::handleCircularReference()` method will have two more arguments : `$format` and `$context` in version 5.0, not defining it is deprecated since version 4.2. |
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.
not defining them
UPGRADE-5.0.md
Outdated
Serializer | ||
---------- | ||
|
||
* The `AbstractNormalizer::handleCircularReference()` method will have two more arguments : `$format` and `$context` in version 5.0, not defining it is deprecated since version 4.2. |
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.
not defining them
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.
ping ;) (rebase needed also + squash if you want)
@@ -5,6 +5,8 @@ CHANGELOG | |||
----- | |||
|
|||
* added `ProcessorInterface`: an optional interface to allow autoconfiguration of Monolog processors | |||
* The methods `DebugProcessor::getLogs()`, `DebugProcessor::countErrors()`, `Logger::getLogs()` and `Logger::countErrors()` | |||
will have one `$request` argument in version 5.0, not defining it is deprecated since Symfony 4.2. |
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.
missing indentation
----- | ||
|
||
* The method `Client::submit()` will have one `$serverParameters` argument | ||
in version 5.0, not defining it is deprecated since version 4.2 |
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.
bad indentation
@@ -146,14 +146,15 @@ public function testSessionStrategyIsCalled() | |||
|
|||
public function testSessionStrategyIsNotCalledWhenStateless() | |||
{ | |||
$providerKey = 'some_provider_key'; |
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.
can be reverted, isn't it? (same below in the same file)
@@ -64,7 +64,7 @@ public function testHandleSuccess() | |||
$this->guardAuthenticatorHandler | |||
->expects($this->once()) | |||
->method('authenticateWithToken') | |||
->with($authenticateToken, $this->request); | |||
->with($authenticateToken, $this->request, $providerKey); |
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.
can be reverted, isn't it?
@@ -8,6 +8,8 @@ CHANGELOG | |||
* added support for XML comment encoding (encoding `['#comment' => ' foo ']` results `<!-- foo -->`) | |||
* added optional `int[] $encoderIgnoredNodeTypes` argument to `XmlEncoder::__construct` to configure node types to be | |||
ignored during encoding. | |||
* the `AbstractNormalizer::handleCircularReference()` method will have two more arguments : `$format` and | |||
`$context` in version 5.0, not defining it is deprecated since version 4.2. |
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.
not defining them
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.
thanks @nicolas-grekas, I fixed the typo and revert unit tests :)
93c3974
to
601fa91
Compare
b19e1be
to
883ed5c
Compare
…but misses adding new arguments
883ed5c
to
f75fffa
Compare
Thank you @kevinjhappy. |
… parent method but misses adding new arguments (kevinjhappy) This PR was merged into the 4.2-dev branch. Discussion ---------- Trigger deprecation notices when inherited class calls parent method but misses adding new arguments | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | This Pull Request concern severals components, the purpose here is to notify in dev mode that in a case of inherit class, a function will have a new or severals arguments in Symfony 5.0, therefore not implement it is deprecated since Symfony 4.2 The function is made by these conditions : 1- ```(func_num_args() < $x)``` where [x] is the number of arguments we will have in Symfony 5.0 this check allow to verify that the arguments are missing 2- ```(__CLASS__ !== \get_class($this))``` this check allow to verify that the name of the class is different than the base class, therefore that we are in the child class 3- ```(__CLASS__ !== (new \ReflectionMethod($this, __FUNCTION__))->getDeclaringClass()->getName())``` this check allow to verify that the class of the current function is different than the base class, therefore the function has been rewrote into the child class Code exemple : ``` public function method(/* void $myNewArgument = null */) { if ((func_num_args() < 1) && (__CLASS__ !== \get_class($this)) && (__CLASS__ !== (new \ReflectionMethod($this, __FUNCTION__))->getDeclaringClass()->getName())){ @trigger_error(sprintf('The "%s()" method will have one `void $myNewArgument = null` argument in version 5.0 and higher.Not defining it is deprecated since Symfony 4.2.', __METHOD__ ), E_USER_DEPRECATED); } // do something } ``` The unit test are made by creating a child Class using for the tested function ```return parent::function()``` and by calling this child class and catching the expected depreciation message Commits ------- f75fffa Trigger deprecation notices when inherited class calls parent method but misses adding new arguments
…efined in sub classes (GuilhemN) This PR was merged into the 4.2-dev branch. Discussion ---------- [Debug] Trigger a deprecation for new parameters not defined in sub classes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#28316 | License | MIT | Doc PR | - I'm not sure the way symfony/symfony#28316 is implemented is the best so here is an alternative. Instead of counting on a call from the child method, it uses the `DebugClassLoader` and `@param` annotations. If a `@param` annotation is used on a parent but is then not actually implemented in the child class, a deprecation will be thrown. Example: ```php class ClassWithAnnotatedParameters { /** * @param string $foo This is a foo parameter. */ public function fooMethod(string $foo) { } /** * @param string $bar parameter not implemented yet */ public function barMethod(/** string $bar = null */) { } /** * @param Quz $quz parameter not implemented yet */ public function quzMethod(/** Quz $quz = null */) { } } ``` ```php class SubClassWithAnnotatedParameters extends ClassWithAnnotatedParameters { public function fooMethod(string $foo) { } public function barMethod($bar = null) { } public function quzMethod() { } } ``` A deprecation will be triggered because ``SubClassWithAnnotatedParameters::quzMethod()`` which doesn't definee `$quz`. Commits ------- 1f5d8b62f7 [Debug] Trigger a deprecation for new parameters not defined in sub classes
…efined in sub classes (GuilhemN) This PR was merged into the 4.2-dev branch. Discussion ---------- [Debug] Trigger a deprecation for new parameters not defined in sub classes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#28316 | License | MIT | Doc PR | - I'm not sure the way symfony/symfony#28316 is implemented is the best so here is an alternative. Instead of counting on a call from the child method, it uses the `DebugClassLoader` and `@param` annotations. If a `@param` annotation is used on a parent but is then not actually implemented in the child class, a deprecation will be thrown. Example: ```php class ClassWithAnnotatedParameters { /** * @param string $foo This is a foo parameter. */ public function fooMethod(string $foo) { } /** * @param string $bar parameter not implemented yet */ public function barMethod(/** string $bar = null */) { } /** * @param Quz $quz parameter not implemented yet */ public function quzMethod(/** Quz $quz = null */) { } } ``` ```php class SubClassWithAnnotatedParameters extends ClassWithAnnotatedParameters { public function fooMethod(string $foo) { } public function barMethod($bar = null) { } public function quzMethod() { } } ``` A deprecation will be triggered because ``SubClassWithAnnotatedParameters::quzMethod()`` which doesn't definee `$quz`. Commits ------- 1f5d8b62f7 [Debug] Trigger a deprecation for new parameters not defined in sub classes
…efined in sub classes (GuilhemN) This PR was merged into the 4.2-dev branch. Discussion ---------- [Debug] Trigger a deprecation for new parameters not defined in sub classes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28316 | License | MIT | Doc PR | - I'm not sure the way #28316 is implemented is the best so here is an alternative. Instead of counting on a call from the child method, it uses the `DebugClassLoader` and `@param` annotations. If a `@param` annotation is used on a parent but is then not actually implemented in the child class, a deprecation will be thrown. Example: ```php class ClassWithAnnotatedParameters { /** * @param string $foo This is a foo parameter. */ public function fooMethod(string $foo) { } /** * @param string $bar parameter not implemented yet */ public function barMethod(/** string $bar = null */) { } /** * @param Quz $quz parameter not implemented yet */ public function quzMethod(/** Quz $quz = null */) { } } ``` ```php class SubClassWithAnnotatedParameters extends ClassWithAnnotatedParameters { public function fooMethod(string $foo) { } public function barMethod($bar = null) { } public function quzMethod() { } } ``` A deprecation will be triggered because ``SubClassWithAnnotatedParameters::quzMethod()`` which doesn't definee `$quz`. Commits ------- 1f5d8b6 [Debug] Trigger a deprecation for new parameters not defined in sub classes
…anagi) This PR was merged into the 5.0-dev branch. Discussion ---------- [Serializer] Remove last deprecated/obsolete paths | Q | A | ------------- | --- | Branch? | master <!-- see below --> | 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 | #28316, #28709, #31030, #27020, #29896, 16f8a13#r201060750 <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | N/A <!-- required for new features --> This should fix the last deprecations & obsolete code paths for the Serializer component. Commits ------- c703b35 [Serializer] Remove last deprecated/obsolete paths
This Pull Request concern severals components, the purpose here is to notify in dev mode that in a case of inherit class, a function will have a new or severals arguments in Symfony 5.0, therefore not implement it is deprecated since Symfony 4.2
The function is made by these conditions :
1-
(func_num_args() < $x)
where [x] is the number of arguments we will have in Symfony 5.0this check allow to verify that the arguments are missing
2-
(__CLASS__ !== \get_class($this))
this check allow to verify that the name of the class is different than the base class, therefore that we are in the child class
3-
(__CLASS__ !== (new \ReflectionMethod($this, __FUNCTION__))->getDeclaringClass()->getName())
this check allow to verify that the class of the current function is different than the base class, therefore the function has been rewrote into the child class
Code exemple :
The unit test are made by creating a child Class using for the tested function
return parent::function()
and by calling this child class and catching the expected depreciation message