-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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())); | ||
|
@@ -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]) : ''; | ||
} | ||
|
||
$parent = get_parent_class($class); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doctrine - valid scenario. good here. Still, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nicolas-grekas I would propose for -'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 -
+'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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For final, I think the message is OK as is now. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @keradus also unlike for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this
I agree 👍, I moved it. |
||
// Don't trigger deprecations for classes in the same vendor | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment does not really match the tested behaviour, does it? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 // 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 ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = ''; | ||
|
@@ -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)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing this looks wrong to me There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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) { | ||
|
@@ -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 {}'); | ||
} | ||
} | ||
} |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why ? If someone want redundancy, I don't think we should care. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
{ | ||
} |
There was a problem hiding this comment.
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 anif
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10000ok, done