8000 [Debug] Trigger deprecation on `@final` annotation in DebugClassLoader - prepare making some classes final by GuilhemN · Pull Request #20493 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Debug] Trigger deprecation on @final annotation in DebugClassLoader - prepare making some classes final #20493

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 15, 2017

Conversation

GuilhemN
Copy link
Contributor
@GuilhemN GuilhemN commented Nov 11, 2016
Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets follows #19734
License MIT
Doc PR

BC promises become quickly huge but making classes final can limit these promises.
At the same time, many classes of the symfony codebase are not meant to be extended and could be final; that's the goal of this PR: prepare making them final in 4.0 by triggering deprecations in their constructor:

public function __construct()
{
    if (__CLASS__ !== get_class($this)) {
        @trigger_error(sprintf('Extending %s is deprecated since 3.3 and won\'t be supported in 4.0 as it will be final.', __CLASS__), E_USER_DEPRECATED);
    }
}

I updated two classes for now but we can do much more if you like it.

@@ -18,6 +18,13 @@
*/
class NullContext implements ContextInterface
{
public function __construct()
{
if (__CLASS__ !== get_class($this)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this won't be called if the extended class doesn't call parent::construct() will it?

Copy link
Contributor Author
@GuilhemN GuilhemN Nov 11, 2016

Choose a reason for hiding this comment

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

That's not very clear whether it's supported by sf's bc policy but in the past we have already supposed that the parent constructor must always be called to have a valid object.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe to be safe you could add a @final tag and give a thumb up for https://youtrack.jetbrains.com/issue/WI-31225 :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As that's not supported by IDEs i don't think that's worth it (without talking about the fact that having an annotation for a keyword supported by php looks weird to me :P).

Copy link
Contributor
@theofidry theofidry Nov 11, 2016

Choose a reason for hiding this comment

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

without talking about the fact that having an annotation for a keyword supported by php looks weird to me :P

I strongly disagree here: if we were in a perfect world no problem, but if you are using proxy manager for example you can't proxy a final service because the proxying is done via inheritance. There is also the case where for practical reason you can't declare it as final, but you wish to convey the information that a given class/method is not meant to be extended/overridden.

And for the IDE auto-completion, Symfony used to use the @api tag for a while and @internal or @private are equally used. IDE support comes with usage, not vice-versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stof then should we only make final classes that already have a constructor?

Copy link
Contributor

Choose a reason for hiding this comment

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

I strongly disagree here: if we were in a perfect world no problem, but if you are using proxy manager for example you can't proxy a final service because the proxying is done via inheritance

If you type hint against an interface, then proxy manager has to be smart enough to proxy an interface (I saw your issue few days ago and I hope it will be fixed on configuration level). After all, it's not that crucial to have ability to proxy everything. We are not in perfect world as you said.

Copy link
Contributor
@theofidry theofidry Nov 12, 2016

Choose a reason for hiding this comment

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

@unkind we should discuss about that in that ticket, but I don't see what you mean, declaring a service which class is final and registered as lazy will cause the error, regardless of its interface(s) or parent class(es)

@stof edited

Copy link
Member

Choose a reason for hiding this comment

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

@unkind proxying is defined at the service level, not at the argument level. So the ProxyManager does not know what are typehints in places using the service (and btw, if the service is public, nothing can know it without analyzing the full project)

Copy link
Contributor
@unkind unkind Nov 12, 2016

Choose a reason for hiding this comment

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

@stof I understand how it works, my point was just if you couple to an interface, you can fix it at infrastructure level. But if you are coupled to a final class somewhere in your code, you are not able to use proxy manager.

By "fixing at infrastructure level" I mean your suggestion, for example (#20392):

This would not be hard to implement. We could just add another attribute alongside lazy (or allow passing lazy: className)

@theofidry

but I don't see what you mean, declaring a service which class is final and registered as lazy will cause the error

It causes the error now, but it can be fixed with @stof's idea, for instance.

@greg0ire
Copy link
Contributor
greg0ire commented Nov 12, 2016

Nice detection trick! The code sample in your PR body should use !== instead of === though.

@GuilhemN
Copy link
Contributor Author

@greg0ire indeed, thanks ! ☺

@dunglas
Copy link
Member
dunglas commented Nov 13, 2016

On the same topic, maybe can we make all new classes final by default (for implementations of interfaces and Value Objects) and encourage people to use decoration instead of extending.
It's in the spirit of what we're already doing for properties (private by default) and it eases the maintenance a lot. We can always remove the final keyword later when needed (good use case, proxying...).

We already follow this strategy for API Platform and are happy with the result.

@GuilhemN
Copy link
Contributor Author

@dunglas 👍
BTW same applies for methods: most could be final and extension points would be better identified.

@unkind
Copy link
Contributor
unkind commented Nov 13, 2016

@dunglas completely agree, it was discussed in the past, though: #15233.

@SoboLAN
Copy link
SoboLAN commented Nov 15, 2016

If you mark a lot of classes as final then, even if people will use composition/decoration (or even traits) instead of extending, it will break unit-tests.

Per PHPUnit documentation:

Please note that final, private and static methods cannot be stubbed or mocked. They are ignored by PHPUnit's test double functionality and retain their original behavior.

Therefore in my tests I won't be able to mock the Symfony objects that I inject in my classes. I know the quote above specifically mentions methods, but it applies to classes as well.

Some of you will say to simply mock the corresponding interface, but the truth is that a loooot of Symfony functionality is not exposed through interfaces.

@greg0ire
Copy link
Contributor

@SoboLAN : I hit that problem on a project lately, but then there is this saying "don't mock what you don't own". Not mocking what you don't own can have its benefits indeed, for instance, in the case of symfony, you will get deprecation messages and realize that you are misusing the class, which will help you migrate to something forward compatible.

@unkind
Copy link
Contributor
unkind commented Nov 15, 2016

@SoboLAN don't mock concrete classes, use the new operator.

@GuilhemN
Copy link
Contributor Author
GuilhemN commented Nov 15, 2016

@SoboLAN most of the time mocking a third party object is useless (a class not mocked would work fine as well) or the corresponding test is almost a copy-paste of the tested class (test of every calls, etc.).
From my point of view, mocking is very rarely necessary when there is no abstraction and we should first test classes output. Classes that are very common to extend (and could need to be extended) either implement an interface or are not final.

In the worst case, if someone has a valid use-case about a final class that shouldn't be final, opening an issue and explaining the problem should be enough and it will be solved in the next version.

Same apply for private arguments. Unless someone asks for it I don't see why sf's promises should cover stuff that we are not even sure will be used one day.

@GuilhemN
Copy link
Contributor Author
GuilhemN commented Nov 15, 2016

As @stof and @theofidry said, it looks hard to trigger deprecations when classes don't already have a constructor.
So I updated the chained encoder/decoder of the serializer component instead as an example. We'll probably not be able to make final classes that don't already have a constructor (that's one more reason to make classes final by default as this may not be doable afterwards).

If we agree to do this, we'll have to list classes that we want to make final. I'm first thinking about ChainAdapter, ChainEncoder, Chain... but they are many other classes that shouldn't be extended.

@stof
Copy link
Member
stof commented Nov 16, 2016

There is a different way to trigger these warnings: doing it in the DebugClassLoader, as we do for deprecated interfaces being implemented. This way, it does not rely on having a parent constructor being called.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Nov 16, 2016

👍 for @stof proposal! DebugClassLoader should look for @final annotations and trigger a deprecation. Then trigger an exception in 4.0.

@stof
Copy link
Member
stof commented Nov 16, 2016

No, DebugClassLoader should not throw an exception in 4.0, as it would prevent adding @final in a 4.x version. what will happen in 4.0 is marking classes as final for real wherever needed (and then, PHP itself will trigger the fatal error)

@nicolas-grekas
Copy link
Member

marking classes as final for real

that would not be always desired, esp. to play nice with lazy proxies.

@unkind
Copy link
Contributor
unkind commented Nov 16, 2016

that would not be always desired, esp. to play nice with lazy proxies.

I doubt there is a valid case to make proxy for VOs, classes with interfaces, listeners, commands, controllers, etc. In other words: proxying concrete classes looks like a hack.

@theofidry
Copy link
Contributor
theofidry commented Nov 16, 2016

@unkind #20392 needs to be solved for 4.0 then

@unkind
Copy link
Contributor
unkind commented Nov 16, 2016

@unkind #20392 needs to be solved first then

Yes, but as far as I see that's not that hard.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Nov 16, 2016

@theofidry not first, there is no blocker, let's move forward. As @unkind noted there are many classes that have no reason to be turned into services/proxies.

@GuilhemN
Copy link
Contributor Author

I moved deprecations to the DebugClassLoader (thanks @stof for the idea!).

@@ -187,6 +197,9 @@ public function loadClass($class)
if ($parent && isset(self::$deprecated[$parent]) && strncmp($ns, $parent, $len)) {
@trigger_error(sprintf('The %s class extends %s that is deprecated %s', $name, $parent, self::$deprecated[$parent]), E_USER_DEPRECATED);
}
if ($parent && isset(self::$final[$parent]) && strncmp($ns, $parent, $len)) {
@trigger_error(sprintf('Extending the class %s in %s is deprecated and may break in the next major release.', $parent, $name), E_USER_DEPRECATED);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this message, do you see a better alternative?

Copy link
Contributor

Choose a reason for hiding this comment

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

What would be wrong with it exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't contain the versions in which the deprecation was added and when it will be applied compared to deprecation messages but maybe that's enough.

Copy link
Member

Choose a reason for hiding this comment

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

you don't add a deprecation, you add a @final tag, which does not have a reason to include a version IMO.
and btw, the next major version could decide to keep the class as virtually final instead of making the final for real, if they want to keep them mockable for instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would imply changing the annotation to something like @final extending is deprecated since 42.3, to be made final in 43 and parse that… too complicated IMO and the value of this piece of information is questionable.

Copy link
Contributor Author
@GuilhemN GuilhemN Nov 16, 2016

Choose a reason for hiding this comment

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

@greg0ire we could do as for @deprecated tags and prepend the triggered message with the annotation description. I'm not sure that's worth it too so let's leave it as is for now :)

@stof if people mock a class having a @final tag, a deprecation will be triggered; I don't think that's acceptable if we want to allow it. If we want to keep some @final annotation virtual, then I think we should add a special case when we want to allow mocks (we could add something special in the annotation description? what about @final only mocks are allowed?).

@@ -165,9 +166,18 @@ public function loadClass($class)

if (in_array(strtolower($refl->getShortName()), self::$php7Reserved)) {
@trigger_error(sprintf('%s uses a reserved class name (%s) that will break on PHP 7 and higher', $name, $refl->getShortName()), E_USER_DEPRECATED);
} elseif (preg_match('#\n \* @deprecated (.*?)\r?\n \*(?: @|/$)#s', $refl->getDocComment(), $notice)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this elseif to an if as there were no reason imo to not continue triggering deprecations after realizing that the name is not allowed on php7. I can't revert it if you think it should be done in another pr.

Copy link
Member

Choose a reason for hiding this comment

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

not related to this PR, and we don't need to trigger two notices here, I'd revert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

} elseif (preg_match('#\n \* @deprecated (.*?)\r?\n \*(?: @|/$)#s', $refl->getDocComment(), $notice)) {
}

if (preg_match('#\n \* @final( .*)?\r?\n \*(?: @|/$)#s', $refl->getDocComment())) {
Copy link
Member

Choose a reason for hiding this comment

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

this does not need to be done for interfaces (marking an interface as final does not make any sense). Add $refl->isClass()

Copy link
Contributor Author
@GuilhemN GuilhemN Nov 16, 2016

Choose a reason for hiding this comment

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

\ReflectionClass::isClass() doesn't exist and I didn't find a simple way to do the same.
After thinking a bit about it I decided to not had this check as no deprecation will be triggered later anyway (because of the get_parent_class call).

Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to use the text after @final is there is any?

Copy link
Contributor Author
@GuilhemN GuilhemN Dec 3, 2016

Choose a reason for hiding this comment

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

I don't know, do you think we should use it to indicate a version and/or what this annotation means?

self::$final[$name] = true;
}

if (preg_match('#\n \* @deprecated (.*?)\r?\n \*(?: @|/$)#s', $refl->getDocComment(), $notice)) {
self::$deprecated[$name] = preg_replace('#\s*\r?\n \* +#', ' ', $notice[1]);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

using else here looks wrong to me, as the logic now also deals with wanna-be-final parent classes.

Btw, the logic allowing a class to extend/implement a deprecated class/interface if it is in the same namespace does not apply to the logic for final classes IMO, so you should not mix this in the same code block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this else was here to prevent deprecated classes to trigger more deprecations when they also implement/extend something else deprecated.

Btw, the logic allowing a class to extend/implement a deprecated class/interface if it is in the same namespace does not apply to the logic for final classes IMO, so you should not mix this in the same code block

I agree 👍, I moved it.

public function getClassMap()
{
return array(__NAMESPACE__.'\Fixtures\NotPSR0bis' => __DIR__.'/Fixtures/notPsr0Bis.php');
}
Copy link
Member

Choose a reason for hiding this comment

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

Removing this looks wrong to me

Copy link
Contributor Author
@GuilhemN GuilhemN Nov 16, 2016

Choose a reason for hiding this comment

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

From what I saw this is not used and this class is already managed in findFile.

Copy link
Member

Choose a reason for hiding this comment

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

unrelated, should be reverted

if (preg_match('#\n \* @final( .*)?\r?\n \*(?: @|/$)#s', $refl->getDocComment())) {
self::$final[$name] = true;
}
$parent = get_parent_class($class);
Copy link
Member

Choose a reason for hiding this comment

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

you can remove the call to get_parent_class below


$parent = get_parent_class($class);
if ($parent && isset(self::$final[$parent])) {
@trigger_error(sprintf('Extending the class %s in %s is deprecated and may break in the next major release.', $parent, $name), E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we throw here? DebugClassLoader is a dev tool so it's not like we're going to break a prod site.
But maybe throw in 4.0 yes. Then the message should be clear, in "may", don't you think?

Copy link
Member
@stof stof Jan 6, 2017

Choose a reason for hiding this comment

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

if you throw, it means that adding @final somewhere is a BC break, as it forces everyone to update immediately (breaking the dev environment is as bad as breaking the prod one, as your BC would then be valid only for people not using the dev environment to continue working on their project, which means nobody sane).

And we will never throw an exception here (it would suffer from the same issue for new @final annotations added in a package used alongside symfony/debug 4.x).
the may is here because the class being wanna-be-final may be turned into a real final class in the next major version of its package (which has nothing to do with the next Symfony major version) if the goal was to move to real final classes. But some package owners may decide to keep them wanna-be-final forever to allow mocking them, so we cannot be sure that the code will break.

@nicolas-grekas nicolas-grekas modified the milestones: 3.3, 3.x Jan 6, 2017
@nicolas-grekas
Copy 10000 link
Member

if you throw, it means that adding @Final somewhere is a BC break, as it forces everyone to update immediately (breaking the dev environment is as bad as breaking the prod one, as your BC would then be valid only for people not using the dev environment to continue working on their project, which means nobody sane).

And we will never throw an exception here (it would suffer from the same issue for new @Final annotations added in a package used alongside symfony/debug 4.x).
the may is here because the class being wanna-be-final may be turned into a real final class in the next major version of its package (which has nothing to do with the next Symfony major version) if the goal was to move to real final classes. But some package owners may decide to keep them wanna-be-final forever to allow mocking them, so we cannot be sure that the code will break.

got it
👍 then

@nicolas-grekas nicolas-grekas changed the title Prepare making some classes final in 4.0 [Debug] Trigger deprecation on @final annotation in DebugClassLoader - prepare making some classes final Jan 6, 2017
@GuilhemN
Copy link
Contributor Author
GuilhemN commented Jan 6, 2017

@nicolas-grekas if you did not understand it, the message is not clear enough.
What about Extending the class %s in %s is deprecated. This class may be made final and break your code in the next major release.?

@nicolas-grekas
Copy link
Member

LGTM


$parent = get_parent_class($class);
if ($parent && isset(self::$final[$parent])) {
@trigger_error(sprintf('Extending the class %s in %s is deprecated. This class may be made final and break your code in the next major release.', $parent, $name), E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

Why may be made instead of will be made?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As stated by @stof:

But some package owners may decide to keep them wanna-be-final forever to allow mocking them, so we cannot be sure that the code will break.

Copy link
Member

Choose a reason for hiding this comment

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

But the intention is to make it final, enforced or not. So, the message must say that extending this class must not be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

message updated

if (in_array(strtolower($refl->getShortName()), self::$php7Reserved)) {
@trigger_error(sprintf('%s uses a reserved class name (%s) that will break on PHP 7 and higher', $name, $refl->getShortName()), E_USER_DEPRECATED);
} elseif (preg_match('#\n \* @deprecated (.*?)\r?\n \*(?: @|/$)#s', $refl->getDocComment(), $notice)) {
self::$deprecated[$name] = preg_replace('#\s*\r?\n \* +#', ' ', $notice[1]);
} else {
// Don't trigger deprecations for classes in the same vendor
Copy link
Member

Choose a reason for hiding this comment

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

This comment does not really match the tested behaviour, does it?

Copy link
Contributor Author
@GuilhemN GuilhemN Jan 14, 2017

Choose a reason for hiding this comment

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

Isn't it? From what I understood, when a class (let's say App\Controller\AppController) is deprecated, deprecations won't be triggered when for example App\Controller\BlogController extends it.
Would you prefer:

// Don't trigger deprecations for classes in the same sub namespace
// (e.g. a deprecated class `App\Foo\Deprecated` won't trigger a deprecation
// when extended by `App\Foo\NewClass` but when extended by `App\Quz\Bar`, it will).
// For Symfony components/bundles/bridges, this rule applies to the root 
// namespace (e.g. `Symfony`).

?

If that locks us I can do it in another PR, it was just the occasion to clarify what this check does ;)

Copy link
Member

Choose a reason for hiding this comment

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

Ah no, I misunderstood the reasoning for adding it. Looks good to me.

@nicolas-grekas
Copy link
Member

Reading #21263 (comment), I think we need to add the version where the @final has been added in the depreciation message also, and annotations be written as @final since version 3.3

@nicolas-grekas
Copy link
Member
BD94 nicolas-grekas commented Jan 12, 2017

The message could be something like:
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, $msgWithFirstUppercaseAndNewlinesAndSpacesAndFinalDotCleanedUp, $class)

@keradus
Copy link
Member
keradus commented Jan 12, 2017

If we will treat class with @final annotation as final indeed and start playing with protected properties/methods without bumping the MAJOR version then I'm strongly against this move here.
If we will bump MAJOR version while playing with interface then it's good as not a bc breaker.

I'm totally for putting a deprecation notice, just let the notice not be an excuse to break BC.

Indeed, sth like @final since 3.3 should prevent devs breaking BC on 3.x line.

@keradus keradus mentioned this pull request Jan 12, 2017

$parent = get_parent_class($class);
if ($parent && isset(self::$final[$parent])) {
@trigger_error(sprintf('Extending the class %s in %s is deprecated. This class will be made final in the next major release.', $parent, $name), E_USER_DEPRECATED);
Copy link
Member
@nicolas-grekas nicolas-grekas Jan 13, 2017

Choose a reason for hiding this comment

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

As discussed on Slack with @javiereguiluz, we propose: 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, $msgWithFirstUppercaseAndNewlinesAndSpacesAndFinalDotCleanedUp, $class)

Copy link
Contributor

Choose a reason for hiding this comment

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

msgWithFirstUppercaseAndNewlinesAndSpacesAndFinalDotCleanedUp 🤷‍♂️🙈

Ps. It's Slack ;) nog Sack 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

s/nog/not/ (Muphry's law)

Copy link
Member
F438

Choose a reason for hiding this comment

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

$msgWithFirstUppercaseAndNewlinesAndSpacesAndFinalDotCleanedUp: this means almost same process as done for deprecations should be applied on the message next to the annotation :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍, updated

Copy link
Member
@nicolas-grekas 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.

👍 with a minor comment (would be great to answer to @xabbuh thought)

}

$parent = get_parent_class($class);
if ($parent && array_key_exists($parent, self::$final)) {
Copy link
Member

Choose a reason for hiding this comment

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

"isset" is enough here

@xabbuh
Copy link
Member
xabbuh commented Jan 14, 2017

👍

fabpot added a commit that referenced this pull request Jan 15, 2017
This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Mark generated containers as final

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | no
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

So that we don't have to care anymore about BC for protected methods in generated containers.
Will leverage deprecations triggered in #20493

Commits
-------

ce0ee1e [DI] Mark generated containers as final
@fabpot
Copy link
Member
fabpot commented Jan 15, 2017

@GuilhemN Can you squash your commits please?


$parent = get_parent_class($class);
if ($parent && isset(self::$final[$parent])) {
@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);
Copy link
Member

Choose a reason for hiding this comment

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

So.
If the class is @final, it means that one may not extend it, but still could use it.
If the class is @internal, it means that he should not use it at all.

For me it's more like that case:

-'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.'
+'The %s class is considered final%s. It will be declared with `final` keyword in next major release. You should not extend it from %s.'

If one would like to ask user to not use that class (as he cannot rely on it's interface), I would rather mark that classes as @internal ones.

Copy link
Member

Choose a reason for hiding this comment

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

this is missing the fact that sometimes, a class cannot be made really final for technical reasons (see eg Doctrine EM) - because "final" forbids lazy proxies.
If one wants to express what you say, then the message after the annotation is done for that.

Copy link
Member

Choose a reason for hiding this comment

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

Doctrine - valid scenario. good here.

Still, final doesn't mean that Backwards Compatibility Promise is not provided. It's not provided for internal classes.
Consider some exception is final. One should not extend it, but he could still catch it and handle it.

Copy link
Member

Choose a reason for hiding this comment

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

True! @final says that extending is not covered by the BC policy.
Might be worth a PR on the BC policy. Would you mind doing it?

Copy link
Member
@keradus keradus Jan 15, 2017

Choose a reason for hiding this comment

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

@nicolas-grekas
Policy is one thing, but then the message here is different.
I would rather want to first set up what is expected, then do it. Not the other way around.

I would propose for @final:

-'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.'
+-'The %s class is considered final%s. You should not extend it from %s.'

Then, for @internal annotation:

-
+'The %s class is considered internal%s. It may change without further notice as of its next major version. You should not use it'

What do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

For final, I think the message is OK as is now.
For internal, I think we should not trigger anything: the user can do it. It's just that you're on your own if doing so. It's like using reflection to access private props: you don't get a notice. But you're on you own and can't complain if this breaks.
So, nothing to change IMHO in the code.

Copy link
Member

Choose a reason for hiding this comment

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

the thing with reflection is that the user will be aware that he is doing sth odd by accessing private props - as he needs to do sth special.

for internal class, with is only marked via annotation, in most cases he would not know

Copy link
Member
@nicolas-grekas nicolas-grekas Jan 15, 2017

Choose a reason for hiding this comment

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

that's why we do not remove them in the same major where we added the tag
but if a new class is created and starts its life @internal, there is no way the user is not aware: the class is not documented, thus one must have opened the code to know what it does.
add IDE for many people + UPGRADE when @internal is added after a release.
I think that's good enough :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@keradus also unlike for @final, most IDE do give you a visual warning when using a @internal class. True that we should not rely on IDEs only as some people are not using them, but as one is very unlikely to use an internal class accidentally, I think it's an acceptable compromise.

Copy link
Member

Choose a reason for hiding this comment

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

I just met a lot of bad code in my life, some of them just because sb was not aware of sth.
The more defensive you are, the less people will make a mistake ;)

very well, let us stop on current state as good enough

/**
* @final since version 3.3.
*/
class FinalClass
Copy link
Member

Choose a reason for hiding this comment

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

please add a test like

/** @final since version 3.3 */
final class FinalClass {}

(or better, add test checking that no final declared class has final annotation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why ? If someone want redundancy, I don't think we should care.

Copy link
Member

Choose a reason for hiding this comment

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

it's not needed to trigger warning to the user that the class will be final if it already is.

Copy link
Member

Choose a reason for hiding this comment

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

but we don't care: if the class is really final, then there is no way we end up in the code path where that final class is the parent of something else, isn't it?

@GuilhemN
Copy link
Contributor Author

done @fabpot

@fabpot
Copy link
Member
fabpot commented Jan 15, 2017

Thank you @GuilhemN.

@fabpot fabpot merged commit c2ff111 into symfony:master Jan 15, 2017
fabpot added a commit that referenced this pull request Jan 15, 2017
…DebugClassLoader - prepare making some classes final (GuilhemN)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Debug] Trigger deprecation on `@final` annotation in DebugClassLoader - prepare making some classes final

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | follows #19734
| License       | MIT
| Doc PR        |  |

BC promises become quickly huge but making classes `final` can limit these promises.
At the same time, many classes of the symfony codebase are not meant to be extended and could be  `final`; that's the goal of this PR: prepare making them final in 4.0 by triggering deprecations in their constructor:
```php
public function __construct()
{
    if (__CLASS__ !== get_class($this)) {
        @trigger_error(sprintf('Extending %s is deprecated since 3.3 and won\'t be supported in 4.0 as it will be final.', __CLASS__), E_USER_DEPRECATED);
    }
}
```

I updated two classes for now but we can do much more if you like it.

Commits
-------

c2ff111 [Debug] Trigger deprecation on `@final` annotation in DebugClassLoader
@GuilhemN GuilhemN deleted the EXTENDING branch January 15, 2017 16:48
@fabpot fabpot mentioned this pull request May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0