[Debug] Detect virtual methods using @method #28902

ro0NL · 2018-10-17T09:31:21Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#28897 (comment)
License	MIT
Doc PR	symfony/symfony-docs#10504

My first Debug PR, so im still on it. But early feedback welcome.

In #28901 we'll introduce a new virtual interface method using @method annotation. IIUC the idea is to trigger whenever such a method is overridden.

stof · 2018-10-17T09:59:21Z

No, the idea of @method on an interface is to trigger whenever a concrete class implementing the interface does not implement the method (directly or inherited from a parent class, we don't care).
The idea is to be able to add the method as an actual interface method in the next major.

stof · 2018-10-17T10:01:04Z

And only interface-level annotations should be considered IMO. Class-level @method annotation should not trigger deprecations when not being added in a child class, as such annotations are meant to document magic methods implement through __call

ro0NL · 2018-10-17T10:38:30Z

I've updated the regex for @method specific due newlines, and perhaps some future optimizing.

See https://regex101.com/r/Jw5R11/1 i think this is sufficient to move forward.

stof · 2018-10-17T10:57:02Z

src/Symfony/Component/Debug/DebugClassLoader.php

+                        continue;
+                    }
+
+                    if (false !== \strpos($doc, $annotation) && preg_match_all('#\n \* @'.$annotation.'(?:( .+?)\.?)(?=\r?\n \*(?: @|/$|\r?\n))#s', $doc, $notice)) {


you should look for @method here, not for method

stof · 2018-10-17T10:58:17Z

src/Symfony/Component/Debug/DebugClassLoader.php

@@ -223,7 +224,19 @@ public function checkAnnotations(\ReflectionClass $refl, $class)

        // Detect annotations on the class
        if (false !== $doc = $refl->getDocComment()) {
-            foreach (array('final', 'deprecated', 'internal') as $annotation) {
+            foreach (array('final', 'deprecated', 'internal', 'method') as $annotation) {
+                if ('method' === $annotation) {


as the implementation for @method is entirely different, I would not put it in the foreach (array('final', 'deprecated', 'internal') as $annotation) {. Put a separate block of code before or after the loop with the different handling.

src/Symfony/Component/Debug/Tests/DebugClassLoaderTest.php

ro0NL · 2018-10-17T12:00:06Z

This is fun :) I think i got to the tricky part, so last commit needs review :) deprecations so far:

array(7) {
  [0] =>
  string(173) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtualParent" should implement "sameLineInterfaceMethod()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualInterface""
  [1] =>
  string(181) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtualParent" should implement "sameLineInterfaceMethodNoBraces()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualInterface""
  [2] =>
  string(172) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtualParent" should implement "newLineInterfaceMethod()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualInterface""
  [3] =>
  string(180) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtualParent" should implement "newLineInterfaceMethodNoBraces()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualInterface""
  [4] =>
  string(172) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtualParent" should implement "invalidInterfaceMethod()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualInterface""
  [5] =>
  string(180) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtualParent" should implement "invalidInterfaceMethodNoBraces()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualInterface""
  [6] =>
  string(165) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtual" should implement "subInterfaceMethod()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualSubInterface""
}

ro0NL · 2018-10-17T12:08:33Z

The issue is with sameLineInterfaceMethodNoBraces, it triggers for ExtendsVirtualParent but is declared on ExtendsVirtual. I think we should skip if the parent is abstract/not-final so the current behavior is actually expected. Correct?

stof · 2018-10-17T12:55:05Z

abstract classes should be excluded from this check, as it is fine for them to provide incomplete implementations of an interface (a child class extending them would complain about the missing method).
not-final is irrelevant here. If you implement the interface in a concrete class, we must warn if you don't implement the extra method, otherwise adding the method in the interface would break the code.

src/Symfony/Component/Debug/DebugClassLoader.php

ro0NL · 2018-10-17T14:20:08Z

abstract classes should be excluded from this check, as it is fine for them to provide incomplete implementations of an interface

I think it works correct now 🎉

ro0NL · 2018-10-17T14:23:19Z

src/Symfony/Component/Debug/DebugClassLoader.php

@@ -249,6 +267,9 @@ public function checkAnnotations(\ReflectionClass $refl, $class)
            if (!isset(self::$checkedClasses[$use])) {
                $this->checkClass($use);
            }
+            if ($refl->isAbstract() && isset(self::$method[$use])) {
+                self::$method[$refl->name][] = $use;


so self::$method stores @methods per interface, or interfaces per abstract, and is resolved below as such. This way we preserve the original interface name. At least that's how i intended it :)

nicolas-grekas · 2018-10-17T14:24:55Z

src/Symfony/Component/Debug/DebugClassLoader.php

@@ -249,6 +267,9 @@ public function checkAnnotations(\ReflectionClass $refl, $class)
            if (!isset(self::$checkedClasses[$use])) {
                $this->checkClass($use);
            }
+            if ($refl->isAbstract() && isset(self::$method[$use])) {


operands should be swapped for performance

src/Symfony/Component/Debug/DebugClassLoader.php

ro0NL · 2018-10-17T15:56:39Z

https://github.com/nrk/predis/blob/v1.1/src/ClientInterface.php#L27 makes test fail, something wrong with description parsing but more important, it made me discover we should skip the deprecration if __call is declared :)

edit: ready!

ro0NL · 2018-10-18T07:24:43Z

From phpDocumentor/phpDocumentor#1855 i discovered we might be able to support static virtual methods as well.

Do we want to include it in this PR? Support it even?

ro0NL · 2018-10-18T07:29:30Z

Turns out it's supported as of phpdoc3 (https://github.com/phpDocumentor/phpDocumentor2/blob/v3.0.0-alpha1/docs/references/phpdoc/tags/method.rst)

I haven tested phpstorm yet, but if it works we might want to have a future proof implementation. IF virtual statics are worth it :)

nicolas-grekas · 2018-10-18T07:52:12Z

It's a few keystrokes away, isn't it? Let's do it :)

ro0NL · 2018-10-19T09:43:34Z

https://regex101.com/r/XLHqy5/3/ i made this :D

@method

This PR was squashed before being merged into the 4.1 branch (closes #28915). Discussion ---------- [Config] Fix @method annotation | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no  | Deprecations? | no | Tests pass? | yes  | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  ``` @method [[static] return type] [name]([[type] [parameter]<, ...>]) [<description>] ``` I dont think this was intended to be the method description, which will be taken into account after #28902 Let me know if there's a better description, other then `Gets the child node definitions` :) Commits ------- 0d7a961 [Config] Fix @method annotation

@method

This PR was squashed before being merged into the 4.2-dev branch (closes #28916). Discussion ---------- [Form] Fix @method annotation | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no  | Deprecations? | no | Tests pass? | yes  | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  Same as #28915 for 4.2 (these are the only occurrences for interfaces) Actually provides a use case for symfony/symfony#28902 (comment) 🎉 Commits ------- 13f0db718e [Form] Fix @method annotation

@method

This PR was squashed before being merged into the 4.2-dev branch (closes #28916). Discussion ---------- [Form] Fix @method annotation | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no  | Deprecations? | no | Tests pass? | yes  | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  Same as #28915 for 4.2 (these are the only occurrences for interfaces) Actually provides a use case for #28902 (comment) 🎉 Commits ------- 13f0db7 [Form] Fix @method annotation

@method

This PR was squashed before being merged into the 4.1 branch (closes symfony#28915). Discussion ---------- [Config] Fix @method annotation | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no  | Deprecations? | no | Tests pass? | yes  | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  ``` @method [[static] return type] [name]([[type] [parameter]<, ...>]) [<description>] ``` I dont think this was intended to be the method description, which will be taken into account after symfony#28902 Let me know if there's a better description, other then `Gets the child node definitions` :) Commits ------- 0d7a961 [Config] Fix @method annotation

ro0NL · 2018-10-20T06:37:32Z

Ready :)

Status: needs review

src/Symfony/Component/Debug/DebugClassLoader.php

fabpot · 2019-01-05T08:11:18Z

Thank you @ro0NL.

@method

This PR was squashed before being merged into the 4.3-dev branch (closes #28902). Discussion ---------- [Debug] Detect virtual methods using @method | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no  | Deprecations? | no | Tests pass? | yes  | Fixed tickets | #28897 (comment) | License | MIT | Doc PR | symfony/symfony-docs#10504 My first Debug PR, so im still on it. But early feedback welcome. In #28901 we'll introduce a new virtual interface method using `@method` annotation. IIUC the idea is to trigger whenever such a method is overridden. Commits ------- 38877c3 [Debug] Detect virtual methods using @method

carsonbot added Status: Needs Review Debug Feature labels Oct 17, 2018

nicolas-grekas added this to the next milestone Oct 17, 2018

stof reviewed Oct 17, 2018

View reviewed changes

ro0NL mentioned this pull request Oct 17, 2018

[Debug] New @method detection symfony/symfony-docs#10504

Closed

javiereguiluz reviewed Oct 17, 2018

View reviewed changes

src/Symfony/Component/Debug/DebugClassLoader.php Outdated Show resolved Hide resolved

ro0NL commented Oct 17, 2018

View reviewed changes

nicolas-grekas reviewed Oct 17, 2018

View reviewed changes

src/Symfony/Component/Debug/DebugClassLoader.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 17, 2018

This was referenced Oct 18, 2018

[Config] Fix @method annotation #28915

Merged

[Form] Fix @method annotation #28916

Merged

carsonbot added Status: Needs Review and removed Status: Needs Work labels Oct 20, 2018

nicolas-grekas approved these changes Dec 1, 2018

View reviewed changes

src/Symfony/Component/Debug/DebugClassLoader.php Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 1, 2018

This was referenced Dec 3, 2018

EntryManagerInterface is missing new functions #29433

Closed

[Ldap] Entry move support #29448

Merged

fabpot approved these changes Jan 5, 2019

View reviewed changes

[Debug] Detect virtual methods using @method

38877c3

fabpot merged commit 38877c3 into symfony:master Jan 5, 2019

ro0NL deleted the debug branch January 5, 2019 08:19

ro0NL mentioned this pull request Mar 20, 2019

[WIP] [Console] Pretty word wrapping #30590

Closed

nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

fabpot mentioned this pull request May 9, 2019

Release v4.3.0-BETA1 #31435

Merged

ogizanagi mentioned this pull request Aug 9, 2019

Introduce readablesFor method Elao/PhpEnums#73

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Debug] Detect virtual methods using @method #28902

[Debug] Detect virtual methods using @method #28902

ro0NL commented Oct 17, 2018 •

edited

Loading

stof commented Oct 17, 2018

stof commented Oct 17, 2018

ro0NL commented Oct 17, 2018

stof Oct 17, 2018

stof Oct 17, 2018

ro0NL commented Oct 17, 2018

ro0NL commented Oct 17, 2018 •

edited

Loading

stof commented Oct 17, 2018

ro0NL commented Oct 17, 2018

ro0NL Oct 17, 2018 •

edited

Loading

nicolas-grekas Oct 17, 2018

ro0NL commented Oct 17, 2018 •

edited

Loading

ro0NL commented Oct 18, 2018

ro0NL commented Oct 18, 2018

nicolas-grekas commented Oct 18, 2018

ro0NL commented Oct 19, 2018

ro0NL commented Oct 20, 2018

fabpot commented Jan 5, 2019

[Debug] Detect virtual methods using @method #28902

[Debug] Detect virtual methods using @method #28902

Conversation

ro0NL commented Oct 17, 2018 • edited Loading

stof commented Oct 17, 2018

stof commented Oct 17, 2018

ro0NL commented Oct 17, 2018

stof Oct 17, 2018

Choose a reason for hiding this comment

stof Oct 17, 2018

Choose a reason for hiding this comment

ro0NL commented Oct 17, 2018

ro0NL commented Oct 17, 2018 • edited Loading

stof commented Oct 17, 2018

ro0NL commented Oct 17, 2018

ro0NL Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Oct 17, 2018

Choose a reason for hiding this comment

ro0NL commented Oct 17, 2018 • edited Loading

ro0NL commented Oct 18, 2018

ro0NL commented Oct 18, 2018

nicolas-grekas commented Oct 18, 2018

ro0NL commented Oct 19, 2018

ro0NL commented Oct 20, 2018

fabpot commented Jan 5, 2019

ro0NL commented Oct 17, 2018 •

edited

Loading

ro0NL commented Oct 17, 2018 •

edited

Loading

ro0NL Oct 17, 2018 •

edited

Loading

ro0NL commented Oct 17, 2018 •

edited

Loading