Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@kevinjhappy
Copy link
Contributor

@kevinjhappykevinjhappy commentedAug 30, 2018
edited
Loading

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets
LicenseMIT
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 functionreturn parent::function() and by calling this child class and catching the expected depreciation message

*/
publicfunctiongetLogs(/* Request $request = null */)
{
if ((func_num_args() <1) && (__CLASS__ !==\get_class($this)) && (__CLASS__ !== (new \ReflectionMethod($this,__FUNCTION__))->getDeclaringClass()->getName())){

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)

{
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);

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

Choose a reason for hiding this comment

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

since Symfony 4.2.

// 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

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

-----

* added`ProcessorInterface`: an optional interface to allow autoconfiguration of Monolog processors
*[DEPRECATION] add trigger_error in case of inherit class using getLogs and countErrors

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

Choose a reason for hiding this comment

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

(same as above)

* @param string $providerKey The name of the provider/firewall being used for authentication
*/
publicfunctionauthenticateWithToken(TokenInterface$token,Request$request/*, string $providerKey */)
publicfunctionauthenticateWithToken(TokenInterface$token,Request$request,string$providerKey)

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
ContributorAuthor

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

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-grekasnicolas-grekas added this to thenext milestoneAug 30, 2018
*/
publicfunctiongetLogs(/* 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)

kevinjhappy reacted with thumbs up emoji
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

* @throws LockAcquiringException If the lock can not be refreshed
*/
publicfunctionrefresh(/*$ttl = null */);
publicfunctionrefresh($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
ContributorAuthor

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

* @param string $providerKey The name of the provider/firewall being used for authentication
*/
publicfunctionauthenticateWithToken(TokenInterface$token,Request$request/*, string $providerKey*/)
publicfunctionauthenticateWithToken(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
ContributorAuthor

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

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

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
ContributorAuthor

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

* 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.

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)

kevinjhappy reacted with thumbs up emoji
Copy link
ContributorAuthor

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

Choose a reason for hiding this comment

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

(and double space :) )

GuilhemN reacted with laugh emoji
Copy link
ContributorAuthor

@kevinjhappykevinjhappyAug 31, 2018
edited
Loading

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-grekasnicolas-grekas changed the title[DEPRECATION] add trigger_error in case of inherit class using functions while missing new argumentsTrigger deprecation notices when inherited class calls parent method but misses adding new argumentsSep 2, 2018
Copy link
Member

@nicolas-grekasnicolas-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.


* 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.

Choose a reason for hiding this comment

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

not definingthem

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.

Choose a reason for hiding this comment

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

not defining them

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)


* 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.

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

Choose a reason for hiding this comment

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

bad indentation


publicfunctiontestSessionStrategyIsNotCalledWhenStateless()
{
$providerKey ='some_provider_key';

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)

->expects($this->once())
->method('authenticateWithToken')
->with($authenticateToken,$this->request);
->with($authenticateToken,$this->request,$providerKey);

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?

* 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.

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
ContributorAuthor

@kevinjhappykevinjhappySep 3, 2018
edited
Loading

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-grekasnicolas-grekasforce-pushed thefeature/add_deprecation_error branch 2 times, most recently fromb19e1be to883ed5cCompareSeptember 9, 2018 17:55
@nicolas-grekas
Copy link
Member

Thank you@kevinjhappy.

@nicolas-grekasnicolas-grekas merged commitf75fffa intosymfony:masterSep 17, 2018
nicolas-grekas added a commit that referenced this pull requestSep 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.2The 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 missing2- ```(__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 class3- ```(__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 classCode 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 messageCommits-------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 requestSep 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 waysymfony/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:```phpclass 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 */)    {    }}``````phpclass 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 requestSep 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 waysymfony/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:```phpclass 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 */)    {    }}``````phpclass 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 requestSep 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:```phpclass 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 */)    {    }}``````phpclass 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-grekasnicolas-grekas modified the milestones:next,4.2Nov 1, 2018
This was referencedNov 3, 2018
nicolas-grekas added a commit that referenced this pull requestJun 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     <!-- seehttps://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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof requested changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhxabbuh approved these changes

@dunglasdunglasAwaiting requested review from dunglas

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

+1 more reviewer

@GuilhemNGuilhemNGuilhemN left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

6 participants

@kevinjhappy@stof@nicolas-grekas@GuilhemN@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp