Skip to content

Added Rector and CS-Fixer #15

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

Open
ArthegaAsdweri wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added Rector and CS-Fixer #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions .github/workflows/php-cs-fixer.yml
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
name: PHP-CS-Fixer

on:
pull_request:
paths:
- '**.php'
- tools/php-cs-fixer/composer.json
- php-cs-fixer.php
- .github/workflows/php-cs-fixer.yml

jobs:
csfixer:
runs-on: ubuntu-latest
strategy:
matrix:
php: ['8.2', '8.4']
name: PHP-CS-Fixer (PHP ${{ matrix.php }})

steps:
- uses: actions/checkout@v4

- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
coverage: none
env:
fail-fast: true

- name: Get composer cache directory
id: composer-cache
run: echo "dir=$(composer config cache-files-dir)" >> $GITHUB_OUTPUT

- name: Cache dependencies
uses: actions/cache@v4
with:
path: ${{ steps.composer-cache.outputs.dir }}
key: ${{ runner.os }}-composer-${{ hashFiles('tools/php-cs-fixer/composer.json') }}
restore-keys: ${{ runner.os }}-composer-

- name: Install dependencies
run: composer composer-cs-fixer -- update --no-progress --prefer-dist

- name: Run PHP-CS-Fixer (dry-run)
run: composer cs-fixer
2 changes: 1 addition & 1 deletion .github/workflows/phpstan.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
php-versions: ['8.1', '8.4']
php-versions: ['8.2', '8.4']
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

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.

prefer: ['prefer-stable', 'prefer-lowest']
name: PHPStan with PHP ${{ matrix.php-versions }} ${{ matrix.prefer }}

Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/phpunit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jobs:
strategy:
fail-fast: true
matrix:
php-versions: ['8.1', '8.4']
php-versions: ['8.2', '8.4']
prefer: ['prefer-stable', 'prefer-lowest']
name: PHPUnit with PHP ${{ matrix.php-versions }} ${{ matrix.prefer }}

Expand Down
45 changes: 45 additions & 0 deletions .github/workflows/rector.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
name: Rector

on:
pull_request:
paths:
- '**.php'
- tools/rector/composer.json
- rector.php
- .github/workflows/rector.yml

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

@dontub dontub Feb 5, 2026

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?

name: Rector (PHP ${{ matrix.php }})

steps:
- uses: actions/checkout@v4

- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
coverage: none
env:
fail-fast: true

- name: Get composer cache directory
id: composer-cache
run: echo "dir=$(composer config cache-files-dir)" >> $GITHUB_OUTPUT

- name: Cache dependencies
uses: actions/cache@v4
with:
path: ${{ steps.composer-cache.outputs.dir }}
key: ${{ runner.os }}-composer-${{ hashFiles('tools/rector/composer.json') }}
restore-keys: ${{ runner.os }}-composer-

- name: Install dependencies
run: composer composer-rector -- update --no-progress --prefer-dist

- name: Run Rector (dry-run)
run: composer rector:test
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/.phpcs.cache
/.phpunit.result.cache
/.php-cs-fixer.cache
/.phpstan/
/composer.lock
/phpstan.neon
Expand Down
3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ Recommended approach:
* Run `composer run -- phpstan --generate-baseline`.
* Include `phpstan-baseline.neon` in `phpstan.neon.dist`:
```
parameters:
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

Choose a reason for hiding this comment

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

Shall this indicate that includes is on the same level as parameters?

...

includes:
- phpstan-baseline.neon
```
Expand Down
29 changes: 28 additions & 1 deletion composer.json.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
}
},
"require": {
"php": "^8.1",
"php": "^8.2",
"civicrm/civicrm-core": ">={EXT_MIN_CIVICRM_VERSION}",
"civicrm/civicrm-packages": ">={EXT_MIN_CIVICRM_VERSION}"
},
Expand All @@ -39,7 +39,15 @@
"composer-phpunit": [
"@composer --working-dir=tools/phpunit"
],
"composer-rector": [
"@composer --working-dir=tools/rector"
],
"composer-cs-fixer": [
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

Choose a reason for hiding this comment

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

I'd name it composer-php-cs-fixer to be consistent.

"@composer --working-dir=tools/php-cs-fixer"
],
"composer-tools": [
"@composer-cs-fixer",
"@composer-rector",
"@composer-phpcs",
"@composer-phpstan",
"@composer-phpunit"
Expand All @@ -56,7 +64,26 @@
"phpunit": [
"@php tools/phpunit/vendor/bin/simple-phpunit --coverage-text"
],
"rector": [
"@php tools/rector/vendor/bin/rector process"
],
"rector:test": [
"@php tools/rector/vendor/bin/rector process --dry-run"
],
"cs-fixer": [
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

Choose a reason for hiding this comment

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

I'd name it php-cs-fixer:fix to be consistent.

I think it's not necessary to explicitly specify the config file.

"@php tools/php-cs-fixer/vendor/bin/php-cs-fixer fix --config=php-cs-fixer.php"
],
"cs-fixer:test": [
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

Choose a reason for hiding this comment

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

I'd name it php-cs-fixer:test to be consistent. Though php-cs-fixer also has a check command. Maybe we could use that here instead of doing --dry-run and use php-cs-fixer:check here.

I think it's not necessary to explicitly specify the config file.

"@php tools/php-cs-fixer/vendor/bin/php-cs-fixer fix --dry-run --diff --config=php-cs-fixer.php"
],
"fix": [
"@rector",
"@cs-fixer",
"@phpcbf"
],
"test": [
"@rector:test",
"@cs-fixer:test",
"@phpcs",
"@phpstan",
"@phpunit"
Expand Down
22 changes: 22 additions & 0 deletions php-cs-fixer.php.template
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

Choose a reason for hiding this comment

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

As this file contains no placeholder the extension .template isn't necessary.

I think it should be named .php-cs-fixer.dist.php.

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

declare(strict_types = 1);

use PhpCsFixer\Config;
use PhpCsFixer\Finder;

$finder = Finder::create()
->in([
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

Choose a reason for hiding this comment

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

The readme should be changed accordingly saying that this might need to be adapted. (Similar instructions already exist for the configurations of phpcs and phpstan.)

__DIR__ . '/CRM',
__DIR__ . '/Civi',
__DIR__ . '/api',
__DIR__ . '/tests',
])
->exclude(['vendor', 'node_modules']);
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

Choose a reason for hiding this comment

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

I'm wondering why this should be necessary. The given directories shouldn't be inside of any of the included directories.


return (new Config())
->setRiskyAllowed(TRUE)
->setRules([
'declare_strict_types' => TRUE,
])
->setFinder($finder);
38 changes: 38 additions & 0 deletions rector.php.template
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?php

declare(strict_types = 1);

use Rector\Config\RectorConfig;
use Rector\Php80\Rector\Class_\ClassPropertyAssignToConstructorPromotionRector;
use Rector\TypeDeclaration\Rector\ClassMethod\AddVoidReturnTypeWhereNoReturnRector;
use Rector\TypeDeclaration\Rector\StmtsAwareInterface\SafeDeclareStrictTypesRector;
use Rector\Set\ValueObject\SetList;
use Rector\ValueObject\PhpVersion;

return RectorConfig::configure()
->withPaths([
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

Choose a reason for hiding this comment

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

The readme should be changed accordingly saying that this might need to be adapted. (Similar instructions already exist for the configurations of phpcs and phpstan.)

__DIR__ . '/api',
__DIR__ . '/Civi',
__DIR__ . '/CRM',
__DIR__ . '/{EXT_SHORT_NAME}.php',
//__DIR__ . '/tests',
])
->withSkip([
__DIR__ . '/vendor',
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

Choose a reason for hiding this comment

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

This directory isn't part of withPaths. Is it actually necessary here?

__DIR__ . '/CRM/*/DAO',
__DIR__ . '/CRM/*/DAO/*',
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

Choose a reason for hiding this comment

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

Do this line and the one before behave differently?

__DIR__ . '/tools',
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

Choose a reason for hiding this comment

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

This directory isn't part of withPaths. Is it actually necessary here?

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

@dontub dontub Feb 5, 2026

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.

SetList::CODE_QUALITY,
SetList::DEAD_CODE,
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

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::TYPE_DECLARATION,
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

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?

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

@dontub dontub Feb 5, 2026

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.

->withPhpSets(php82: true);
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

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

8 changes: 8 additions & 0 deletions tools/php-cs-fixer/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"require-dev": {
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

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.

"friendsofphp/php-cs-fixer": "^3.0"
},
"config": {
"sort-packages": true
}
}
11 changes: 11 additions & 0 deletions tools/rector/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"require-dev": {
Copy link
Copy Markdown
Contributor

@dontub dontub Feb 5, 2026

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.

"rector/rector": "^2.3"
},
"config": {
"sort-packages": true,
"allow-plugins": {
"dealerdirect/phpcodesniffer-composer-installer": true
}
}
}
Loading