Skip to content

Added Rector and CS-Fixer#15

Open
ArthegaAsdweri wants to merge 1 commit intomainfrom
rector+csfixer-integration
Open

Added Rector and CS-Fixer#15
ArthegaAsdweri wants to merge 1 commit intomainfrom
rector+csfixer-integration

Conversation

@ArthegaAsdweri
Copy link
Copy Markdown
Contributor

No description provided.

@ArthegaAsdweri ArthegaAsdweri requested a review from dontub February 5, 2026 11:25
strategy:
matrix:
php-versions: ['8.1', '8.4']
php-versions: ['8.2', '8.4']
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should keep PHP 8.1 aus default minimum for now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As far as I can see the only rule enabled in the php-cs-fixer config is declare_strict_types. In that case there's nothing that's not already checked by phpcs. So I'm wondering if it makes sense to have a GitHub workflow for php-cs-fixer.

runs-on: ubuntu-latest
strategy:
matrix:
php: ['8.2', '8.4']
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are there different rules applied with PHP 8.2 and PHP 8.4?

@@ -0,0 +1,8 @@
{
"require-dev": {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this case it's actually not a dev dependency.

@@ -0,0 +1,11 @@
{
"require-dev": {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this case it's actually not a dev dependency.

->withSets([
SetList::CODE_QUALITY,
SetList::DEAD_CODE,
//SetList::TYPE_DECLARATION,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason to have this line or should it be removed?

])
->withSets([
SetList::CODE_QUALITY,
SetList::DEAD_CODE,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

phpstan also has some dead code checking. Does it make sense to additionally have this one?

SetList::EARLY_RETURN,
])
->withPhpVersion(PhpVersion::PHP_82)
->withPhpSets(php82: true);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we could just use withPhpSets() as the documentation says:

The best practise is to use PHP version defined in composer.json. Rector will automatically pick it up with empty ->withPhpSets() method

//SetList::TYPE_DECLARATION,
SetList::EARLY_RETURN,
])
->withPhpVersion(PhpVersion::PHP_82)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we don't need this as the documentation says:

Rector reads the PHP version from composer.json. It's very rare to use different PHP version than one provided by composer.json, as it might use newer syntax and break your code.

->withRules([
SafeDeclareStrictTypesRector::class,
])
->withSets([
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As far as I can understand the documentation indicates to use withPreparedSets() and to use withSets() for community or external sets.

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.

2 participants