Skip to content

[CI] Re-add PHP-CS-Fixer - #2803

Merged
Kocal merged 1 commit into
symfony:2.xfrom
Kocal:ci/re-run-php-cs-fixer
May 31, 2025
Merged

[CI] Re-add PHP-CS-Fixer#2803
Kocal merged 1 commit into
symfony:2.xfrom
Kocal:ci/re-run-php-cs-fixer

Conversation

@Kocal

@Kocal Kocal commented May 31, 2025

Copy link
Copy Markdown
Member
Q A
Bug fix? no
New feature? no
Docs? no
Issues Fix #...
License MIT

Follow #2802, #2074, #2014 and more.

I feel like Fabbot only runs on modified files for a given PR, but sometimes it does not really help (e.g.: if you untouched files contain old references to a deleted class).

This behavior is understandable for symfony/symfony, which contains a tons of code and is super active, but that's not the case symfony/ux, we have very less code and less activity.

EDIT: Well, I'm still not sure... PHP-CS-Fixer is showing me fixes for src/LiveComponent/src/Util/TypeHelper.php, added in #2778, but it was totally ignored by Fabbot... 😫

So, I suggest to re-add PHP-CS-Fixer run in the CI to ensure all files are correctly CS-fixed.

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label May 31, 2025
@Kocal
Kocal force-pushed the ci/re-run-php-cs-fixer branch from 12cabbe to e0d0f06 Compare May 31, 2025 06:47
@Kocal
Kocal requested review from kbond and smnandre May 31, 2025 13:22

@kbond kbond left a comment

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 works for me. I don't think fabbot ever worked for the website code.

Once merged, I'll disable the webhook?

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels May 31, 2025
@Kocal

Kocal commented May 31, 2025

Copy link
Copy Markdown
Member Author

Once merged, I'll disable the webhook?

Mmmmh... I don't know, these checks can still be pretty useful (when they are correctly ran):
image

Can we keep both PHP-CS-Fixer and Fabbot?

@kbond

kbond commented May 31, 2025

Copy link
Copy Markdown
Member

Yeah no rush to remove it.

@smnandre

Copy link
Copy Markdown
Member

100% 👍

... and i'm thinking we should either find a way to exclude false positives in fabbot... or find a way to adapt our code to get green results when it runs

Comment thread .github/workflows/code-quality.yaml
- uses: actions/checkout@v4
- uses: shivammathur/setup-php@v2
with:
php-version: '8.1'

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.

we could use the php-cs-fixer tool provided by shivammathur also

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.

But we won't be able to run it locally, see #617

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.

Yep, easier to use the installed version so you don't have to duplicate version constraints. I forgot that I don't include in my packages - the ci fixes automatically.

php-version: '8.1'
- run: composer install
- name: php-cs-fixer
run: ./vendor/bin/php-cs-fixer fix --dry-run --diff

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.

Suggested change
run: ./vendor/bin/php-cs-fixer fix --dry-run --diff
run: ./vendor/bin/php-cs-fixer fix --dry-run --diff --show-progress=none

I have a doubt to be honest, it may be automatically set

@Kocal
Kocal force-pushed the ci/re-run-php-cs-fixer branch from e0d0f06 to 1ab48b6 Compare May 31, 2025 15:09
@Kocal
Kocal merged commit 1d64fb3 into symfony:2.x May 31, 2025
@Kocal
Kocal deleted the ci/re-run-php-cs-fixer branch May 31, 2025 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Reviewed Has been reviewed by a maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants