Skip to content

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

Merged
TomasVotruba merged 2 commits intomainfrom
tv-polyfill-check
Dec 24, 2023
Merged

[DX] Add "Related polyfill" feature to upgrade based on used symfony/polyfill-* packages#5388
TomasVotruba merged 2 commits intomainfrom
tv-polyfill-check

Conversation

@TomasVotruba
Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba commented Dec 24, 2023

@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
Copy Markdown
Member Author

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

@TomasVotruba TomasVotruba merged commit ebb2d2d into main Dec 24, 2023
@TomasVotruba TomasVotruba deleted the tv-polyfill-check branch December 24, 2023 12:50

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

$rectorConfig->polyfillPackages([PolyfillPackage::PHP_80]);
Copy link
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

$activeRectors = [];
foreach ($rectors as $rector) {
if ($rector instanceof RelatedPolyfillInterface) {
$polyfillPackageNames = $this->polyfillPackagesProvider->provide();
Copy link
Copy Markdown
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
Copy Markdown
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.

/**
* @var array<PolyfillPackage::*>
*/
private array $cachedPolyfillPackages = [];
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

return [];
}

if ($this->cachedPolyfillPackages !== []) {
Copy link
Copy Markdown
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

}

$projectComposerJson = getcwd() . '/composer.json';
if (! file_exists($projectComposerJson)) {
Copy link
Copy Markdown
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

$composerContents = FileSystem::read($projectComposerJson);
$composerJson = Json::decode($composerContents, Json::FORCE_ARRAY);

$this->cachedPolyfillPackages = $this->filterPolyfillPackages($composerJson['require'] ?? []);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this fallback to empty array means the cache never works on no polifill found, that means composer.json will be re-read, that's why nullable for init value comparison is needed instead of compare to non-empty array

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.

[DX] Add RequiredPolyfillInterface to some rule to enable then, once the project is using polyfill package

2 participants