Repository navigation
Conversation
…() called with no argument
samsonasik
left a comment
There was a problem hiding this comment.
I think "framework" can be changed to "package" for generic wording
|
I like the idea 👍 The Symfony/Laravel would have to be triggered by multiple packages though. E.g. |
|
Thanks to both of you, updated in 995bbfe:
The test now uses a fixture project with |
|
The failing "PHP 8.4 tests (ubuntu-latest)" job is not related to this PR. It is a race between two test classes that share one fixture directory.
The first push passed this job, and nothing under |
| 'twig' => ['twig/twig'], | ||
| 'doctrine' => ['doctrine/orm', 'doctrine/dbal', 'doctrine/doctrine-bundle'], | ||
| 'phpunit' => ['phpunit/phpunit'], | ||
| 'symfony' => ['symfony/framework-bundle', 'symfony/http-kernel', 'symfony/console'], |
There was a problem hiding this comment.
This should be 20-30 packages, check the linked set for more packages we upgrade.
Ideally build a script that extracts these values, and commit it here, so we have this list up-to-date.
There was a problem hiding this comment.
Done in aa524fe. scripts/generate-composer-based-trigger-packages.php loads each composer-based set and collects the packages its rules are bound to. It covers both ComposerPackageConstraintInterface rules and ruleWithConfigurationComposerVersionBound() configurations, and writes them to src/Configuration/ComposerBasedTriggerPackages.php.
The result is 57 packages for Symfony and 9 for Doctrine. Laravel and Drupal stay hardcoded in the script, because rector-laravel and drupal-rector are not required in this repository. A test checks that every withComposerBased() argument has an entry in the generated list.
Should CI also run the script and fail when the generated list is out of date? I left that out because rector-symfony and rector-doctrine are pulled in as dev-main. Any change there would then fail unrelated PRs until someone regenerates the list. I'm happy to add it if you prefer.
| @@ -0,0 +1,132 @@ | |||
| <?php | |||
There was a problem hiding this comment.
I think the script should be on github action with weekly run?
see https://github.com/rectorphp/rector-src/blob/main/.github/workflows/weekly_pull_requests.yaml
There was a problem hiding this comment.
We can run it per commit, to fail if new package is there.
How fast it is?
There was a problem hiding this comment.
run locally, seems fast enough: 0.253 seconds
➜ rector-src git:(composer-based-auto-detect) time php scripts/generate-composer-based-trigger-packages.php
twig: 1 packages
doctrine: 9 packages
phpunit: 1 packages
symfony: 56 packages
laravel: 2 packages
drupal: 1 packages
[OK] File "/Users/samsonasik/www/rector-src/src/Configuration/ComposerBasedTriggerPackages.php" was generated
0.19s user 0.03s system 87% cpu 0.253 total
withPhpSets()with no argument reads the PHP version fromcomposer.json.withComposerBased()with no argument currently does nothing, so every project lists its packages by hand:This PR makes the no-argument call pick the sets of the packages installed in the project:
->withComposerBased()Passing any argument keeps today's behavior: only the passed sets are loaded, and detection is skipped.
withComposerBased(phpunit: false)loads nothing, as before.Detection
A set loads when any of its trigger packages is installed.
InstalledPackageResolverreads the installed packages.The trigger packages are listed in
src/Configuration/ComposerBasedTriggerPackages.php. That file is generated byphp scripts/generate-composer-based-trigger-packages.php, which reads every package the composer-based sets are bound to. It covers both rules that implementComposerPackageConstraintInterfaceand configurations registered withruleWithConfigurationComposerVersionBound(). Re-run the script when a set starts upgrading a new package.symfonysymfony/*components and bundles, plusjms/serializerandtwig/twigdoctrinedoctrine/orm,doctrine/dbal,doctrine/doctrine-bundle,doctrine/mongodb-odm,gedmo/doctrine-extensions, and otherstwigtwig/twigphpunitphpunit/phpunitlaravellaravel/framework,illuminate/support(hardcoded in the script, as rector-laravel is not required here)drupaldrupal/core(hardcoded in the script, as drupal-rector is not required here)Each rule in these sets is bound to its own package version, so a broad trigger is safe. A rule bound to a package that is not installed stays inactive.
BC
bool ... = false). The no-argument path is detected withfunc_num_args() === 0.withComposerBased()with no arguments loaded nothing before. Now it loads the detected sets, which is the intent of such a call.Tests
tests/Configuration/ComposerBasedAutoDetectTest.phpuses two fixture projects. One hasdoctrine/dbal,phpunit/phpunitandsymfony/consoleinstalled; the other has none of the trigger packages. It covers four cases, and a fifth test checks that everywithComposerBased()argument has an entry in the generated list:doctrine/dbaltriggers Doctrine andsymfony/consoletriggers Symfony.twig: trueloads only Twig.phpunit: falseloads nothing.