Skip to content

[TASK] Update PHP-CS-Fixer #592

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
Merged
merged 3 commits into from
Jun 21, 2024
Merged

[TASK] Update PHP-CS-Fixer #592

merged 3 commits into from
Jun 21, 2024

Conversation

oliverklee
Copy link
Collaborator

No description provided.

@oliverklee oliverklee added the dependencies Pull requests that update a dependency file label Jun 19, 2024
@oliverklee oliverklee requested review from sabberworm and JakeQZ June 19, 2024 17:41
@oliverklee oliverklee self-assigned this Jun 19, 2024
@oliverklee oliverklee marked this pull request as draft June 19, 2024 17:43
@oliverklee oliverklee marked this pull request as ready for review June 19, 2024 17:47
@oliverklee oliverklee requested review from sabberworm and JakeQZ June 19, 2024 17:47
JakeQZ
JakeQZ reviewed Jun 19, 2024
View reviewed changes
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

I just have a query about the comment in the config file, otherwise looks fine.

@oliverklee oliverklee requested a review from JakeQZ June 20, 2024 07:37
JakeQZ
JakeQZ reviewed Jun 20, 2024
View reviewed changes
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

I still think the comment is misleading - unless the defaults have actually changed and the docs are out of date.

@JakeQZ
Copy link
Collaborator

JakeQZ commented Jun 20, 2024

I still think the comment is misleading - unless the defaults have actually changed and the docs are out of date.

It appears the defaults have changed, breaking PHP 7.4 (even though it is supported by the tool), and all CI on whatever PHP version. The latest version thus breaks semantic versioning.

(I ran phive update. it installed the latest 3.x version, and I got loads of errors.)

Does the tool claim to follow semantic versioning? If not, then we should have a tilde version contraint in phars.xml. Or based on the above, should probably have that anyway.

@oliverklee
Copy link
Collaborator Author

Does the tool claim to follow semantic versioning?

No, I don't think so.

If not, then we should have a tilde version contraint in phars.xml. Or based on the above, should probably have that anyway.

I'd prefer not to: In phars.xml, we also store the exact version that will be installed by phive install, so this will only be relevant when someone uses phive update. And in that case, I'd like to get the latest version. (And yes, I know that a new version might require autoformatting the code or changes to the configuration to not break PHP 7.x.)

@oliverklee oliverklee requested a review from JakeQZ June 21, 2024 19:53
@oliverklee
Copy link
Collaborator Author

It appears the defaults have changed, breaking PHP 7.4 (even though it is supported by the tool),

That PHP-CS-Fixer supports PHP 7.4 means that it will run on PHP 7.4., not that it will create PHP 7.4-compliant code with all possible configurations. (The PER2 standard has some rules that only work on PHP 8.x. We'll need to avoid those for the time being.)

JakeQZ
JakeQZ approved these changes Jun 21, 2024
View reviewed changes
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

The comment now accurately conveys the reason for the setting, I think.

@JakeQZ JakeQZ merged commit 93835bc into main Jun 21, 2024
18 checks passed
@JakeQZ JakeQZ deleted the task/update-tools branch June 21, 2024 21:11
@JakeQZ
Copy link
Collaborator

JakeQZ commented Jun 22, 2024

That PHP-CS-Fixer supports PHP 7.4 means that it will run on PHP 7.4

Apparently not, with the match option explicitly enabled.

@oliverklee
Copy link
Collaborator Author

Apparently not, with the match option explicitly enabled.

Oh, I wasn't aware of that. So my learning is: Whether PHP-CS-Fixer runs under a specific PHP version also depends on the configuration of the rules. Thanks for pointing that out! 🙏

@JakeQZ
Copy link
Collaborator

JakeQZ commented Jun 22, 2024

Looking again, it seems that the default for trailing_comma_in_multiline has actually changed from ['after_heredoc' => false, 'elements' => ['arrays']] to ['after_heredoc' => false, 'elements' => ['arrays', 'arguments']], since PHP 7.4 supports trailing commas in function call arguments.

I think this is a bug in PHP-CS-Fixeer. The execution environment is irrelevant. The target environment is the important one. Also, since match itself is a PHP 8 language construct, it cannot possibly matter if a rule that could only apply to PHP 8 code is run in a PHP 7 environment. It is possible to have separate code paths based on PHP version.

We should remove the match option and/or file a bug with PHP-CS-Fixer.

For completeness, here is the error:

> "./.phive/php-cs-fixer" --config=config/php-cs-fixer.php fix --dry-run -v --show-progress=dots --diff bin src tests config
PHP CS Fixer 3.59.3 (064efa1) 7th Gear by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 7.4.16
Running analysis on 1 core sequentially.
You can enable parallel runner and speed up the analysis! Please see https://cs.symfony.com/doc/usage.html for more information.
Loaded config default from "config/php-cs-fixer.php".
Using cache file ".php-cs-fixer.cache".

In ConfigurableFixerTrait.php line 81:

  [PhpCsFixer\ConfigurationException\InvalidForEnvFixerConfigurationException (32)]
  [trailing_comma_in_multiline] Invalid configuration for env: "match" option can only be enabled with PHP 8.0+.


Exception trace:
  at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/Fixer/ConfigurableFixerTrait.php:81
 PhpCsFixer\Fixer\ControlStructure\TrailingCommaInMultilineFixer->configure() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/FixerFactory.php:174
 PhpCsFixer\FixerFactory->useRuleSet() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/Console/ConfigurationResolver.php:336
 PhpCsFixer\Console\ConfigurationResolver->getFixers() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/Console/Command/FixCommand.php:322
 PhpCsFixer\Console\Command\FixCommand->execute() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/vendor/symfony/console/Command/Command.php:298
 Symfony\Component\Console\Command\Command->run() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/vendor/symfony/console/Application.php:1040
 Symfony\Component\Console\Application->doRunCommand() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/Console/Application.php:177
 PhpCsFixer\Console\Application->doRunCommand() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/vendor/symfony/console/Application.php:301
 Symfony\Component\Console\Application->doRun() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/Console/Application.php:98
 PhpCsFixer\Console\Application->doRun() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/vendor/symfony/console/Application.php:171
 Symfony\Component\Console\Application->run() at C:\Users\Jake\.phive\phars\php-cs-fixer-3.59.3.phar:102

In TrailingCommaInMultilineFixer.php line 121:

  [PhpCsFixer\FixerConfiguration\InvalidOptionsForEnvException]
  "match" option can only be enabled with PHP 8.0+.


Exception trace:
  at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/Fixer/ControlStructure/TrailingCommaInMultilineFixer.php:121
 PhpCsFixer\Fixer\ControlStructure\TrailingCommaInMultilineFixer::PhpCsFixer\Fixer\ControlStructure\{closure}() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/vendor/symfony/options-resolver/OptionsResolver.php:1152
 Symfony\Component\OptionsResolver\OptionsResolver->offsetGet() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/vendor/symfony/options-resolver/OptionsResolver.php:924
 Symfony\Component\OptionsResolver\OptionsResolver->resolve() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/FixerConfiguration/FixerConfigurationResolver.php:130
 PhpCsFixer\FixerConfiguration\FixerConfigurationResolver->resolve() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/Fixer/ConfigurableFixerTrait.php:73
 PhpCsFixer\Fixer\ControlStructure\TrailingCommaInMultilineFixer->configure() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/FixerFactory.php:174
 PhpCsFixer\FixerFactory->useRuleSet() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/Console/ConfigurationResolver.php:336
 PhpCsFixer\Console\ConfigurationResolver->getFixers() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/Console/Command/FixCommand.php:322
 PhpCsFixer\Console\Command\FixCommand->execute() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/vendor/symfony/console/Command/Command.php:298
 Symfony\Component\Console\Command\Command->run() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/vendor/symfony/console/Application.php:1040
 Symfony\Component\Console\Application->doRunCommand() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/Console/Application.php:177
 PhpCsFixer\Console\Application->doRunCommand() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/vendor/symfony/console/Application.php:301
 Symfony\Component\Console\Application->doRun() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/src/Console/Application.php:98
 PhpCsFixer\Console\Application->doRun() at phar://C:/Users/Jake/.phive/phars/php-cs-fixer-3.59.3.phar/vendor/symfony/console/Application.php:171
 Symfony\Component\Console\Application->run() at C:\Users\Jake\.phive\phars\php-cs-fixer-3.59.3.phar:102

fix [--path-mode PATH-MODE] [--allow-risky ALLOW-RISKY] [--config CONFIG] [--dry-run] [--rules RULES] [--using-cache USING-CACHE] [--cache-file CACHE-FILE] [--diff] [--format FORMAT] [--stop-on-violation] [--show-progress SHOW-PROGRESS] [--sequential] [--] [<path>...]

Script "./.phive/php-cs-fixer" --config=config/php-cs-fixer.php fix --dry-run -v --show-progress=dots --diff bin src tests config handling the ci:php:fixer event returned with error code 32

@JakeQZ
Copy link
Collaborator

JakeQZ commented Jun 29, 2024

Since found it's not a BC introduced in PHP-CS-Fixer itself, but a in the @PER-CS2.0 ruleset update.

@JakeQZ JakeQZ mentioned this pull request Jun 30, 2024
Merged
JakeQZ pushed a commit that referenced this pull request Jun 30, 2024
@oliverklee
[TASK] Remove another PHP-8-only setting from the PHP-CS-Fixer config (…
27aa880 
…#629)

Resolves problem introduced by #592.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakeQZ JakeQZ JakeQZ approved these changes

@sabberworm sabberworm Awaiting requested review from sabberworm

Assignees

@oliverklee oliverklee

Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
9.0.0: Drop support for PHP < 7.2 and...
Development

Successfully merging this pull request may close these issues.
2 participants
@oliverklee @JakeQZ