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.

Sign up for GitHub

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

Jump to bottom

[DX] Add "Related polyfill" feature to upgrade based on used symfony/polyfill-* packages #5388

Merged
merged 2 commits into from
Dec 24, 2023
Merged

[DX] Add "Related polyfill" feature to upgrade based on used symfony/polyfill-* packages #5388

merged 2 commits into from
Dec 24, 2023

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Dec 24, 2023 •
edited
Loading

@TomasVotruba TomasVotruba changed the title tv polyfill check [DX] Add related polyfill feature to allow upgrade based on used PHP symfony/polyfill-* packages Dec 24, 2023
@TomasVotruba TomasVotruba changed the title [DX] Add related polyfill feature to allow upgrade based on used PHP symfony/polyfill-* packages [DX] Add "Related polyfill" feature to upgrade based on used symfony/polyfill-* packages Dec 24, 2023
@TomasVotruba TomasVotruba force-pushed the tv-polyfill-check branch 7 times, most recently from c9105b7 to 7b8b0bd Compare December 24, 2023 12:44
@TomasVotruba
Copy link
Member Author

Let's ship it to test first 2 rules in the wild 👍

@TomasVotruba TomasVotruba merged commit ebb2d2d into main Dec 24, 2023
41 checks passed
@TomasVotruba TomasVotruba deleted the tv-polyfill-check branch December 24, 2023 12:50
samsonasik
samsonasik reviewed Dec 24, 2023
View reviewed changes
rules-tests/Php80/Rector/Identical/StrStartsWithRector/config/polyfill_configured_rule.php

$rectorConfig->phpVersion(PhpVersion::PHP_74);

$rectorConfig->polyfillPackages([PolyfillPackage::PHP_80]);
Copy link
Member

Choose a reason for hiding this comment

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

It needs test if rectorConfig->polyfillPackages() is not enabled, to verify it skipped

Copy link
Member

Choose a reason for hiding this comment

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

I created a PR to add test for it:

samsonasik
samsonasik reviewed Dec 24, 2023
View reviewed changes
packages/VersionBonding/PhpVersionedFilter.php
@@ -25,6 +28,15 @@ public function filter(iterable $rectors): array

$activeRectors = [];
foreach ($rectors as $rector) {
if ($rector instanceof RelatedPolyfillInterface) {
$polyfillPackageNames = $this->polyfillPackagesProvider->provide();
Copy link
Member

Choose a reason for hiding this comment

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

this can be moved before loop

Copy link
Member

Choose a reason for hiding this comment

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

alternatively, init before loop:

  $polyfillPackageNames = null;

and use coalesce on loop:

  $polyfillPackageNames ??= this->polyfillPackagesProvider->provide();

That cache is then no longer needed, as cached on loop itself to use isset data when available.

samsonasik
samsonasik reviewed Dec 24, 2023
View reviewed changes
src/Php/PolyfillPackagesProvider.php
/**
* @var array<PolyfillPackage::*>
*/
private array $cachedPolyfillPackages = [];
Copy link
Member

Choose a reason for hiding this comment

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

This should be nullable,

  • cache is null: never filled
  • cache is array, it filled, even empty, it means already read composer.json

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, could you add it?

Copy link
Member

Choose a reason for hiding this comment

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

sure, I created PR :

for it.

samsonasik
samsonasik reviewed Dec 24, 2023
View reviewed changes
src/Php/PolyfillPackagesProvider.php
return [];
}

if ($this->cachedPolyfillPackages !== []) {
Copy link
Member

Choose a reason for hiding this comment

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

It should compare to null

samsonasik
samsonasik reviewed Dec 24, 2023
View reviewed changes
src/Php/PolyfillPackagesProvider.php
}

$projectComposerJson = getcwd() . '/composer.json';
if (! file_exists($projectComposerJson)) {
Copy link
Member

Choose a reason for hiding this comment

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

file exists can be verified after null check cache on unitialized verify