Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[test-autoload] Trigger preload only in case of old php-parser to avoid double php-parser crash #6751

Merged
merged 3 commits into from
Feb 23, 2025

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Feb 23, 2025

@TomasVotruba TomasVotruba marked this pull request as draft February 23, 2025 00:21
@TomasVotruba TomasVotruba force-pushed the tv-avoid-double-autoload branch from c0bc99e to 1f03afc Compare February 23, 2025 00:26
@peterfox
Copy link
Contributor

Working for me now 👍

@TomasVotruba TomasVotruba marked this pull request as ready for review February 23, 2025 01:17
@TomasVotruba TomasVotruba merged commit 8d38ba3 into main Feb 23, 2025
44 checks passed
@TomasVotruba TomasVotruba deleted the tv-avoid-double-autoload branch February 23, 2025 01:17
@samsonasik
Copy link
Member

/cc @nikophil @staabm @markstory FYI

ensure verify this effect by remove this manual require in phpunit bootstrap

-require dirname(__DIR__) . '/vendor/rector/rector/preload.php';

line require to avoid conflict by test dev-main or next release, see the previous usage on cakephp upgrade:

https://github.com/cakephp/upgrade/blob/0f84083e7fa59315c5a37c8c38632d4670bd1001/tests/bootstrap.php#L14

@staabm
Copy link
Contributor

staabm commented Feb 23, 2025

I wonder why a 3rd party test-suite is/should including the rector preload.php at phpunit bootstrap phase?

@samsonasik
Copy link
Member

@staabm the preload is needed to ensure testing rector custom rule working to work with php-parser 5 over 4, which seems some conflict with some phpunit dependency which still on php-parser 4

@samsonasik
Copy link
Member

@staabm manual require should not longer needed, but may need removal of manual require if it was added in custom rules to avoid double require.

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Feb 23, 2025

@peterfox Could you share a bit more of your issue that you wrote me directly, so we have more context here for other readers?

@samsonasik
Copy link
Member

samsonasik commented Feb 23, 2025

Ref the original issue

that was require manual include rector preload, see

cakephp/upgrade@5b896c1#diff-96a82592013e5f4b91e682332583b7a4a6ca1ff0431d267c61179b1a21b167da

Yes, moving require means need to be verified to remove the previous manual require preload it in community rules, ensuring no double require that may cause another "the name is already in use" issue.

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.

4 participants