Skip to content

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

Merged

Conversation

kevinjhappy
Copy link
Contributor

@kevinjhappy kevinjhappy commented Aug 30, 2018

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

@@ -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())){
Copy link
Member

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);
Copy link
Member

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

Copy link
Member

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
Copy link
Member

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)

Copy link
Member

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
Copy link
Member

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
Copy link
Member

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)
Copy link
Member

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)

Copy link
Contributor Author

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

Copy link
Member

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.

@nicolas-grekas nicolas-grekas added this to the next milestone Aug 30, 2018
@@ -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()) {
Copy link
Member

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)

Copy link
Member

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);
Copy link
Member

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

Copy link
Contributor Author

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)
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

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.

@stof
Copy link
Member

stof commented Aug 30, 2018

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)

@nicolas-grekas
Copy link
Member

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. DebugClassLoader or via a static analyzer. Could be a useful improvement (but not for this PR of course.)

@kevinjhappy
Copy link
Contributor Author

Thanks @nicolas-grekas and @stof, I fixed Changelog and functions according to your feedbacks.
About the providerKey in GuardAuthenticatorHandler::authenticateWithToken(), I started reverting my change to finally uncomment the argument according to Nicolas's last message.

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.
Copy link
Member

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)

Copy link
Contributor Author

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

@GuilhemN
Copy link
Contributor

I think we can avoid the limitation @stof described using @param annotations. I opened #28329 to suggest this alternative.

4.2.0
-----

* The method `Client::submit()` will have one `$serverParameters` argument in version 5.0, not definig it
Copy link
Contributor

Choose a reason for hiding this comment

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

defining

Copy link
Member

Choose a reason for hiding this comment

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

(and double space :) )

Copy link
Contributor Author

@kevinjhappy kevinjhappy Aug 31, 2018

Choose a reason for hiding this comment

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

missed it sorry, done :)

@nicolas-grekas nicolas-grekas changed the title [DEPRECATION] add trigger_error in case of inherit class using functions while missing new arguments Trigger deprecation notices when inherited class calls parent method but misses adding new arguments Sep 2, 2018
Copy link
Member

@nicolas-grekas nicolas-grekas left a 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.
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

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

not defining them

Copy link
Member

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.
Copy link
Member

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
Copy link
Member

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';
Copy link
Member

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);
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

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

not defining them

Copy link
Contributor Author

@kevinjhappy kevinjhappy Sep 3, 2018

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 :)

@nicolas-grekas nicolas-grekas force-pushed the feature/add_deprecation_error branch 2 times, most recently from 93c3974 to 601fa91 Compare September 9, 2018 17:29
@nicolas-grekas nicolas-grekas force-pushed the feature/add_deprecation_error branch 2 times, most recently from b19e1be to 883ed5c Compare September 9, 2018 17:55
@nicolas-grekas
Copy link
Member

Thank you @kevinjhappy.

@nicolas-grekas nicolas-grekas merged commit f75fffa into symfony:master Sep 17, 2018
nicolas-grekas added a commit that referenced this pull request Sep 17, 2018
… 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
symfony-splitter pushed a commit to symfony/debug that referenced this pull request Sep 21, 2018
…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
symfony-splitter pushed a commit to symfony/monolog-bridge that referenced this pull request Sep 21, 2018
…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
nicolas-grekas added a commit that referenced this pull request Sep 21, 2018
…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
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
nicolas-grekas added a commit that referenced this pull request Jun 8, 2019
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants