NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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

Jump to bottom

Closed

GuilhemN wants to merge2 commits intosymfony:masterfromGuilhemN:DEBUGMETHODS

Closed

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

GuilhemN wants to merge2 commits intosymfony:masterfromGuilhemN:DEBUGMETHODS

Conversation

Copy link

Contributor

GuilhemN commentedJan 30, 2017

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.     */publicfunctionbar()    {    }}

ping@nicolas-grekas

carsonbot added Status: Needs Review Feature labels

Jan 30, 2017

GuilhemN force-pushed theDEBUGMETHODS branch from847f7b2 toae936f8Compare

January 30, 2017 18:56

nicolas-grekas reviewed

Jan 30, 2017

View reviewed changes

src/Symfony/Component/Debug/DebugClassLoader.php Outdated

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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).

Copy link

Contributor

robfrawleyJan 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

nicolas-grekasJan 31, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 theself::$finalMethods props, then children should have everything to decide quickly.

Copy link

ContributorAuthor

GuilhemNJan 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@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...

Copy link

Member

nicolas-grekasJan 31, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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")

Copy link

ContributorAuthor

GuilhemNJan 31, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I tried it and the code is clearer too so 👍

nicolas-grekas added this to the3.3 milestone

Jan 31, 2017

GuilhemN force-pushed theDEBUGMETHODS branch froma2e83ce toea53d22Compare

January 31, 2017 18:07

nicolas-grekas reviewed

Jan 31, 2017

View reviewed changes

src/Symfony/Component/Debug/DebugClassLoader.php Outdated


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

Copy link

Member

nicolas-grekasJan 31, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

GuilhemNJan 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good point 👍

nicolas-grekas reviewed

Jan 31, 2017

View reviewed changes

src/Symfony/Component/Debug/DebugClassLoader.php Outdated

		}

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

Copy link

Member

nicolas-grekasJan 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

the code says it :)

nicolas-grekas reviewed

Jan 31, 2017

View reviewed changes

src/Symfony/Component/Debug/DebugClassLoader.php Outdated

		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

Copy link

Member

nicolas-grekasJan 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

ueless to me :)

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

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.',

Copy link

Member

nicolas-grekasJan 31, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

GuilhemNJan 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

nicolas-grekasJan 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

indeed!

GuilhemN force-pushed theDEBUGMETHODS branch fromf3bb16a tod1c9d49Compare

January 31, 2017 18:39

nicolas-grekas reviewed

Feb 2, 2017

View reviewed changes

src/Symfony/Component/Debug/DebugClassLoader.php

		}

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

Copy link

Member

nicolas-grekasFeb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

GuilhemNFeb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

nicolas-grekasFeb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't see why?

Copy link

ContributorAuthor

GuilhemNFeb 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

GuilhemNFeb 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

done

GuilhemN added2 commits

February 11, 2017 14:39

Support@Finalon methods

17e9488

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

d1e0588

GuilhemN force-pushed theDEBUGMETHODS branch fromd1c9d49 tod1e0588Compare

February 11, 2017 13:40

nicolas-grekas approved these changes

Feb 11, 2017

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍

Copy link

Member

fabpot commentedFeb 12, 2017

Thank you@GuilhemN.

fabpot closed this

Feb 12, 2017

fabpot added a commit that referenced this pull request

Feb 12, 2017

feature#21465[Debug] Support@finalon methods (GuilhemN)

6d3d17d

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:```phpclass Foo {    /**     *@Final since version 2.0.     */    public function bar()    {    }}```ping@nicolas-grekasCommits-------1b0a6b6 [Debug] Support  on methods

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

nicolas-grekas added a commit that referenced this pull request

Mar 27, 2017

minor#22179[HttpFoundation] Fix bad merge (nicolas-grekas)

195e464

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#21465Commits-------af1eed9 [HttpFoundation] Fix bad merge

fabpot mentioned this pull request

May 1, 2017

Release v3.3.0-BETA1#22603

Merged

Labels

Feature Status: Needs Review

5 participants

Movatterモバイル変換

Uh oh!

[Debug] Support@final on methods#21465

[Debug] Support@final on methods#21465

Uh oh!

Conversation

GuilhemN commentedJan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasJan 31, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasJan 31, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuilhemNJan 31, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasJan 31, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasJan 31, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

fabpot commentedFeb 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

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

nicolas-grekasJan 31, 2017•
edited
Loading

nicolas-grekasJan 31, 2017•
edited
Loading

GuilhemNJan 31, 2017•
edited
Loading

nicolas-grekasJan 31, 2017•
edited
Loading

nicolas-grekasJan 31, 2017•
edited
Loading