diff --git a/CHANGELOG.md b/CHANGELOG.md index b24bc956..4a9b5c44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,7 @@ See [UPGRADE-4.0.md](UPGRADE-4.0.md) for the migration guide. * [Fixed] The weaving metadata files (`_transformation.cache`, `_include.cache`) carry a format version: a cache of another version is rebuilt, and with `Features::PREBUILT_CACHE` it fails with a hint to run `cache:warmup:aop`. Records keep the size and mtime of the woven source, so a source whose mtime moved backwards (rsync `-t`, checkout of an older revision) is woven again instead of serving the stale proxy (#685). * [Fixed] `cache:warmup:aop` and `debug:advisor` no longer fail with "Cannot rewind a generator that was already run" when more than one include path is configured (#751). * [Fixed] The proxy of a class declared in the second (or a later) block of a namespace that a file declares several times copies the imports of its own block instead of the first block's. +* [Fixed] A file declaring several woven classes gets one proxy file per class, below the file name like a file inside a phar (`Holder.php/App/First.php`): the proxies used to share one cache file, so all classes but the last one were not found (#760). **Internal** * [Internal] Tooling: PHPUnit 13 with strict flags, PHPStan 2 at level 10 over `src/` and `tests/`, php-cs-fixer with the PER-CS rule set, and `composer test`/`analyze`/`cs`/`check` scripts. diff --git a/src/Console/Command/DebugWeavingCommand.php b/src/Console/Command/DebugWeavingCommand.php index 59b4112d..c02d8152 100644 --- a/src/Console/Command/DebugWeavingCommand.php +++ b/src/Console/Command/DebugWeavingCommand.php @@ -165,8 +165,10 @@ private function getProxies(CachePathManager $cachePathManager): array if (str_ends_with($pathname, AspectContainer::ORIGINAL_TRAIT_FILE_SUFFIX)) { continue; } - // Only collect proxy files: they have a sibling `OriginalTrait.php` file - $traitSibling = PathResolver::withSuffixBeforeExtension($pathname, AspectContainer::ORIGINAL_TRAIT_SUFFIX); + // Only collect proxy files: they have a sibling `OriginalTrait.php` file. Proxies of a file with + // several woven classes live below its name (`Holder.php/App/First.php`), next to `HolderOriginalTrait.php` + $proxiedFile = $path . preg_replace('#(?<=\.php)/.*$#', '', substr($pathname, strlen($path))); + $traitSibling = PathResolver::withSuffixBeforeExtension($proxiedFile, AspectContainer::ORIGINAL_TRAIT_SUFFIX); if (!file_exists($traitSibling)) { continue; } diff --git a/src/Instrument/AGENTS.md b/src/Instrument/AGENTS.md index c89bdf35..6b04cf5d 100644 --- a/src/Instrument/AGENTS.md +++ b/src/Instrument/AGENTS.md @@ -30,6 +30,10 @@ trait FooOriginalTrait { /* original methods verbatim */ } include_once AOP_CACHE_DIR . '/Foo.php'; ``` +Proxy path = source path below the cache dir (`/Foo.php`). A file with several woven class-likes gets one proxy per class +below its name, like a file inside a phar: `/Holder.php/Ns/First.php` (#760). Proxies are saved once the whole file is +woven (the layout depends on the woven class count); switching layouts removes the proxies of the other one. + ### Proxy file (loaded by include_once) ```php class Foo extends OriginalParent implements OriginalInterfaces, \Go\Aop\Proxy diff --git a/src/Instrument/Transformer/WeavingTransformer.php b/src/Instrument/Transformer/WeavingTransformer.php index d6221de1..e43878c9 100644 --- a/src/Instrument/Transformer/WeavingTransformer.php +++ b/src/Instrument/Transformer/WeavingTransformer.php @@ -31,6 +31,7 @@ use Go\Proxy\FunctionProxyGenerator; use Go\Proxy\TraitProxyGenerator; use Closure; +use FilesystemIterator; use PhpParser\Node\Attribute; use PhpParser\Node\AttributeGroup; use PhpParser\Node\Name; @@ -38,7 +39,10 @@ use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\EnumCase; use PhpParser\Node\Stmt\Property; +use RecursiveDirectoryIterator; +use RecursiveIteratorIterator; use ReflectionProperty; +use SplFileInfo; use Throwable; /** @@ -92,6 +96,8 @@ public function transform(StreamMetaData $metadata): TransformerResult $advisors = $this->container->getServicesByInterface(Advisor::class); $namespaces = $parsedSource->getFileNamespaces(); + // Proxies are saved once the whole file is woven: their file names depend on the number of woven classes + $wovenProxies = []; foreach ($namespaces as $namespace) { $classes = $namespace->getClasses(); @@ -105,33 +111,42 @@ public function transform(StreamMetaData $metadata): TransformerResult if ($class->isInterface()) { continue; } - $wasClassProcessed = $this->processSingleClass( + $wovenProxy = $this->processSingleClass( $advisors, $metadata, $class, $namespace, $parsedSource->isStrictMode(), ); - $totalTransformations += (int) $wasClassProcessed; + if ($wovenProxy !== null) { + $wovenProxies[] = $wovenProxy; + } } $wasFunctionsProcessed = $this->processFunctions($advisors, $metadata, $namespace); $totalTransformations += (int) $wasFunctionsProcessed; } + foreach ($wovenProxies as [$class, $childCode, $lastClassToken]) { + $contentToInclude = $this->saveProxyToCache($class, $childCode, count($wovenProxies) === 1); + $metadata->tokenStream[$lastClassToken]->text .= PHP_EOL . $contentToInclude; + } + $totalTransformations += count($wovenProxies); + $result = ($totalTransformations > 0) ? TransformerResult::Transformed : TransformerResult::Abstain; return $result; } /** - * Performs weaving of single class if needed, returns true if the class was processed + * Performs weaving of single class if needed, returns its proxy code and the position of its last token * * @param Advisor[] $advisors List of advisors * @param StreamMetaData $metadata * @param ReflectionClass $class * @param ReflectionFileNamespace $namespace Namespace block of the file that declares the class * @param bool $useStrictMode If the source file used strict mode, the proxy should too - * @return bool + * + * @return array{ReflectionClass, string, int}|null Null if the class was not woven */ private function processSingleClass( array $advisors, @@ -139,13 +154,13 @@ private function processSingleClass( ReflectionClass $class, ReflectionFileNamespace $namespace, bool $useStrictMode, - ): bool { + ): ?array { try { $advices = $this->adviceMatcher->getAdvicesForClass($class, $advisors); } catch (Throwable $matchingError) { // Aspects used to be skipped before matching, so matching one must not fail the file if ($this->isAspectSafe($class)) { - return false; + return null; } throw $matchingError; } @@ -153,12 +168,12 @@ private function processSingleClass( if (empty($advices)) { // Fast return if there aren't any advices for that class. It comes before the aspect check, // as that one reflects (and parses) every ancestor and fails when one can't be located - return false; + return null; } // Aspects are never woven if ($class->implementsInterface(Aspect::class)) { - return false; + return null; } // Sort advices in advance to keep the correct order in cache, and leave only keys for the cache @@ -197,18 +212,10 @@ private function processSingleClass( $childCode = 'declare(strict_types=1);' . PHP_EOL . $childCode; } - $contentToInclude = $this->saveProxyToCache($class, $childCode); + // The include of the proxy goes after the last token of the class + [, $lastClassToken] = $this->getDeclarationTokenRange($class->getNode()); - // Get last token for this class - $classNode = $class->getNode(); - $lastClassToken = $classNode->getAttribute('endTokenPos'); - if (!is_int($lastClassToken)) { - return false; - } - - $metadata->tokenStream[$lastClassToken]->text .= PHP_EOL . $contentToInclude; - - return true; + return [$class, $childCode, $lastClassToken]; } /** @@ -969,9 +976,12 @@ private function processFunctions( } /** - * Save AOP proxy to the separate file anr returns the php source code for inclusion + * Save AOP proxy to the separate file and returns the php source code for inclusion + * + * The proxy of the only woven class of a source file is named like that file. A file with several woven classes + * gets one proxy per class below its name, used as a directory like a file inside a phar: `src/Holder.php/App/First.php`. */ - private function saveProxyToCache(ReflectionClass $class, string $childCode): string + private function saveProxyToCache(ReflectionClass $class, string $childCode, bool $isOnlyClass): string { $cacheRootDir = $this->cachePathManager->getCacheDir(); if ($cacheRootDir === null) { @@ -984,7 +994,16 @@ private function saveProxyToCache(ReflectionClass $class, string $childCode): st // Classes outside the application root keep their absolute path below the cache root $relativePath = ltrim(PathResolver::rebase($classFileName, $this->options['appDir'], '') ?? $classFileName, '/\\'); $proxyRelativePath = str_replace('\\', '/', $relativePath); - $proxyFileName = $cacheRootDir . '/' . $proxyRelativePath; + $sourceProxyPath = $cacheRootDir . '/' . $proxyRelativePath; + if (!$isOnlyClass) { + $proxyRelativePath .= '/' . str_replace('\\', '/', $class->getName()) . '.php'; + } + $proxyFileName = $cacheRootDir . '/' . $proxyRelativePath; + + // The file switched between one and several woven classes since it was woven: drop the proxies of the other layout + if ($isOnlyClass ? is_dir($sourceProxyPath) : is_file($sourceProxyPath)) { + self::removeCachePath($sourceProxyPath); + } // Atomic write: a concurrent request including the proxy never sees a partial file $this->cachePathManager->getCacheFileWriter()->write($proxyFileName, 'isDir()) { + @rmdir($fileInfo->getPathname()); + } else { + @unlink($fileInfo->getPathname()); + } + } + @rmdir($path); + } + /** * Utility method to load and register unloaded aspects * diff --git a/tests/Console/Command/DebugWeavingCommandInProcessTest.php b/tests/Console/Command/DebugWeavingCommandInProcessTest.php index 84ab361f..b06b7ff7 100644 --- a/tests/Console/Command/DebugWeavingCommandInProcessTest.php +++ b/tests/Console/Command/DebugWeavingCommandInProcessTest.php @@ -142,6 +142,26 @@ public function testVerboseOutputShowsTheDifferenceAndTheStableProxies(): void $this->assertStringContainsString('Bar.php" is consistently', $display); } + public function testProxiesOfFileWithSeveralWovenClassesAreCompared(): void + { + $cacheDir = $this->createEmptyCacheDir(); + $calls = 0; + // Per-class proxies live below the name of their source file, the woven source sits next to that name + $warmUp = static function () use (&$calls, $cacheDir): void { + ++$calls; + @mkdir($cacheDir . '/Holder.php/App', 0777, true); + file_put_contents($cacheDir . '/Holder.php/App/First.php', "createCommandWithFakedKernel($cacheDir, $warmUp)); + + $exitCode = $tester->execute(['loader' => 'unused.php']); + + $display = (string) preg_replace('/\s+/', ' ', $tester->getDisplay()); + $this->assertSame(Command::FAILURE, $exitCode); + $this->assertStringContainsString('Holder.php/App/First.php" is weaved differently', $display); + } + /** * Builds the command against a faked kernel: the AspectKernel is a per-process * singleton, so the kernel/container/warmer collaborators are replaced with diff --git a/tests/Instrument/Transformer/WeavingTransformerTest.php b/tests/Instrument/Transformer/WeavingTransformerTest.php index 0faa8b24..579ed361 100644 --- a/tests/Instrument/Transformer/WeavingTransformerTest.php +++ b/tests/Instrument/Transformer/WeavingTransformerTest.php @@ -883,10 +883,11 @@ public function testWeaverStripsAttributeClassMarkersFromWovenTrait(): void // The compatible part of the grouped attribute must survive $this->assertStringContainsString('#[\FakeMarkerAttr]', $actual); - // The proxy (last class in the file wins the shared cache path) must keep #[\Attribute(...)] + // Every woven class of the file has its own proxy, the one of the attribute class keeps #[\Attribute(...)] $matches = []; - $this->assertSame(1, preg_match("/AOP_CACHE_DIR . '(.+)';$/m", $actual, $matches)); - $proxyContent = (string) file_get_contents('vfs://' . $matches[1]); + $this->assertSame(3, preg_match_all("/AOP_CACHE_DIR \. '([^']+)';/", $actual, $matches)); + $this->assertStringEndsWith('php80-attribute-class.php/Test/ns1/TestCustomAttribute.php', $matches[1][2]); + $proxyContent = (string) file_get_contents('vfs://' . $matches[1][2]); $this->assertStringContainsString( '#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::IS_REPEATABLE)]', $proxyContent, @@ -1595,6 +1596,48 @@ public function testMultipleClasses(): void $actual = $this->normalizeWhitespaces($metadata->source); $expected = $this->normalizeWhitespaces($this->loadTestMetadata('multiple-classes-woven')->source); $this->assertEquals($expected, $actual); + + // Every woven class gets its own proxy below the source file name, like a file inside a phar (#760) + foreach (['TestClass1', 'TestClass11', 'TestClass2'] as $className) { + $proxyContent = (string) file_get_contents("vfs://Transformer/_files/multiple-classes.php/Test/ns3/{$className}.php"); + $this->assertStringContainsString("class {$className} implements", $proxyContent); + $this->assertStringContainsString("use {$className}OriginalTrait", $proxyContent); + } + } + + /** + * A file going from one woven class to several replaces the proxy named like the file with a directory of + * per-class proxies, and back + */ + public function testProxyLayoutFollowsNumberOfWovenClasses(): void + { + mkdir('vfs://layout/Transformer/_files', 0777, true); + $container = $this->getContainerMock(); + $kernel = $this->getKernelMock( + [ + 'appDir' => dirname(__DIR__), + 'cacheDir' => 'vfs://layout', + 'includePaths' => [], + 'excludePaths' => [], + 'cacheFileMode' => 0770, + ], + $container, + ); + $loader = $this->getMockBuilder(AspectLoader::class)->setConstructorArgs([$container])->getMock(); + $transformer = new WeavingTransformer($kernel, $this->adviceMatcher, new CachePathManager($kernel), $loader); + $proxyPath = 'vfs://layout/Transformer/_files/multiple-classes.php'; + + file_put_contents($proxyPath, 'transform($this->loadTestMetadata('multiple-classes')); + $this->assertFileExists($proxyPath . '/Test/ns3/TestClass1.php'); + + $proxyPath = 'vfs://layout/Transformer/_files/class.php'; + mkdir($proxyPath . '/Test/ns1', 0777, true); + file_put_contents($proxyPath . '/Test/ns1/Stale.php', 'loadTestMetadata('class'); + $transformer->transform($metadata); + $this->assertStringContainsString("include_once AOP_CACHE_DIR . '/Transformer/_files/class.php';", $metadata->source); + $this->assertFileIsReadable($proxyPath); } /** diff --git a/tests/Instrument/Transformer/_files/multiple-classes-woven.php b/tests/Instrument/Transformer/_files/multiple-classes-woven.php index 08a37ade..dd3dcd27 100644 --- a/tests/Instrument/Transformer/_files/multiple-classes-woven.php +++ b/tests/Instrument/Transformer/_files/multiple-classes-woven.php @@ -5,15 +5,15 @@ trait TestClass1OriginalTrait { public static function test() {} } -include_once AOP_CACHE_DIR . '/Transformer/_files/multiple-classes.php'; +include_once AOP_CACHE_DIR . '/Transformer/_files/multiple-classes.php/Test/ns3/TestClass1.php'; TestClass1::test(); trait TestClass11OriginalTrait { public static function test() {} } -include_once AOP_CACHE_DIR . '/Transformer/_files/multiple-classes.php'; +include_once AOP_CACHE_DIR . '/Transformer/_files/multiple-classes.php/Test/ns3/TestClass11.php'; TestClass11::test(); trait TestClass2OriginalTrait { public static function test() {} } -include_once AOP_CACHE_DIR . '/Transformer/_files/multiple-classes.php'; +include_once AOP_CACHE_DIR . '/Transformer/_files/multiple-classes.php/Test/ns3/TestClass2.php'; TestClass2::test(); diff --git a/tests/Instrument/Transformer/_files/multiple-ns-woven.php b/tests/Instrument/Transformer/_files/multiple-ns-woven.php index 62ded0ad..21cd5e7e 100644 --- a/tests/Instrument/Transformer/_files/multiple-ns-woven.php +++ b/tests/Instrument/Transformer/_files/multiple-ns-woven.php @@ -4,11 +4,11 @@ trait TestClass1OriginalTrait { public static function test() {} } -include_once AOP_CACHE_DIR . '/Transformer/_files/multiple-ns.php'; +include_once AOP_CACHE_DIR . '/Transformer/_files/multiple-ns.php/Test/ns1/TestClass1.php'; } namespace Test\ns2 { trait TestClass2OriginalTrait { public static function test() {} } -include_once AOP_CACHE_DIR . '/Transformer/_files/multiple-ns.php'; +include_once AOP_CACHE_DIR . '/Transformer/_files/multiple-ns.php/Test/ns2/TestClass2.php'; } diff --git a/tests/Instrument/Transformer/_files/php80-attribute-class-woven.php b/tests/Instrument/Transformer/_files/php80-attribute-class-woven.php index 795accb1..e9b51549 100644 --- a/tests/Instrument/Transformer/_files/php80-attribute-class-woven.php +++ b/tests/Instrument/Transformer/_files/php80-attribute-class-woven.php @@ -17,7 +17,7 @@ public function name(): string return 'grouped'; } } -include_once AOP_CACHE_DIR . '/Transformer/_files/php80-attribute-class.php'; +include_once AOP_CACHE_DIR . '/Transformer/_files/php80-attribute-class.php/Test/ns1/TestGroupedAttributeClass.php'; trait TestDynamicPropertiesClassOriginalTrait @@ -27,7 +27,7 @@ public function touch(): bool return true; } } -include_once AOP_CACHE_DIR . '/Transformer/_files/php80-attribute-class.php'; +include_once AOP_CACHE_DIR . '/Transformer/_files/php80-attribute-class.php/Test/ns1/TestDynamicPropertiesClass.php'; trait TestCustomAttributeOriginalTrait @@ -41,4 +41,4 @@ public function reason(): string return $this->reason; } } -include_once AOP_CACHE_DIR . '/Transformer/_files/php80-attribute-class.php'; +include_once AOP_CACHE_DIR . '/Transformer/_files/php80-attribute-class.php/Test/ns1/TestCustomAttribute.php'; diff --git a/tests/Instrument/Transformer/_files/php80-class-attribute-woven.php b/tests/Instrument/Transformer/_files/php80-class-attribute-woven.php index 01a68ab3..49606c67 100644 --- a/tests/Instrument/Transformer/_files/php80-class-attribute-woven.php +++ b/tests/Instrument/Transformer/_files/php80-class-attribute-woven.php @@ -17,7 +17,7 @@ public function doSomething(): int return 42; } } -include_once AOP_CACHE_DIR . '/Transformer/_files/php80-class-attribute.php'; +include_once AOP_CACHE_DIR . '/Transformer/_files/php80-class-attribute.php/Test/ns1/TestClassWithPlainAttribute.php'; #[\FakeMarkerAttr] @@ -28,4 +28,4 @@ public function doSomethingElse(): string return 'else'; } } -include_once AOP_CACHE_DIR . '/Transformer/_files/php80-class-attribute.php'; +include_once AOP_CACHE_DIR . '/Transformer/_files/php80-class-attribute.php/Test/ns1/TestClassWithArgumentAttribute.php';