From 3e51fb4ef12d4bac1da3221d0c1017c8126d019d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Wed, 15 Oct 2025 12:07:00 +0200 Subject: [PATCH] Support closure in PHPDriver Add test on unbound scope of the included file Deprecate updating $metadata variable in the php file directly Exclude _files directory from PHPStan inspection --- UPGRADE.md | 25 ++++++++ composer.json | 1 + docs/en/reference/index.rst | 11 +++- phpstan.neon | 2 +- src/Persistence/Mapping/Driver/PHPDriver.php | 31 ++++++++++ tests/Persistence/Mapping/PHPDriverTest.php | 62 ++++++++++++++++++- ...ersistence.Mapping.PHPTestEntityAssert.php | 8 +++ ...rsistence.Mapping.PHPTestEntityClosure.php | 9 +++ ...ence.Mapping.PHPTestIncorrectUseStatic.php | 9 +++ ...stence.Mapping.PHPTestIncorrectUseThis.php | 7 +++ 10 files changed, 160 insertions(+), 5 deletions(-) create mode 100644 tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestEntityAssert.php create mode 100644 tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestEntityClosure.php create mode 100644 tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestIncorrectUseStatic.php create mode 100644 tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestIncorrectUseThis.php diff --git a/UPGRADE.md b/UPGRADE.md index d7a9a869..ca89d1bc 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -16,6 +16,31 @@ The interface `Doctrine\Persistence\Mapping\ClassMetadata` has two new methods: Not implementing these methods is deprecated. They will be required in 5.0. +## Deprecated modifying `$metadata` in PHP mapping files + +Relying on the `$metadata` variable directly in PHP mapping files is deprecated. +Instead, wrap the code in a closure that is returned by the configuration file. + +Before: + +```php +name = \App\Entity\User::class; +``` + +After: + +```php +name = \App\Entity\User::class; +}; +``` + # Upgrade to 4.0 ## BC Break: Removed `StaticReflectionService` diff --git a/composer.json b/composer.json index 6e5a5ccf..e407cc6b 100644 --- a/composer.json +++ b/composer.json @@ -21,6 +21,7 @@ ], "require": { "php": "^8.1", + "doctrine/deprecations": "^1", "doctrine/event-manager": "^1 || ^2", "psr/cache": "^1.0 || ^2.0 || ^3.0" }, diff --git a/docs/en/reference/index.rst b/docs/en/reference/index.rst index df529bf5..e17eca8f 100644 --- a/docs/en/reference/index.rst +++ b/docs/en/reference/index.rst @@ -258,10 +258,15 @@ mapping metadata. .. code-block:: php use App\Model\User; + use Doctrine\Persistence\Mapping\ClassMetadata; + + return function (ClassMetadata $metadata): void { + $metadata->name = User::class; + + // ... - $metadata->name = User::class; + }; - // ... StaticPHPDriver -------------- @@ -284,6 +289,8 @@ Your class in ``App\Model\User`` would look like the following. namespace App\Model; + use Doctrine\Persistence\Mapping\ClassMetadata; + final class User { // ... diff --git a/phpstan.neon b/phpstan.neon index 2d2827e9..dbf7d337 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -14,7 +14,7 @@ parameters: - tests/Persistence/Mapping/_files/colocated/Foo.mphp excludePaths: - - tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestEntity.php + - tests/Persistence/Mapping/_files/Doctrine.*.php ignoreErrors: - '#Variable property access on \$this\(Doctrine\\Persistence\\Reflection\\TypedNoDefaultReflectionProperty\)\.#' diff --git a/src/Persistence/Mapping/Driver/PHPDriver.php b/src/Persistence/Mapping/Driver/PHPDriver.php index 10061b82..318c7fa6 100644 --- a/src/Persistence/Mapping/Driver/PHPDriver.php +++ b/src/Persistence/Mapping/Driver/PHPDriver.php @@ -4,7 +4,11 @@ namespace Doctrine\Persistence\Mapping\Driver; +use Closure; +use CompileError; +use Doctrine\Deprecations\Deprecation; use Doctrine\Persistence\Mapping\ClassMetadata; +use Error; /** * The PHPDriver includes php files which just populate ClassMetadataInfo @@ -35,6 +39,33 @@ public function loadMetadataForClass(string $className, ClassMetadata $metadata) */ protected function loadMappingFile(string $file): array { + try { + $callback = Closure::bind(static function (string $file): mixed { + $metadata = null; + + return include $file; + }, null, null)($file); + } catch (CompileError $e) { + throw $e; + } catch (Error) { + // Calling any method on $metadata=null will raise an Error + // Falling back to legacy behavior of expecting $metadata to be populated + $callback = null; + } + + if ($callback instanceof Closure) { + $callback($this->metadata); + + return [$this->metadata->getName() => $this->metadata]; + } + + Deprecation::trigger( + 'doctrine/persistence', + 'https://github.com/doctrine/persistence/pull/450', + 'Not returning a Closure from a PHP mapping file is deprecated', + ); + + unset($callback); $metadata = $this->metadata; include $file; diff --git a/tests/Persistence/Mapping/PHPDriverTest.php b/tests/Persistence/Mapping/PHPDriverTest.php index 069421a5..429a2f1b 100644 --- a/tests/Persistence/Mapping/PHPDriverTest.php +++ b/tests/Persistence/Mapping/PHPDriverTest.php @@ -4,22 +4,80 @@ namespace Doctrine\Tests\Persistence\Mapping; +use Doctrine\Deprecations\PHPUnit\VerifyDeprecations; use Doctrine\Persistence\Mapping\ClassMetadata; use Doctrine\Persistence\Mapping\Driver\PHPDriver; use Doctrine\Tests\DoctrineTestCase; +use Error; +use PHPUnit\Framework\Attributes\IgnoreDeprecations; +use PHPUnit\Framework\Attributes\TestWith; class PHPDriverTest extends DoctrineTestCase { - public function testLoadMetadata(): void + use VerifyDeprecations; + + /** @phpstan-param class-string $className */ + #[IgnoreDeprecations] + #[TestWith([PHPTestEntity::class])] + #[TestWith([PHPTestEntityAssert::class])] + public function testLoadMetadata(string $className): void + { + $metadata = $this->createMock(ClassMetadata::class); + $metadata->expects(self::once())->method('getFieldNames'); + $driver = new PHPDriver([__DIR__ . '/_files']); + + $this->expectDeprecationWithIdentifier('https://github.com/doctrine/persistence/pull/450'); + $driver->loadMetadataForClass($className, $metadata); + } + + public function testLoadMetadataWithClosure(): void { $metadata = $this->createMock(ClassMetadata::class); $metadata->expects(self::once())->method('getFieldNames'); $driver = new PHPDriver([__DIR__ . '/_files']); - $driver->loadMetadataForClass(PHPTestEntity::class, $metadata); + $driver->loadMetadataForClass(PHPTestEntityClosure::class, $metadata); + } + + public function testLoadMetadataClosureNotBoundToObject(): void + { + $metadata = $this->createMock(ClassMetadata::class); + $driver = new PHPDriver([__DIR__ . '/_files']); + + $this->expectException(Error::class); + $this->expectExceptionMessage('Using $this when not in object context'); + + $driver->loadMetadataForClass(PHPTestIncorrectUseThis::class, $metadata); + } + + public function testLoadMetadataClosureNotBoundToClass(): void + { + $metadata = $this->createMock(ClassMetadata::class); + $driver = new PHPDriver([__DIR__ . '/_files']); + + $this->expectException(Error::class); + $this->expectExceptionMessage('Cannot use "static" in the global scope'); + + $driver->loadMetadataForClass(PHPTestIncorrectUseStatic::class, $metadata); } } class PHPTestEntity { } + +class PHPTestEntityAssert +{ +} + +class PHPTestEntityClosure +{ +} + +class PHPTestIncorrectUseThis +{ +} + +class PHPTestIncorrectUseStatic +{ +} diff --git a/tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestEntityAssert.php b/tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestEntityAssert.php new file mode 100644 index 00000000..7ec5a964 --- /dev/null +++ b/tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestEntityAssert.php @@ -0,0 +1,8 @@ +getFieldNames(); diff --git a/tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestEntityClosure.php b/tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestEntityClosure.php new file mode 100644 index 00000000..2cefa33a --- /dev/null +++ b/tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestEntityClosure.php @@ -0,0 +1,9 @@ +getFieldNames(); +}; diff --git a/tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestIncorrectUseStatic.php b/tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestIncorrectUseStatic.php new file mode 100644 index 00000000..dd38b03a --- /dev/null +++ b/tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestIncorrectUseStatic.php @@ -0,0 +1,9 @@ +isIdentifier(static::class); +}; diff --git a/tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestIncorrectUseThis.php b/tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestIncorrectUseThis.php new file mode 100644 index 00000000..3373a96d --- /dev/null +++ b/tests/Persistence/Mapping/_files/Doctrine.Tests.Persistence.Mapping.PHPTestIncorrectUseThis.php @@ -0,0 +1,7 @@ +setGlobalBasename('global-mapping'); +};