Activity

1. GErpeldinger commented on Sep 29, 2026

   

   GErpeldinger

   on Sep 29, 2026

   Author

   More actions

   Hey @TomasVotruba, @samsonasik,

   rectorphp/rector-src#8512 was closed, but the cache bug still reproduces on rector-src main, also with PHP-Parser master (which includes nikic/PHP-Parser#1168).

   To see how common it is, I ran Rector once on two big projects, then a dry run. In the dry run, these files are affected by this bug (reported as changed, but the output is identical to the input):

   • symfony/symfony 8.2, with withImportNames(): 47 files (22 in source code, 25 in tests)
   • Sylius 2.3, with its withImportNames() options (importShortClasses: false, removeUnusedImports: true): 67 files

   With the fix from rectorphp/rector-src#8512, none of them is reported.

   How I counted

   One registered rule that matches nothing (Rector needs at least one rule), one normal run, then --dry-run --clear-cache. symfony/symfony: src/, with */Fixtures/* skipped. Sylius: src/ and tests/.

   The same symfony/symfony dry run also reports 6 files with the same symptom, from two other causes:

   1. DocblockNameImportingPostRector: a fully qualified name in the docblock of an anonymous class (5 files, for example HttpFoundation/ParameterBag.php)

   <?php
   
   namespace App;
   
   use ArrayIterator;
   
   $iterator = new /** @extends \ArrayIterator<string, mixed> */ class([]) extends ArrayIterator {};

   2. NameImportingPostRector: a file without a namespace that uses the same class more than once (1 file, Bridge/PhpUnit/bin/simple-phpunit.php)

   <?php
   
   $flags = FilesystemIterator::KEY_AS_FILENAME | FilesystemIterator::UNIX_PATHS;

   With #9910 (a regular rule, already fixed), that makes 4 bugs with this symptom.

   When the bug is in a post rector, a doTestFile() fixture cannot detect it: the output is the same as the input, and the "File not changed but some Rector rules applied" warning skips post rectors. I propose a check in AbstractRectorTestCase: when the output is the same as the input but a post rector reported a change, the test fails.

   // AbstractRectorTestCase::doTestFileMatchesExpectedContent()
   if ($changedContents === $inputFileContents) {
       $this->assertSame(
           [],
           $rectorTestResult->getAppliedPostRectorClasses(),
           $failureMessage . PHP_EOL . 'The file is unchanged, but post rectors reported a change, so a dry run would not cache it'
       );
   }
   
   // RectorTestResult
   /**
    * @return array<class-string<PostRectorInterface>>
    */
   public function getAppliedPostRectorClasses(): array
   {
       $postRectorClasses = [];
   
       foreach ($this->processResult->getFileDiffs(false) as $fileDiff) {
           foreach ($fileDiff->getRectorChanges() as $rectorWithLineChange) {
               $rectorClass = $rectorWithLineChange->getRectorClass();
               if (is_a($rectorClass, PostRectorInterface::class, true)) {
                   $postRectorClasses[] = $rectorClass;
               }
           }
       }
   
       return array_values(array_unique($postRectorClasses));
   }

   The existing getAppliedRectorClasses() cannot be reused, because it does not return post rectors since rectorphp/rector-src#7365. The list of applied rules shown to users does not change.

   As skip fixtures, this issue and the two cases above fail with the check and pass without it. The check makes no existing test fail: I ran rector-src and 7 packages (rector-symfony, rector-doctrine, rector-phpunit, rector-downgrade-php, rector-laravel, typo3-rector, drupal-rector). In total, the check takes under 20 ms on the whole rector-src suite. Third-party packages also use AbstractRectorTestCase; if a failing test is too strict for them, the check can first print a warning, like the existing one.

   Should I open a PR, and separate issues for the two new cases?
2. TomasVotruba commented on Sep 29, 2026

   

   TomasVotruba

   on Sep 29, 2026

   Last edited by TomasVotruba

   Member

   More actions

   This is hard to read. Can you put it in plain words?
3. GErpeldinger commented on Sep 29, 2026

   

   GErpeldinger

   on Sep 29, 2026

   Last edited by GErpeldinger

   Author

   More actions

   @TomasVotruba

   • perf: Group NameResolver::enterNode() checks by node category nikic/PHP-Parser#1168 does not fix this bug: with withImportNames(), --dry-run still reports files as changed when nothing changes, so they are never cached.
   • It is not rare: 47 files in symfony/symfony, 67 in Sylius and 43 in my work project.
   • In symfony/symfony I also found 2 other post rector bugs of this kind so it's recurring.
   • No fixture test can catch this kind of bug today. A small check in AbstractRectorTestCase can: the test fails when the file does not change but a post rector says it changed.

   Do you want a PR for the change in AbstractRectorTestCase ? With this check, a test can show these bugs, so they can be fixed and stay fixed. And hopefully simplify the readability problem that you asked about in rectorphp/rector-src#8512