[Debug] Support `@final` on methods #21465

GuilhemN · 2017-01-30T18:32:08Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21461 (comment)
License	MIT
Doc PR

This adds support for @final on methods:

class Foo {
    /**
     * @final since version 2.0.
     */
    public function bar()
    {
    }
}

ping @nicolas-grekas

nicolas-grekas · 2017-01-30T20:08:18Z

src/Symfony/Component/Debug/DebugClassLoader.php

+            if ($parent) {
+                foreach ($refl->getMethods(\ReflectionMethod::IS_PUBLIC | \ReflectionMethod::IS_PROTECTED) as $method) {
+                    try {
+                        $prototype = $method->getPrototype();


Why is getPrototype needed? (I'm not very familiar with it :) )

I discovered it today, it is fetching the parent method, which is what we need, we could do that manually but we would have to support several edge cases (for instance a method of the parent of the parent extended).

@GuilhemN Very cool find. I've needed something similar in the past.

Nice finding.
Then I'm wondering if we need it at all :)
Parent classes are always loaded before their children. This means each class should only look at methods that have $method->class === $class (let's use properties as much as possible, performance is critical here).
By populating the self::$finalMethods props, then children should have everything to decide quickly.

@nicolas-grekas we would have to support the edge case I described above and maybe others i didn't think about, we would also have to parse every method doc blocks which is bad for perfs, imo it is better this way.

This means each class should only look at methods that have $method->class === $class

It looks like I forgot this check...

parsing the docblock (doing an strpos preflight really) should be faster than trying getPrototype, then fall in the catch block, which involves instanciating a ReflectionException behind the scene - especially since this situation is going to happen quite often (since many methods don't have a "protototype")

I tried it and the code is clearer too so 👍

nicolas-grekas · 2017-01-31T18:16:06Z

src/Symfony/Component/Debug/DebugClassLoader.php

+
+                if ($parent && isset(self::$finalMethods[$parent][$method->name])) {
+                    // Fetch the real class declaring the parent method
+                    $prototype = $method->getPrototype();


I guess you don't need this call if you compute the message when populating self::$finalMethods[$name][$method->name]?

Good point 👍

nicolas-grekas · 2017-01-31T18:16:46Z

src/Symfony/Component/Debug/DebugClassLoader.php

+                }
+
+                $doc = $method->getDocComment();
+                // No @final annotation


the code says it :)

nicolas-grekas · 2017-01-31T18:16:55Z

src/Symfony/Component/Debug/DebugClassLoader.php

+            self::$finalMethods[$name] = $parent && isset(self::$finalMethods[$parent]) ? self::$finalMethods[$parent] : array();
+
+            foreach ($refl->getMethods(\ReflectionMethod::IS_PUBLIC | \ReflectionMethod::IS_PROTECTED) as $method) {
+                // It's not the declaring class


ueless to me :)

nicolas-grekas · 2017-01-31T18:25:45Z

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

+
+        $xError = array(
+            'type' => E_USER_DEPRECATED,
+            'message' => 'The Symfony\Component\Debug\Tests\Fixtures\FinalMethod::finalMethod() method is considered final since version 3.3. It may change without further notice as of its next major version. You should not extend it from Symfony\Component\Debug\Tests\Fixtures\ExtendedFinalMethod.',


would it be possible to turn this into an @expectedDeprecation annotation?

Sure, I just followed the rest of the file, I guess it was done to not add the legacy group.

nicolas-grekas · 2017-02-02T14:00:57Z

src/Symfony/Component/Debug/DebugClassLoader.php

@@ -173,6 +174,32 @@ public function loadClass($class)
                @trigger_error(sprintf('The %s class is considered final%s. It may change without further notice as of its next major version. You should not extend it from %s.', $parent, self::$final[$parent], $name), E_USER_DEPRECATED);
            }

+            // Not an interface nor a trait
+            if (class_exists($name, false)) {


this should also apply to @final on the class above, isn't it?

Yes but I think we should wait for this to be merged before doing it, to avoid conflicts.

I don't see why?

In fact it's not a big deal, I can just rebase this PR if we do it in another PR.

nicolas-grekas

👍

fabpot · 2017-02-12T19:34:12Z

Thank you @GuilhemN.

@nicolas-grekas

This PR was squashed before being merged into the 3.3-dev branch (closes #21465). Discussion ---------- [Debug] Support `@final` on methods | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21461 (comment) | License | MIT | Doc PR | This adds support for `@final` on methods: ```php class Foo { /** * @Final since version 2.0. */ public function bar() { } } ``` ping @nicolas-grekas Commits ------- 1b0a6b6 [Debug] Support on methods

This PR was merged into the 3.3-dev branch. Discussion ---------- [HttpFoundation] Fix bad merge | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Bad merge, code had been removed in #21465 Commits ------- af1eed9 [HttpFoundation] Fix bad merge

carsonbot added Status: Needs Review Feature labels Jan 30, 2017

GuilhemN force-pushed the DEBUGMETHODS branch from 847f7b2 to ae936f8 Compare January 30, 2017 18:56

nicolas-grekas reviewed Jan 30, 2017

View reviewed changes

nicolas-grekas added this to the 3.3 milestone Jan 31, 2017

GuilhemN force-pushed the DEBUGMETHODS branch from a2e83ce to ea53d22 Compare January 31, 2017 18:07

nicolas-grekas reviewed Jan 31, 2017

View reviewed changes

nicolas-grekas changed the title ~~Support @final on methods~~ [Debug] Support @final on methods Jan 31, 2017

nicolas-grekas reviewed Jan 31, 2017

View reviewed changes

GuilhemN force-pushed the DEBUGMETHODS branch from f3bb16a to d1c9d49 Compare January 31, 2017 18:39

nicolas-grekas reviewed Feb 2, 2017

View reviewed changes

GuilhemN added 2 commits February 11, 2017 14:39

Support @Final on methods

17e9488

Don't parse interfaces/traits doc blocks for @Final

d1e0588

GuilhemN force-pushed the DEBUGMETHODS branch from d1c9d49 to d1e0588 Compare February 11, 2017 13:40

nicolas-grekas approved these changes Feb 11, 2017

View reviewed changes

fabpot closed this Feb 12, 2017

GuilhemN deleted the DEBUGMETHODS branch February 13, 2017 15:35

nicolas-grekas mentioned this pull request Mar 27, 2017

[HttpFoundation] Fix bad merge #22179

Merged

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Debug] Support `@final` on methods #21465

[Debug] Support `@final` on methods #21465

GuilhemN commented Jan 30, 2017

nicolas-grekas Jan 30, 2017

GuilhemN Jan 30, 2017

robfrawley Jan 30, 2017

nicolas-grekas Jan 31, 2017 •

edited

Loading

GuilhemN Jan 31, 2017

nicolas-grekas Jan 31, 2017 •

edited

Loading

GuilhemN Jan 31, 2017 •

edited

Loading

nicolas-grekas Jan 31, 2017 •

edited

Loading

GuilhemN Jan 31, 2017

nicolas-grekas Jan 31, 2017

nicolas-grekas Jan 31, 2017

nicolas-grekas Jan 31, 2017 •

edited

Loading

GuilhemN Jan 31, 2017

nicolas-grekas Jan 31, 2017

nicolas-grekas Feb 2, 2017

GuilhemN Feb 2, 2017

nicolas-grekas Feb 3, 2017

GuilhemN Feb 10, 2017

GuilhemN Feb 11, 2017

nicolas-grekas left a comment

fabpot commented Feb 12, 2017

[Debug] Support @final on methods #21465

[Debug] Support @final on methods #21465

Conversation

GuilhemN commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

GuilhemN Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Feb 12, 2017

[Debug] Support `@final` on methods #21465

[Debug] Support `@final` on methods #21465

nicolas-grekas Jan 31, 2017 •

edited

Loading

nicolas-grekas Jan 31, 2017 •

edited

Loading

GuilhemN Jan 31, 2017 •

edited

Loading

nicolas-grekas Jan 31, 2017 •

edited

Loading

nicolas-grekas Jan 31, 2017 •

edited

Loading