Skip to content

Conversation

@TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Oct 7, 2025

This happens when:

  • project uses phpstan rule tests
  • phpunit runs
  • rector rule test case runs second

Copy link
Member

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may cause invalid phpunit test for rector rules, since we need load preload script, see

private function includePreloadFilesAndScoperAutoload(): void
{
if (file_exists(__DIR__ . '/../../../preload.php')) {
if (file_exists(__DIR__ . '/../../../vendor')) {
require_once __DIR__ . '/../../../preload.php';
// test case in rector split package
} elseif (file_exists(__DIR__ . '/../../../../../../vendor')) {
require_once __DIR__ . '/../../../preload-split-package.php';
}
}
if (\file_exists(__DIR__ . '/../../../vendor/scoper-autoload.php')) {
require_once __DIR__ . '/../../../vendor/scoper-autoload.php';
}

Also, please run the script:

php build/build-preload.php .

so it regenerated and we can test.

@TomasVotruba TomasVotruba force-pushed the tv-autoload-conflict branch 2 times, most recently from a56cb3d to 9ff48de Compare October 7, 2025 12:17
Comment on lines +5 to +6
use PhpParser\Node;
use PHPStan\Testing\PHPStanTestCase;
Copy link
Member

@samsonasik samsonasik Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may cause repetitive apply cs after rebuild weekly, if cs is required, the cs fix position needs to be after generate preload on workflow build so it always have same result:

-
name: 'Apply Coding Standard'
run: "composer fix-cs"
branch: 'automated-apply-coding-standards'
-
name: 'Re-Generate preload.php'
run: "composer preload"
branch: 'automated-regenerated-preload'

or on build/build-preload.php script, or add use statement early there.

@samsonasik
Copy link
Member

/cc @Androoha1 @nikophil @staabm @megawubs could you verify manual this patch, thank you.

update vendor/rector/rector/preload.php with the preload.php updated here.

@samsonasik
Copy link
Member

samsonasik commented Oct 7, 2025

Also @ivastly @nikophil @devnix , could you verify manual this patch with the following steps, thank you.

  • remove hack on composer.json -> files handling
  • run composer update
  • Manually update vendor/rector/rector/preload.php with the preload.php updated here

@samsonasik
Copy link
Member

Also @ostrolucky @smnandre could you verify manual this patch with the following steps, thank you.

  • remove hack on composer.json -> files handling
  • run composer update
  • Manually update vendor/rector/rector/preload.php with the preload.php updated here.

Ref of existing PR on your repository that use composer.json -> files handling.

Copy link
Member

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomasVotruba I tested manually on https://github.com/ostrolucky/rector-rules repository, and seems still error:

➜  rector-rules git:(main) ✗ vendor/bin/phpunit
PHP Fatal error:  Cannot declare interface PhpParser\NodeVisitor, because the name is already in use in /Users/samsonasik/www/rector-rules/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeVisitor.php on line 6

Fatal error: Cannot declare interface PhpParser\NodeVisitor, because the name is already in use in /Users/samsonasik/www/rector-rules/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeVisitor.php on line 6

Step to reproduce:

@staabm
Copy link
Contributor

staabm commented Oct 7, 2025

could you verify manual this patch, thank you.

@samsonasik what do you want to be tested?

@samsonasik
Copy link
Member

samsonasik commented Oct 7, 2025

@staabm I already tested on phpunit 12, and it still error with this patch as above #7440 (review)

➜  rector-rules git:(main) ✗ vendor/bin/phpunit
PHP Fatal error:  Cannot declare interface PhpParser\NodeVisitor, because the name is already in use in /Users/samsonasik/www/rector-rules/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeVisitor.php on line 6

Fatal error: Cannot declare interface PhpParser\NodeVisitor, because the name is already in use in /Users/samsonasik/www/rector-rules/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeVisitor.php on line 6

Step to reproduce:

@TomasVotruba
Copy link
Member Author

@samsonasik This is not related to PHPUnit 12. This bug existed before. It's about order of the tests loaded.
If PHPStan test case is loaded first (by any testing tool), then RectorTestCase, it would trigged loading php-parser classes twice. This PR avoid it and gives priority to PHPStan classes. Despite not having access to stmts aware interface, that's the trade off.

@TomasVotruba TomasVotruba merged commit b177eb8 into main Oct 7, 2025
50 checks passed
@TomasVotruba TomasVotruba deleted the tv-autoload-conflict branch October 7, 2025 12:57
@samsonasik
Copy link
Member

@TomasVotruba then the tweak still needed? I tested on https://github.com/ostrolucky/rector-rules, apply this patch, and still error.

@TomasVotruba
Copy link
Member Author

Let's give it a go. Test this in projects with both PHPStan and Rector tests.

@TomasVotruba
Copy link
Member Author

@samsonasik For PHPUnit 12, yes. This is not related to PHPUnit version, but autoload.

'*/Expected/*',

// avoid re-running on build
__DIR__ . '/preload.php',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also preload-split-package.php:

Suggested change
__DIR__ . '/preload.php',
__DIR__ . '/preload.php',
__DIR__ . '/preload-split-package.php',

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Oct 7, 2025

I'll check https://github.com/ostrolucky/rector-rules repo

}

return interface_exists(Node::class, false);
}require_once __DIR__ . '/../../../vendor/nikic/php-parser/lib/PhpParser/Node.php';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line is needed here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Oct 7, 2025

Let's try reviving 3 years old PR that solves this magic autoload workarounds :)
nikic/PHP-Parser#836

@samsonasik
Copy link
Member

samsonasik commented Oct 7, 2025

@TomasVotruba for note, we not only patch to apply StmtsAwareInterface, but also NodeTraverser.php for cache NodeVisitor

https://github.com/rectorphp/vendor-patches/blob/18e174cbea7beaccb140cfcf03cca3097e95ee92/patches/nikic-php-parser-lib-phpparser-nodetraverser-php.patch

so we still can't rely on "in project" nikic/php-parser vendor.

@samsonasik
Copy link
Member

@TomasVotruba in preload.php, we also load phpstan-phpdoc-parser classes, so the order still matters.

private function buildPreloadScriptForSplitPhpDocParser(string $buildDirectory, string $preloadFile): void
{
$vendorDir = $buildDirectory . '/vendor';
if (! is_dir($vendorDir . '/phpstan/phpdoc-parser')) {
return;
}
// 1. find phpdoc-parser file infos
$fileInfos = $this->findPhpDocParserFilesAndSortThem($vendorDir);
// 2. create preload-split-package.php from provided files
$preloadFileContent = $this->createPreloadFileContentForSplitPackage($fileInfos, true);
file_put_contents($preloadFile, $preloadFileContent, FILE_APPEND);
}

@TomasVotruba
Copy link
Member Author

@samsonasik Let's resolve one problem at a time.

@nikophil
Copy link
Contributor

nikophil commented Oct 7, 2025

Hi @samsonasik

sorry for the late reply.

I tried to update rector, and manualy modify preload.php and still have the problem with PhpParser

tried on Foundry's rules : vendor/bin/phpunit -c phpunit-rector.xml.dist

@TomasVotruba
Copy link
Member Author

@nikophil This is not related to your issue. It's a different case: #7440 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants