From 751eea72a7adec0495b517616373d9df81f2b95f Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Tue, 8 Feb 2022 19:16:54 +0100 Subject: [PATCH 1/2] [ErrorHandler] trigger deprecations for `@final` properties --- UPGRADE-6.1.md | 8 ++++- .../Tests/Adapter/MaxIdLengthAdapterTest.php | 4 +-- src/Symfony/Component/Console/CHANGELOG.md | 2 +- .../Console/Tests/Command/CommandTest.php | 14 ++++++++ .../Tests/ContainerTest.php | 19 +++++----- .../Tests/Loader/FileLoaderTest.php | 7 ++-- .../Component/ErrorHandler/CHANGELOG.md | 2 +- .../ErrorHandler/DebugClassLoader.php | 35 ++++++++++++------- .../Tests/DebugClassLoaderTest.php | 19 +++++++++- .../Fixtures/FinalProperty/FinalProperty.php | 21 +++++++++++ .../Tests/Fixtures/OverrideFinalProperty.php | 12 +++++++ .../Component/Form/Tests/VersionAwareTest.php | 2 +- .../Component/Ldap/Adapter/ExtLdap/Query.php | 3 -- .../Test/ConstraintValidatorTestCase.php | 4 ++- .../Tests/Constraints/ImageValidatorTest.php | 9 ++--- .../Tests/Fixtures/Attribute/Entity.php | 2 +- .../Tests/Fixtures/Attribute/EntityParent.php | 2 +- .../Tests/Fixtures/NestedAttribute/Entity.php | 2 +- 18 files changed, 121 insertions(+), 46 deletions(-) create mode 100644 src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/FinalProperty.php create mode 100644 src/Symfony/Component/ErrorHandler/Tests/Fixtures/OverrideFinalProperty.php diff --git a/UPGRADE-6.1.md b/UPGRADE-6.1.md index 70d78fd7cc97e..7d24e202f8410 100644 --- a/UPGRADE-6.1.md +++ b/UPGRADE-6.1.md @@ -1,10 +1,16 @@ UPGRADE FROM 6.0 to 6.1 ======================= +All components +-------------- + + * Public and protected properties are now considered final; + instead of overriding a property, consider setting its value in the constructor + Console ------- - * Deprecate `Command::$defaultName` and `Command::$defaultDescription`, use the `AsCommand` attribute instead. + * Deprecate `Command::$defaultName` and `Command::$defaultDescription`, use the `AsCommand` attribute instead Serializer ---------- diff --git a/src/Symfony/Component/Cache/Tests/Adapter/MaxIdLengthAdapterTest.php b/src/Symfony/Component/Cache/Tests/Adapter/MaxIdLengthAdapterTest.php index 6097ec2ef7f71..00a288e01b45b 100644 --- a/src/Symfony/Component/Cache/Tests/Adapter/MaxIdLengthAdapterTest.php +++ b/src/Symfony/Component/Cache/Tests/Adapter/MaxIdLengthAdapterTest.php @@ -79,10 +79,10 @@ public function testTooLongNamespace() abstract class MaxIdLengthAdapter extends AbstractAdapter { - protected $maxIdLength = 50; - public function __construct(string $ns) { + $this->maxIdLength = 50; + parent::__construct($ns); } } diff --git a/src/Symfony/Component/Console/CHANGELOG.md b/src/Symfony/Component/Console/CHANGELOG.md index 8a34f80b0b6af..2573354f04c2f 100644 --- a/src/Symfony/Component/Console/CHANGELOG.md +++ b/src/Symfony/Component/Console/CHANGELOG.md @@ -5,7 +5,7 @@ CHANGELOG --- * Add method `__toString()` to `InputInterface` - * Deprecate `Command::$defaultName` and `Command::$defaultDescription`, use the `AsCommand` attribute instead. + * Deprecate `Command::$defaultName` and `Command::$defaultDescription`, use the `AsCommand` attribute instead 6.0 --- diff --git a/src/Symfony/Component/Console/Tests/Command/CommandTest.php b/src/Symfony/Component/Console/Tests/Command/CommandTest.php index d14c55df26628..044cb2037f35d 100644 --- a/src/Symfony/Component/Console/Tests/Command/CommandTest.php +++ b/src/Symfony/Component/Console/Tests/Command/CommandTest.php @@ -503,13 +503,27 @@ class Php8Command2 extends Command class MyCommand extends Command { + /** + * @deprecated since Symfony 6.1 + */ protected static $defaultName = 'my:command'; + + /** + * @deprecated since Symfony 6.1 + */ protected static $defaultDescription = 'This is a command I wrote all by myself'; } #[AsCommand(name: 'my:command', description: 'This is a command I wrote all by myself')] class MyAnnotatedCommand extends Command { + /** + * @deprecated since Symfony 6.1 + */ protected static $defaultName = 'i-shall-be-ignored'; + + /** + * @deprecated since Symfony 6.1 + */ protected static $defaultDescription = 'This description should be ignored.'; } diff --git a/src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php b/src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php index 497dc99385834..bfa5b02fab666 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php @@ -413,16 +413,6 @@ class ProjectServiceContainer extends Container public $__foo_bar; public $__foo_baz; public $__internal; - protected $privates; - protected $methodMap = [ - 'bar' => 'getBarService', - 'foo_bar' => 'getFooBarService', - 'foo.baz' => 'getFoo_BazService', - 'circular' => 'getCircularService', - 'throw_exception' => 'getThrowExceptionService', - 'throws_exception_on_service_configuration' => 'getThrowsExceptionOnServiceConfigurationService', - 'internal_dependency' => 'getInternalDependencyService', - ]; public function __construct() { @@ -434,6 +424,15 @@ public function __construct() $this->__internal = new \stdClass(); $this->privates = []; $this->aliases = ['alias' => 'bar']; + $this->methodMap = [ + 'bar' => 'getBarService', + 'foo_bar' => 'getFooBarService', + 'foo.baz' => 'getFoo_BazService', + 'circular' => 'getCircularService', + 'throw_exception' => 'getThrowExceptionService', + 'throws_exception_on_service_configuration' => 'getThrowsExceptionOnServiceConfigurationService', + 'internal_dependency' => 'getInternalDependencyService', + ]; } protected function getInternalService() diff --git a/src/Symfony/Component/DependencyInjection/Tests/Loader/FileLoaderTest.php b/src/Symfony/Component/DependencyInjection/Tests/Loader/FileLoaderTest.php index 5b3b7e37147e2..9a4aacf30cf7a 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Loader/FileLoaderTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Loader/FileLoaderTest.php @@ -89,7 +89,7 @@ public function testRegisterClasses() $container = new ContainerBuilder(); $container->setParameter('sub_dir', 'Sub'); $loader = new TestFileLoader($container, new FileLocator(self::$fixturesPath.'/Fixtures')); - $loader->autoRegisterAliasesForSinglyImplementedInterfaces = false; + $loader->noAutoRegisterAliasesForSinglyImplementedInterfaces(); $loader->registerClasses(new Definition(), 'Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Sub\\', 'Prototype/%sub_dir%/*'); $loader->registerClasses(new Definition(), 'Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Sub\\', 'Prototype/%sub_dir%/*'); // loading twice should not be an issue @@ -270,7 +270,10 @@ public function testRegisterClassesWithWhenEnv(?string $env, bool $expected) class TestFileLoader extends FileLoader { - public $autoRegisterAliasesForSinglyImplementedInterfaces = true; + public function noAutoRegisterAliasesForSinglyImplementedInterfaces() + { + $this->autoRegisterAliasesForSinglyImplementedInterfaces = false; + } public function load(mixed $resource, string $type = null): mixed { diff --git a/src/Symfony/Component/ErrorHandler/CHANGELOG.md b/src/Symfony/Component/ErrorHandler/CHANGELOG.md index f6b689e13936d..ff0b18c97de23 100644 --- a/src/Symfony/Component/ErrorHandler/CHANGELOG.md +++ b/src/Symfony/Component/ErrorHandler/CHANGELOG.md @@ -4,7 +4,7 @@ CHANGELOG 6.1 --- - * Report overridden `@final` constants + * Report overridden `@final` constants and properties 5.4 --- diff --git a/src/Symfony/Component/ErrorHandler/DebugClassLoader.php b/src/Symfony/Component/ErrorHandler/DebugClassLoader.php index 863eaf1e9be43..21c32eeaed8a9 100644 --- a/src/Symfony/Component/ErrorHandler/DebugClassLoader.php +++ b/src/Symfony/Component/ErrorHandler/DebugClassLoader.php @@ -112,6 +112,7 @@ class DebugClassLoader private static array $checkedClasses = []; private static array $final = []; private static array $finalMethods = []; + private static array $finalProperties = []; private static array $finalConstants = []; private static array $deprecated = []; private static array $internal = []; @@ -469,9 +470,10 @@ public function checkAnnotations(\ReflectionClass $refl, string $class): array self::$finalMethods[$class] = []; self::$internalMethods[$class] = []; self::$annotatedParameters[$class] = []; + self::$finalProperties[$class] = []; self::$finalConstants[$class] = []; foreach ($parentAndOwnInterfaces as $use) { - foreach (['finalMethods', 'internalMethods', 'annotatedParameters', 'returnTypes', 'finalConstants'] as $property) { + foreach (['finalMethods', 'internalMethods', 'annotatedParameters', 'returnTypes', 'finalProperties', 'finalConstants'] as $property) { if (isset(self::${$property}[$use])) { self::${$property}[$class] = self::${$property}[$class] ? self::${$property}[$use] + self::${$property}[$class] : self::${$property}[$use]; } @@ -626,22 +628,29 @@ public function checkAnnotations(\ReflectionClass $refl, string $class): array } } - foreach ($refl->getReflectionConstants(\ReflectionClassConstant::IS_PUBLIC | \ReflectionClassConstant::IS_PROTECTED) as $constant) { - if ($constant->class !== $class) { - continue; - } + $finals = isset(self::$final[$class]) || $refl->isFinal() ? [] : [ + 'finalConstants' => $refl->getReflectionConstants(\ReflectionClassConstant::IS_PUBLIC | \ReflectionClassConstant::IS_PROTECTED), + 'finalProperties' => $refl->getProperties(\ReflectionProperty::IS_PUBLIC | \ReflectionProperty::IS_PROTECTED), + ]; + foreach ($finals as $type => $reflectors) { + foreach ($reflectors as $r) { + if ($r->class !== $class) { + continue; + } + + $doc = $this->parsePhpDoc($r); - foreach ($parentAndOwnInterfaces as $use) { - if (isset(self::$finalConstants[$use][$constant->name])) { - $deprecations[] = sprintf('The "%s::%s" constant is considered final. You should not override it in "%s".', self::$finalConstants[$use][$constant->name], $constant->name, $class); + foreach ($parentAndOwnInterfaces as $use) { + if (isset(self::${$type}[$use][$r->name]) && !isset($doc['deprecated']) && ('finalConstants' === $type || substr($use, 0, strrpos($use, '\\')) !== substr($use, 0, strrpos($class, '\\')))) { + $msg = 'finalConstants' === $type ? '%s" constant' : '$%s" property'; + $deprecations[] = sprintf('The "%s::'.$msg.' is considered final. You should not override it in "%s".', self::${$type}[$use][$r->name], $r->name, $class); + } } - } - if (!($doc = $this->parsePhpDoc($constant)) || !isset($doc['final'])) { - continue; + if (isset($doc['final']) || ('finalProperties' === $type && str_starts_with($class, 'Symfony\\') && !$r->hasType())) { + self::${$type}[$class][$r->name] = $class; + } } - - self::$finalConstants[$class][$constant->name] = $class; } return $deprecations; diff --git a/src/Symfony/Component/ErrorHandler/Tests/DebugClassLoaderTest.php b/src/Symfony/Component/ErrorHandler/Tests/DebugClassLoaderTest.php index 98e7dbd90f578..461062a1c01e2 100644 --- a/src/Symfony/Component/ErrorHandler/Tests/DebugClassLoaderTest.php +++ b/src/Symfony/Component/ErrorHandler/Tests/DebugClassLoaderTest.php @@ -401,13 +401,30 @@ class_exists('Test\\'.ReturnType::class, true); ], $deprecations); } + public function testOverrideFinalProperty() + { + $deprecations = []; + set_error_handler(function ($type, $msg) use (&$deprecations) { $deprecations[] = $msg; }); + $e = error_reporting(E_USER_DEPRECATED); + + class_exists(Fixtures\OverrideFinalProperty::class, true); + + error_reporting($e); + restore_error_handler(); + + $this->assertSame([ + 'The "Symfony\Component\ErrorHandler\Tests\Fixtures\FinalProperty\FinalProperty::$pub" property is considered final. You should not override it in "Symfony\Component\ErrorHandler\Tests\Fixtures\OverrideFinalProperty".', + 'The "Symfony\Component\ErrorHandler\Tests\Fixtures\FinalProperty\FinalProperty::$prot" property is considered final. You should not override it in "Symfony\Component\ErrorHandler\Tests\Fixtures\OverrideFinalProperty".', + ], $deprecations); + } + public function testOverrideFinalConstant() { $deprecations = []; set_error_handler(function ($type, $msg) use (&$deprecations) { $deprecations[] = $msg; }); $e = error_reporting(E_USER_DEPRECATED); - class_exists( Fixtures\FinalConstant\OverrideFinalConstant::class, true); + class_exists(Fixtures\FinalConstant\OverrideFinalConstant::class, true); error_reporting($e); restore_error_handler(); diff --git a/src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/FinalProperty.php b/src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/FinalProperty.php new file mode 100644 index 0000000000000..ce5b88020a4f8 --- /dev/null +++ b/src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/FinalProperty.php @@ -0,0 +1,21 @@ + + * + * @template T of ConstraintValidatorInterface */ abstract class ConstraintValidatorTestCase extends TestCase { @@ -48,7 +50,7 @@ abstract class ConstraintValidatorTestCase extends TestCase protected $context; /** - * @var ConstraintValidatorInterface + * @var T */ protected $validator; diff --git a/src/Symfony/Component/Validator/Tests/Constraints/ImageValidatorTest.php b/src/Symfony/Component/Validator/Tests/Constraints/ImageValidatorTest.php index 28330aacbaff0..487afd5620b24 100644 --- a/src/Symfony/Component/Validator/Tests/Constraints/ImageValidatorTest.php +++ b/src/Symfony/Component/Validator/Tests/Constraints/ImageValidatorTest.php @@ -18,16 +18,11 @@ /** * @requires extension fileinfo + * + * @extends ConstraintValidatorTestCase */ class ImageValidatorTest extends ConstraintValidatorTestCase { - protected $context; - - /** - * @var ImageValidator - */ - protected $validator; - protected $path; protected $image; protected $imageLandscape; diff --git a/src/Symfony/Component/Validator/Tests/Fixtures/Attribute/Entity.php b/src/Symfony/Component/Validator/Tests/Fixtures/Attribute/Entity.php index f6a0ff2a6355d..3414c6db6e8d4 100644 --- a/src/Symfony/Component/Validator/Tests/Fixtures/Attribute/Entity.php +++ b/src/Symfony/Component/Validator/Tests/Fixtures/Attribute/Entity.php @@ -46,7 +46,7 @@ class Entity extends EntityParent implements EntityInterfaceB Assert\NotNull, Assert\Range(min: 3), ] - public $firstName; + public string $firstName; #[Assert\Valid] public $childA; #[Assert\Valid] diff --git a/src/Symfony/Component/Validator/Tests/Fixtures/Attribute/EntityParent.php b/src/Symfony/Component/Validator/Tests/Fixtures/Attribute/EntityParent.php index e595f82d0439b..6aa7dacb18530 100644 --- a/src/Symfony/Component/Validator/Tests/Fixtures/Attribute/EntityParent.php +++ b/src/Symfony/Component/Validator/Tests/Fixtures/Attribute/EntityParent.php @@ -16,7 +16,7 @@ class EntityParent implements EntityInterfaceA { - protected $firstName; + protected string $firstName; private $internal; private $data = 'Data'; private $child; diff --git a/src/Symfony/Component/Validator/Tests/Fixtures/NestedAttribute/Entity.php b/src/Symfony/Component/Validator/Tests/Fixtures/NestedAttribute/Entity.php index 8555cdb81dc2b..0686e2666f335 100644 --- a/src/Symfony/Component/Validator/Tests/Fixtures/NestedAttribute/Entity.php +++ b/src/Symfony/Component/Validator/Tests/Fixtures/NestedAttribute/Entity.php @@ -67,7 +67,7 @@ class Entity extends EntityParent implements EntityInterfaceB new Assert\Range(min: 5), ]), ] - public $firstName; + public string $firstName; #[Assert\Valid] public $childA; #[Assert\Valid] From eada4c046b59b42249a5a27732ed6477d6520252 Mon Sep 17 00:00:00 2001 From: Thomas Calvet Date: Wed, 9 Feb 2022 13:07:45 +0100 Subject: [PATCH 2/2] Add more tests --- .../Tests/DebugClassLoaderTest.php | 8 +++++++ .../Fixtures/FinalProperty/FinalProperty.php | 11 +++++++++ .../FinalProperty/OutsideFinalProperty.php | 23 +++++++++++++++++++ .../OverrideFinalPropertySameNamespace.php | 8 +++++++ .../Tests/Fixtures/OverrideFinalProperty.php | 6 +++++ .../Fixtures/OverrideOutsideFinalProperty.php | 15 ++++++++++++ 6 files changed, 71 insertions(+) create mode 100644 src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/OutsideFinalProperty.php create mode 100644 src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/OverrideFinalPropertySameNamespace.php create mode 100644 src/Symfony/Component/ErrorHandler/Tests/Fixtures/OverrideOutsideFinalProperty.php diff --git a/src/Symfony/Component/ErrorHandler/Tests/DebugClassLoaderTest.php b/src/Symfony/Component/ErrorHandler/Tests/DebugClassLoaderTest.php index 461062a1c01e2..d1223c529f2cb 100644 --- a/src/Symfony/Component/ErrorHandler/Tests/DebugClassLoaderTest.php +++ b/src/Symfony/Component/ErrorHandler/Tests/DebugClassLoaderTest.php @@ -408,6 +408,8 @@ public function testOverrideFinalProperty() $e = error_reporting(E_USER_DEPRECATED); class_exists(Fixtures\OverrideFinalProperty::class, true); + class_exists(Fixtures\FinalProperty\OverrideFinalPropertySameNamespace::class, true); + class_exists('Test\\'.OverrideOutsideFinalProperty::class, true); error_reporting($e); restore_error_handler(); @@ -415,6 +417,8 @@ class_exists(Fixtures\OverrideFinalProperty::class, true); $this->assertSame([ 'The "Symfony\Component\ErrorHandler\Tests\Fixtures\FinalProperty\FinalProperty::$pub" property is considered final. You should not override it in "Symfony\Component\ErrorHandler\Tests\Fixtures\OverrideFinalProperty".', 'The "Symfony\Component\ErrorHandler\Tests\Fixtures\FinalProperty\FinalProperty::$prot" property is considered final. You should not override it in "Symfony\Component\ErrorHandler\Tests\Fixtures\OverrideFinalProperty".', + 'The "Symfony\Component\ErrorHandler\Tests\Fixtures\FinalProperty\FinalProperty::$implicitlyFinal" property is considered final. You should not override it in "Symfony\Component\ErrorHandler\Tests\Fixtures\OverrideFinalProperty".', + 'The "Test\Symfony\Component\ErrorHandler\Tests\FinalProperty\OutsideFinalProperty::$final" property is considered final. You should not override it in "Test\Symfony\Component\ErrorHandler\Tests\OverrideOutsideFinalProperty".' ], $deprecations); } @@ -540,6 +544,10 @@ public function ownAbstractBaseMethod() { } return $fixtureDir.\DIRECTORY_SEPARATOR.'ReturnType.php'; } elseif ('Test\\'.Fixtures\OutsideInterface::class === $class) { return $fixtureDir.\DIRECTORY_SEPARATOR.'OutsideInterface.php'; + } elseif ('Test\\'.OverrideOutsideFinalProperty::class === $class) { + return $fixtureDir.'OverrideOutsideFinalProperty.php'; + } elseif ('Test\\Symfony\\Component\\ErrorHandler\\Tests\\FinalProperty\\OutsideFinalProperty' === $class) { + return $fixtureDir.'FinalProperty'.\DIRECTORY_SEPARATOR.'OutsideFinalProperty.php'; } } } diff --git a/src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/FinalProperty.php b/src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/FinalProperty.php index ce5b88020a4f8..3d226721fce10 100644 --- a/src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/FinalProperty.php +++ b/src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/FinalProperty.php @@ -18,4 +18,15 @@ class FinalProperty * @final */ private $priv; + + /** + * @final + */ + public $notOverriden; + + protected $implicitlyFinal; + + protected string $typedSoNotFinal; + + protected $deprecated; } diff --git a/src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/OutsideFinalProperty.php b/src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/OutsideFinalProperty.php new file mode 100644 index 0000000000000..462731985dd97 --- /dev/null +++ b/src/Symfony/Component/ErrorHandler/Tests/Fixtures/FinalProperty/OutsideFinalProperty.php @@ -0,0 +1,23 @@ +