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

[Experiment] Use composer.json as priority, fallback to php version features on when parameter exists on ComposerJsonPhpVersionResolver #6384

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik force-pushed the experiment-use-composer-json-priority-fallback branch from 6af5727 to d9ee208 Compare October 16, 2024 04:31
@samsonasik
Copy link
Member Author

implemented 🎉, here the summary:

This PR provide a way to set withPhpVersion() even composer.json not exists or doesn't define "php" config, so it fallback php version feature defined, with priority:

  1. read composer.json, if exists and define "php" config, then use it
  2. verify if it has Option::PHP_VERSION_FEATURES via withPhpVersion(), if exists, then use it as fallback
  3. throw error because nothing to rely

The config can be:

use Rector\Config\RectorConfig;
use Rector\ValueObject\PhpVersionFeature;

return RectorConfig::configure()
    ->withPaths([__DIR__ . '/src'])
    ->withPhpVersion(PhpVersionFeature::ATTRIBUTES)
    ->withPhpLevel(2);

I add e2e test on this PR for proof that it works for apply on level 2, but current php version is php 8, which only apply php 5.2 change:

-ldap_first_attribute($ldap, $entry, $more);
+ldap_first_attribute($ldap, $entry);

By this PR, I found a bug duplicated ContinueToBreakInSwitchRector register which registered in php52 and php73 config 06026a0

I will create separate PR to remove duplicated register ContinueToBreakInSwitchRector

@samsonasik
Copy link
Member Author

I created separate PR to remove duplicated register ContinueToBreakInSwitchRector that exists in both php52 and php73 config

Comment on lines +11 to +15
+ldap_first_attribute($ldap, $entry);
----------- end diff -----------

Applied rules:
* RemoveExtraParametersRector
Copy link
Member Author

Choose a reason for hiding this comment

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

it should show RemoveFuncCallArgRector per php 5.2 config set, but it shows RemoveExtraParametersRector on php 7.1 set, it probably due to array_merge() config so configured config is late instead of added, that's seems needs separate PR for that.

Comment on lines +3 to +5
final class SomeClass
{
public function __construct(private readonly string $b)
Copy link
Member Author

Choose a reason for hiding this comment

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

this not transformed to readonly class even with PHP_INT_MAX on level, as I define

->withPhpVersion(PhpVersionFeature::ATTRIBUTES)

on rector.php e2e test https://github.com/rectorphp/rector-src/pull/6384/files#diff-464b8708ccfbbbeecb99d3dc19bea3998a6d55af99232759e70edae715f3abf0 so it only apply up to php 8.0 feature.

@@ -37,7 +36,6 @@
$rectorConfig->rules([
StringifyStrNeedlesRector::class,
RegexDashEscapeRector::class,
ContinueToBreakInSwitchRector::class,
Copy link
Member Author

@samsonasik samsonasik Oct 16, 2024

Choose a reason for hiding this comment

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

it exists already in php52 config set, I removed it to avoid duplicated error message.

cd e2e/with-php-level && composer install
with-php-level git:(experiment-use-composer-json-priority-fallback) ✗ php ../e2eTestRunner.php
    ---------- begin diff ----------
@@ @@
-[ERROR] Following rules are registered twice: Rector\Php52\Rector\Switch_\ContinueToBreakInSwitchRector

removed in separate PR:

@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba it is ready for review.

I don't rebase cherry-pick patch #6385 so you can see the process and how it found a bug with e2e test :)

@@ -0,0 +1,2 @@
{
}
Copy link
Member Author

Choose a reason for hiding this comment

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

empty composer.json on purpose so withPhpVersion() can act as fallback

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.

1 participant