Skip to content

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

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.

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

Merged
merged 1 commit into from
May 31, 2025
Merged

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

merged 1 commit into from
May 31, 2025

Conversation

Kocal
Copy link
Member

@Kocal Kocal commented May 31, 2025

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
Copy link
Member

@kbond kbond left a comment

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
Copy link
Member Author

Kocal commented May 31, 2025

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
Copy link
Member

kbond commented May 31, 2025

Yeah no rush to remove it.

@smnandre
Copy link
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

- uses: actions/checkout@v4
- uses: shivammathur/setup-php@v2
with:
php-version: '8.1'
Copy link
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
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
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
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
1 check was pending
@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