[Debug] Detect internal and deprecated methods #23816

GuilhemN · 2017-08-07T12:20:06Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
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.

nicolas-grekas · 2017-08-07T12:32:48Z

rebase unlocked - the same should also apply to internal methods, isn't it?

GuilhemN · 2017-08-07T12:35:32Z

Yes, I'm on it.

GuilhemN · 2017-08-07T13:22:53Z

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.

GuilhemN · 2017-08-07T17:37:25Z

fabbot failure is a false-positive. Travis and appveyor failures are unrelated.

nicolas-grekas · 2017-08-08T06:11:00Z

src/Symfony/Component/Debug/DebugClassLoader.php

+     *
+     * @return string[]
+     */
+    private function getOwnInterfaces($class)


$ownInterfaces = class_implements($class, false); if ($parent = get_parent_class($class)) { foreach (class_implements($parent, false) as $interface) { unset($ownInterfaces[$interface]); } } foreach ($ownInterfaces as $interface) { foreach (class_implements($interface) as $interface) { unset($ownInterfaces[$interface]); } } return $ownInterfaces;

nicolas-grekas · 2017-08-08T06:12:19Z

src/Symfony/Component/Debug/DebugClassLoader.php

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


could be merged with next line

nicolas-grekas · 2017-08-08T06:13:36Z

src/Symfony/Component/Debug/DebugClassLoader.php

@@ -361,4 +364,30 @@ public function loadClass($class)
            return true;
        }
    }
+
+    /**
+     * `class_implements` includes interfaces from the parents so we have to


I think you can configure your IDE to auto-break lines at 120, not 80 :)

nicolas-grekas · 2017-08-08T06:17:58Z

src/Symfony/Component/Debug/DebugClassLoader.php

+                }
+            }
+
+            $traits = class_uses($name, false);


$traits = array($parent) + class_uses($name, false);
Then below foreach ($interfaces + $traits to remove the array_merge() (directly $traits after, maybe renamed btw)

array($parent) + $interfaces; could cause conflicts so I did $parentAndTraits = ($parent ? array($parent => $parent) : array()) + class_uses($name, false); instead.

nicolas-grekas

with minor comments

nicolas-grekas · 2017-08-09T16:15:45Z

Thank you @GuilhemN.

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  | BC breaks? | no | Deprecations? | no  | 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

…(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  | BC breaks? | no | Deprecations? | no  | 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

@nicolas-grekas

…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  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | I just realized there were two issues in symfony#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 symfony#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

@nicolas-grekas

…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  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | I just realized there were two issues in symfony#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 symfony#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

@nicolas-grekas

…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  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | I just realized there were two issues in symfony#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 symfony#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

@nicolas-grekas

…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  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | I just realized there were two issues in symfony#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 symfony#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

@nicolas-grekas

…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  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | I just realized there were two issues in symfony#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 symfony#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

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

stof · 2017-10-19T14:58:10Z

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 setDefaultOptions for 2.7 and the new configureOptions for newer versions).

carsonbot added Status: Needs Review Debug Feature labels Aug 7, 2017

nicolas-grekas mentioned this pull request Aug 7, 2017

[Debug] Trigger a deprecation when using an internal class/trait/interface #23807

Closed

nicolas-grekas added this to the 3.4 milestone Aug 7, 2017

GuilhemN changed the title ~~[Debug] Detect deprecated methods~~ [Debug] Detect internal and deprecated methods Aug 7, 2017

GuilhemN force-pushed the deprecated branch 2 times, most recently from 6ba9fd2 to 0891882 Compare August 7, 2017 14:16

[Debug] Detect internal and deprecated methods

47c8632

GuilhemN force-pushed the deprecated branch from 0891882 to 47c8632 Compare August 7, 2017 15:02

nicolas-grekas reviewed Aug 8, 2017

View reviewed changes

nicolas-grekas approved these changes Aug 8, 2017

View reviewed changes

GuilhemN added 2 commits August 8, 2017 10:16

Fixes

87c39fb

fixes

a69c60d

GuilhemN force-pushed the deprecated branch from 1930c5c to a69c60d Compare August 8, 2017 08:25

nicolas-grekas added Status: Reviewed and removed Status: Needs Review labels Aug 8, 2017

chalasr closed this Aug 9, 2017

GuilhemN mentioned this pull request Aug 9, 2017

[Debug] Correctly detect methods not from the same vendor #23844

Merged

xabbuh mentioned this pull request Sep 12, 2017

[Debug] fix test assertion #24167

Merged

chalasr mentioned this pull request Oct 11, 2017

[TwigBridge] Unexpected deprecation notices for Twig\Extension::getName() #24522

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Debug] Detect internal and deprecated methods #23816

[Debug] Detect internal and deprecated methods #23816

GuilhemN commented Aug 7, 2017

nicolas-grekas commented Aug 7, 2017

GuilhemN commented Aug 7, 2017

GuilhemN commented Aug 7, 2017 •

edited

Loading

GuilhemN commented Aug 7, 2017

nicolas-grekas Aug 8, 2017

nicolas-grekas Aug 8, 2017 •

edited

Loading

GuilhemN Aug 8, 2017

nicolas-grekas Aug 8, 2017

GuilhemN Aug 8, 2017

nicolas-grekas Aug 8, 2017 •

edited

Loading

GuilhemN Aug 8, 2017

nicolas-grekas left a comment

nicolas-grekas commented Aug 9, 2017

stof commented Oct 19, 2017

[Debug] Detect internal and deprecated methods #23816

[Debug] Detect internal and deprecated methods #23816

Conversation

GuilhemN commented Aug 7, 2017

nicolas-grekas commented Aug 7, 2017

GuilhemN commented Aug 7, 2017

GuilhemN commented Aug 7, 2017 • edited Loading

GuilhemN commented Aug 7, 2017

nicolas-grekas Aug 8, 2017

Choose a reason for hiding this comment

nicolas-grekas Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

GuilhemN Aug 8, 2017

Choose a reason for hiding this comment

nicolas-grekas Aug 8, 2017

Choose a reason for hiding this comment

GuilhemN Aug 8, 2017

Choose a reason for hiding this comment

nicolas-grekas Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

GuilhemN Aug 8, 2017

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 9, 2017

stof commented Oct 19, 2017

GuilhemN commented Aug 7, 2017 •

edited

Loading

nicolas-grekas Aug 8, 2017 •

edited

Loading

nicolas-grekas Aug 8, 2017 •

edited

Loading