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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/Symfony/Component/Debug/DebugClassLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ class DebugClassLoader
private $classLoader;
private $isFinder;
private static $caseCheck;
private static $final = array();
private static $deprecated = array();
private static $php7Reserved = array('int', 'float', 'bool', 'string', 'true', 'false', 'null');
private static $darwinCache = array('/' => array('/', array()));
Expand Down Expand Up @@ -163,11 +164,21 @@ public function loadClass($class)
throw new \RuntimeException(sprintf('Case mismatch between loaded and declared class names: %s vs %s', $class, $name));
}

if (preg_match('#\n \* @final(?:( .+?)\.?)?\r?\n \*(?: @|/$)#s', $refl->getDocComment(), $notice)) {
self::$final[$name] = isset($notice[1]) ? preg_replace('#\s*\r?\n \* +#', ' ', $notice[1]) : '';
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.

10000

ok, done

}

$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

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

}

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

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

if (2 > $len = 1 + (strpos($name, '\\', 1 + strpos($name, '\\')) ?: strpos($name, '_'))) {
$len = 0;
$ns = '';
Expand All @@ -181,7 +192,6 @@ public function loadClass($class)
break;
}
}
$parent = get_parent_class($class);

if (!$parent || strncmp($ns, $parent, $len)) {
if ($parent && isset(self::$deprecated[$parent]) && strncmp($ns, $parent, $len)) {
Expand Down
26 changes: 26 additions & 0 deletions src/Symfony/Component/Debug/Tests/DebugClassLoaderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,28 @@ class_exists('Test\\'.__NAMESPACE__.'\\Float', true);

$this->assertSame($xError, $lastError);
}

public function testExtendedFinalClass()
{
set_error_handler(function () { return false; });
$e = error_reporting(0);
trigger_error('', E_USER_NOTICE);

class_exists('Test\\'.__NAMESPACE__.'\\ExtendsFinalClass', true);

error_reporting($e);
restore_error_handler();

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

$lastError = error_get_last();
unset($lastError['file'], $lastError['line']);

$xError = array(
'type' => E_USER_DEPRECATED,
'message' => 'The Symfony\Component\Debug\Tests\Fixtures\FinalClass class 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 Test\Symfony\Component\Debug\Tests\ExtendsFinalClass.',
);

$this->assertSame($xError, $lastError);
}
}

class ClassLoader
Expand Down Expand Up @@ -300,6 +322,8 @@ public function findFile($class)
return $fixtureDir.'notPsr0Bis.php';
} elseif (__NAMESPACE__.'\Fixtures\DeprecatedInterface' === $class) {
return $fixtureDir.'DeprecatedInterface.php';
} elseif (__NAMESPACE__.'\Fixtures\FinalClass' === $class) {
return $fixtureDir.'FinalClass.php';
} elseif ('Symfony\Bridge\Debug\Tests\Fixtures\ExtendsDeprecatedParent' === $class) {
eval('namespace Symfony\Bridge\Debug\Tests\Fixtures; class ExtendsDeprecatedParent extends \\'.__NAMESPACE__.'\Fixtures\DeprecatedClass {}');
} elseif ('Test\\'.__NAMESPACE__.'\DeprecatedParentClass' === $class) {
Expand All @@ -310,6 +334,8 @@ public function findFile($class)
eval('namespace Test\\'.__NAMESPACE__.'; class NonDeprecatedInterfaceClass implements \\'.__NAMESPACE__.'\Fixtures\NonDeprecatedInterface {}');
} elseif ('Test\\'.__NAMESPACE__.'\Float' === $class) {
eval('namespace Test\\'.__NAMESPACE__.'; class Float {}');
} elseif ('Test\\'.__NAMESPACE__.'\ExtendsFinalClass' === $class) {
eval('namespace Test\\'.__NAMESPACE__.'; class ExtendsFinalClass extends \\'.__NAMESPACE__.'\Fixtures\FinalClass {}');
}
}
}
10 changes: 10 additions & 0 deletions src/Symfony/Component/Debug/Tests/Fixtures/FinalClass.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

namespace Symfony\Component\Debug\Tests\Fixtures;

/**
* @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?

{
}
2 changes: 2 additions & 0 deletions src/Symfony/Component/Serializer/Encoder/ChainDecoder.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
* @author Jordi Boggiano <j.boggiano@seld.be>
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
* @author Lukas Kahwe Smith <smith@pooteeweet.org>
*
* @final since version 3.3.
*/
class ChainDecoder implements DecoderInterface
{
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/Serializer/Encoder/ChainEncoder.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
* @author Jordi Boggiano <j.boggiano@seld.be>
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
* @author Lukas Kahwe Smith <smith@pooteeweet.org>
*
* @final since version 3.3.
*/
class ChainEncoder implements EncoderInterface
{
Expand Down
0