Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Debug] Detect internal and deprecated methods#23816
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
Closed
Closed
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
rebase unlocked - the same should also apply to internal methods, isn't it? |
Yes, I'm on it. |
ContributorAuthor
GuilhemN commentedAug 7, 2017 • 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.
rebased Edit: known issue, when you extend a method of an inherited trait, the deprecation will show the name of the class using the trait instead of the name of the trait itself. I didn't find a workaround but I think it's still better than no warning at all. |
6ba9fd2 to0891882Comparefabbot failure is a false-positive. Travis and appveyor failures are unrelated. |
| * | ||
| * @return string[] | ||
| */ | ||
| private function getOwnInterfaces($class) |
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.
$ownInterfaces =class_implements($class,false);if ($parent =get_parent_class($class)) {foreach (class_implements($parent,false)as$interface) { unset($ownInterfaces[$interface]); } }foreach ($ownInterfacesas$interface) {foreach (class_implements($interface)as$interface) { unset($ownInterfaces[$interface]); } }return$ownInterfaces;
| if (!isset($parentInterfaces[$interface])) { | ||
| @trigger_error(sprintf('The "%s" %s "%s" that is deprecated %s', $name, $refl->isInterface() ? 'interface extends' : 'class implements', $interface, self::$deprecated[$interface]), E_USER_DEPRECATED); | ||
| // Detect method annotations | ||
| $doc = $method->getDocComment(); |
Member
nicolas-grekasAug 8, 2017 • 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.
could be merged with next line
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
| @@ -361,4 +364,30 @@ public function loadClass($class) | |||
| return true; | |||
| } | |||
| } | |||
| /** | |||
| * `class_implements` includes interfaces from the parents so we have to | |||
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 think you can configure your IDE to auto-break lines at 120, not 80 :)
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 :)
| } | ||
| } | ||
| $traits = class_uses($name, false); |
Member
nicolas-grekasAug 8, 2017 • 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.
$traits = array($parent) + class_uses($name, false);
Then belowforeach ($interfaces + $traits to remove the array_merge() (directly$traits after, maybe renamed btw)
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.
array($parent) + $interfaces; could cause conflicts so I did$parentAndTraits = ($parent ? array($parent => $parent) : array()) + class_uses($name, false); instead.
nicolas-grekas approved these changesAug 8, 2017
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 minor comments
Thank you@GuilhemN. |
nicolas-grekas added a commit that referenced this pull requestAug 9, 2017
This PR was squashed before being merged into the 3.4 branch (closes#23816).Discussion----------[Debug] Detect internal and deprecated methods| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | yes <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |I just realized we do not detect `@deprecated` methods like we do for `@final` methods, so here's a fix.I reorganized the file to avoid code duplication... so the diff is kind of impressive but the behavior is the same.Commits-------16eeabf [Debug] Detect internal and deprecated methods
nicolas-grekas added a commit that referenced this pull requestAug 9, 2017
…(GuilhemN)This PR was merged into the 3.4 branch.Discussion----------[Debug] Correctly detect methods not from the same vendor| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |I just realized there were two issues in#23816:* when a class extended another class from the same vendor itself extending a class from another vendor containing deprecated methods, the deprecation wasn't triggered in case the first class extended a deprecated method from the last class because the parent from the same vendor was used for "the same vendor check".* As stated in#23816 (comment), ReflectionMethod::$class doesn't contain the right value when using a trait so if you extended a trait containing methods from another vendor you would have always get deprecations. I fixed this by comparing the method file to the filename of the class using the trait.@nicolas-grekas are you aware of any issue of `ReflectionMethod::getFilename()`?Commits-------08d352a [Debug] Correctly detect methods not from the same vendor
jderusse pushed a commit to jderusse/symfony that referenced this pull requestAug 23, 2017
…vendor (GuilhemN)This PR was merged into the 3.4 branch.Discussion----------[Debug] Correctly detect methods not from the same vendor| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |I just realized there were two issues insymfony#23816:* when a class extended another class from the same vendor itself extending a class from another vendor containing deprecated methods, the deprecation wasn't triggered in case the first class extended a deprecated method from the last class because the parent from the same vendor was used for "the same vendor check".* As stated insymfony#23816 (comment), ReflectionMethod::$class doesn't contain the right value when using a trait so if you extended a trait containing methods from another vendor you would have always get deprecations. I fixed this by comparing the method file to the filename of the class using the trait.@nicolas-grekas are you aware of any issue of `ReflectionMethod::getFilename()`?Commits-------08d352a [Debug] Correctly detect methods not from the same vendor
jderusse pushed a commit to jderusse/symfony that referenced this pull requestAug 23, 2017
…vendor (GuilhemN)This PR was merged into the 3.4 branch.Discussion----------[Debug] Correctly detect methods not from the same vendor| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |I just realized there were two issues insymfony#23816:* when a class extended another class from the same vendor itself extending a class from another vendor containing deprecated methods, the deprecation wasn't triggered in case the first class extended a deprecated method from the last class because the parent from the same vendor was used for "the same vendor check".* As stated insymfony#23816 (comment), ReflectionMethod::$class doesn't contain the right value when using a trait so if you extended a trait containing methods from another vendor you would have always get deprecations. I fixed this by comparing the method file to the filename of the class using the trait.@nicolas-grekas are you aware of any issue of `ReflectionMethod::getFilename()`?Commits-------08d352a [Debug] Correctly detect methods not from the same vendor
jderusse pushed a commit to jderusse/symfony that referenced this pull requestAug 23, 2017
…vendor (GuilhemN)This PR was merged into the 3.4 branch.Discussion----------[Debug] Correctly detect methods not from the same vendor| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |I just realized there were two issues insymfony#23816:* when a class extended another class from the same vendor itself extending a class from another vendor containing deprecated methods, the deprecation wasn't triggered in case the first class extended a deprecated method from the last class because the parent from the same vendor was used for "the same vendor check".* As stated insymfony#23816 (comment), ReflectionMethod::$class doesn't contain the right value when using a trait so if you extended a trait containing methods from another vendor you would have always get deprecations. I fixed this by comparing the method file to the filename of the class using the trait.@nicolas-grekas are you aware of any issue of `ReflectionMethod::getFilename()`?Commits-------08d352a [Debug] Correctly detect methods not from the same vendor
jderusse pushed a commit to jderusse/symfony that referenced this pull requestAug 23, 2017
…vendor (GuilhemN)This PR was merged into the 3.4 branch.Discussion----------[Debug] Correctly detect methods not from the same vendor| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |I just realized there were two issues insymfony#23816:* when a class extended another class from the same vendor itself extending a class from another vendor containing deprecated methods, the deprecation wasn't triggered in case the first class extended a deprecated method from the last class because the parent from the same vendor was used for "the same vendor check".* As stated insymfony#23816 (comment), ReflectionMethod::$class doesn't contain the right value when using a trait so if you extended a trait containing methods from another vendor you would have always get deprecations. I fixed this by comparing the method file to the filename of the class using the trait.@nicolas-grekas are you aware of any issue of `ReflectionMethod::getFilename()`?Commits-------08d352a [Debug] Correctly detect methods not from the same vendor
jderusse pushed a commit to jderusse/symfony that referenced this pull requestAug 23, 2017
…vendor (GuilhemN)This PR was merged into the 3.4 branch.Discussion----------[Debug] Correctly detect methods not from the same vendor| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |I just realized there were two issues insymfony#23816:* when a class extended another class from the same vendor itself extending a class from another vendor containing deprecated methods, the deprecation wasn't triggered in case the first class extended a deprecated method from the last class because the parent from the same vendor was used for "the same vendor check".* As stated insymfony#23816 (comment), ReflectionMethod::$class doesn't contain the right value when using a trait so if you extended a trait containing methods from another vendor you would have always get deprecations. I fixed this by comparing the method file to the filename of the class using the trait.@nicolas-grekas are you aware of any issue of `ReflectionMethod::getFilename()`?Commits-------08d352a [Debug] Correctly detect methods not from the same vendor
xabbuh added a commit that referenced this pull requestSep 12, 2017
This PR was merged into the 3.4 branch.Discussion----------[Debug] fix test assertion| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |Fixes tests for#23816 after the changes made in#24149.Commits-------75a26bb [Debug] fix test assertion
This cause issue when you still need to extend the old method to keep support for older versions (look at the way bundles were able to support both Symfony 2.7 and 2.8 to configure options in form types, by implementing both the old |
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I just realized we do not detect
@deprecatedmethods like we do for@finalmethods, so here's a fix.I reorganized the file to avoid code duplication... so the diff is kind of impressive but the behavior is the same.