From 1b30a039a5426fa6ede1be7227443bf4d5a3bb0f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 18 Oct 2026 21:14:22 +0000 Subject: [PATCH 1/2] fix(weaving): give each woven class of a multi-class file its own proxy file A file declaring several woven classes wrote all proxies to one cache path named like the file, so every class but the last one was not found. The proxy of the only woven class of a file keeps the file name. A file with several woven classes gets one proxy per class below its name, used as a directory like a file inside a phar: `Holder.php/App/First.php`. Each proxy is still included right after its class. Proxies are saved once the whole file is woven, as the layout depends on the woven class count; switching layouts removes the proxies of the other one. debug:weaving finds the new proxies. Fixes #760 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KXjD75egSqDNujmNS5kr5c --- CHANGELOG.md | 1 + src/Console/Command/DebugWeavingCommand.php | 6 +- src/Instrument/AGENTS.md | 4 + .../Transformer/WeavingTransformer.php | 82 +++++++++++++++---- .../DebugWeavingCommandInProcessTest.php | 20 +++++ .../Transformer/WeavingTransformerTest.php | 49 ++++++++++- .../_files/multiple-classes-woven.php | 6 +- .../Transformer/_files/multiple-ns-woven.php | 4 +- .../_files/php80-attribute-class-woven.php | 6 +- .../_files/php80-class-attribute-woven.php | 4 +- 10 files changed, 150 insertions(+), 32 deletions(-) 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..ad4457c8 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,14 @@ private function processSingleClass( $childCode = 'declare(strict_types=1);' . PHP_EOL . $childCode; } - $contentToInclude = $this->saveProxyToCache($class, $childCode); - // Get last token for this class $classNode = $class->getNode(); $lastClassToken = $classNode->getAttribute('endTokenPos'); if (!is_int($lastClassToken)) { - return false; + return null; } - $metadata->tokenStream[$lastClassToken]->text .= PHP_EOL . $contentToInclude; - - return true; + return [$class, $childCode, $lastClassToken]; } /** @@ -969,9 +980,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 +998,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'; From 8531b1fe4e49567ac3e39a2431ecb42f70b956c1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 18 Oct 2026 21:17:00 +0000 Subject: [PATCH 2/2] refactor(weaving): read the last class token through getDeclarationTokenRange() Every conversion to a trait already reads the token range through it and fails without token positions, so the separate null branch was unreachable. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KXjD75egSqDNujmNS5kr5c --- src/Instrument/Transformer/WeavingTransformer.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Instrument/Transformer/WeavingTransformer.php b/src/Instrument/Transformer/WeavingTransformer.php index ad4457c8..e43878c9 100644 --- a/src/Instrument/Transformer/WeavingTransformer.php +++ b/src/Instrument/Transformer/WeavingTransformer.php @@ -212,12 +212,8 @@ private function processSingleClass( $childCode = 'declare(strict_types=1);' . PHP_EOL . $childCode; } - // Get last token for this class - $classNode = $class->getNode(); - $lastClassToken = $classNode->getAttribute('endTokenPos'); - if (!is_int($lastClassToken)) { - return null; - } + // The include of the proxy goes after the last token of the class + [, $lastClassToken] = $this->getDeclarationTokenRange($class->getNode()); return [$class, $childCode, $lastClassToken]; }