Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Debug] Trigger a deprecation for new parameters not defined in sub classes#28329
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| if (\strncmp($ns,$declaringClass,$len)) { | ||
| @trigger_error(sprintf('The "%s::%s()" method is considered internal%s. It may change without further notice. You should not extend it from "%s".',$declaringClass,$method->name,$message,$name),E_USER_DEPRECATED); | ||
| } | ||
| if (isset(self::$internalMethods[$name][$method->name])) { |
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.
Unrelated change but theforeach was useless since we merge parent + traits infos a few lines above.
nicolas-grekas commentedAug 31, 2018
We should try to enable DebugClassLoader using the phpunit bridge and see how it goes! |
GuilhemN commentedAug 31, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nicolas-grekas imo we can enable it either by updating the bridge and adding an In both cases, it will require a consequent work to fix all deprecations (using php 7.1, I already have |
7fd5331 to57d935aComparenicolas-grekas commentedAug 31, 2018
Ok works for me :) |
623ebe7 tof69ec6fComparenicolas-grekas commentedSep 8, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@GuilhemN FYI I push-forced on your fork after squashing the CS commit. See also the 2nd commit I added: it enables |
| namespaceSymfony\Component\Debug; | ||
| usePHPUnit\Framework\MockObject\Matcher\StatelessInvocation; |
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.
| * | ||
| * @see http://tools.ietf.org/html/rfc2616#section-10.3.5 | ||
| * | ||
| * @final |
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.
Required because StreamedResponse overrides this method. Otherwise, we'll seeThe "Symfony\Component\HttpFoundation\Response::setNotModified()" method is considered final. It may change without further notice as of its next major version. You should not extend it from "Symfony\Component\HttpFoundation\StreamedResponse". as e.g. inhttps://travis-ci.org/symfony/symfony/jobs/426209061
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.
(to be discussed; there might be other solutions than this one, wdyt?)
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.
We could ignore@final annotations from the same namespace like we do for@deprecated but imo this is not worth it and maybe we even don't want this behavior
GuilhemN commentedSep 8, 2018
👍 thanks for doing these changes, I hope your second commit will help symfony keep a clean codebase :) |
nicolas-grekas left a comment
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.
With this PR, DebugClassLoader will trigger a deprecation whenever a child method doesn't declare an argument while its parent does so via an@param annotation. This provides a forward path to adding new arguments to methods of base classes/interfaces in next major versions.
nicolas-grekas commentedSep 9, 2018
phpunit-bridge part split in#28412 |
| self::$finalMethods[$name] =array(); | ||
| self::$internalMethods[$name] =array(); | ||
| self::$annotatedParameters[$name] =array(); | ||
| foreach ($parentAndTraitsas$use) { |
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.
Do we want to support@param in interfaces (imo yes)? If so we should add a separate foreach to inheritannotatedParameters.
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.
yep we want to report@param added to interfaces for sure
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.
done
| } | ||
| // Method from a trait | ||
| if ($method->getFilename() !==$refl->getFileName()) { |
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 should be kept imo, I think we also don't want methods from traits being checked against their own@param annotations.
nicolas-grekasSep 9, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
worth a new test case :)
we also want to report implementations provided by traits that don't provide the argument declared by parents
and also to report missing arguments declared by@param on traits
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.
we also want to report implementations provided by traits that don't provide the argument declared by parents
👍, you were right about this change.
and also to report missing arguments declared by@param on traits
Thinking of it again, I prefer not parsing@param in traits directly since PHP allows a full overwriting of their methods, instead their methods are checked as if they belong to the sub classes using them.
…colas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[PhpUnitBridge] enable DebugClassLoader by default| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR |symfony/symfony-docs#10360With this PR, the phpunit-bridge will enable `DebugClassLoader` by default, making it do its job: throw deprecation notices at autoloading time. On top of#28329, this made me spot some glitches in the code base, fixed here also.This can be disabled by configuring the listener in `phpunit.xml.dist` files, adding `<element key="debug-class-loader"><integer>0</integer></element>` next to `<element key="time-sensitive">...`.Commits-------2fb11fc [PhpUnitBridge] enable DebugClassLoader by default
| self::$annotatedParameters[$name] =array(); | ||
| foreach ($parentAndTraitsas$use) { | ||
| foreach (array('finalMethods','internalMethods','annotatedParameters')as$property) { | ||
| foreach (array( |
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 CS is a bit weird here, better move the array in some local var
nicolas-grekas commentedSep 19, 2018
Could you please rebase (+squash) while fixing the small comment above to trigger tests? |
7cef7f2 toc03d5a1Comparenicolas-grekas commentedSep 21, 2018
Thank you@GuilhemN. |
…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
GuilhemN commentedSep 21, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thank you, you've made a significant part (most? 😅) of this PR :) |
Uh oh!
There was an error while loading.Please reload this page.
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
DebugClassLoaderand@paramannotations. If a@paramannotation is used on a parent but is then not actually implemented in the child class, a deprecation will be thrown.Example:
A deprecation will be triggered because
SubClassWithAnnotatedParameters::quzMethod()which doesn't definee$quz.