Skip to content
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

CI: Ensure php-cs-fixer PHP compatibility #8235

Merged
merged 5 commits into from
Oct 5, 2024

Conversation

Wirone
Copy link
Member

@Wirone Wirone commented Oct 2, 2024

This PR fixes backward compatibility for php-cs-fixer entrypoint for PHP 5.6 that was unintentionally lost some time ago. Also it introduces CI job for ensuring BC promise is remained (before vs after).

@Wirone Wirone self-assigned this Oct 2, 2024
@coveralls
Copy link

coveralls commented Oct 2, 2024

Coverage Status

coverage: 95.126%. remained the same
when pulling 64ff4b8 on Wirone:codito/lint-main-script
into 46abfa6 on PHP-CS-Fixer:master.

Verified

This commit was signed with the committer’s verified signature.
Wirone Greg Korba
@Wirone Wirone force-pushed the codito/lint-main-script branch from 417fa46 to 59a4d4f Compare October 2, 2024 09:00

Verified

This commit was signed with the committer’s verified signature.
Wirone Greg Korba
@Wirone Wirone marked this pull request as ready for review October 2, 2024 09:06
- name: Setup PHP
uses: ./.github/composite-actions/setup-php
with:
version: 5.6
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version: 5.6
version: 7.0

please don't retrofix this because of https://3v4l.org/7ESuc#v5.6.40 - the #8229 CS fixing would be then harder

the idea is PHP 5.6 is already unsupported (by the entry file) for a long time and the real usage is really super low

Copy link
Member Author

Choose a reason for hiding this comment

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

It's up to @keradus - his will was to support 5.6 and I believe the lost support for it was unintentional. If we want to change BC promise to 7.0 then I am fine with it, but it's not up to me.

Copy link
Contributor

@mvorisek mvorisek Oct 2, 2024

Choose a reason for hiding this comment

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

The PHP 5.6 support was removed 3 years ago - 60a618f - it might be unintentional, but 3 years we did not received any report. Users are simply not using PHP 5.6 anymore and the users who do, they already know "the syntax error behaviour". So IMO the value added returning support after 3 years is negative as it will complicate CI (#8229).

Copy link
Member Author

Choose a reason for hiding this comment

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

@mvorisek I know it and I agree with you, I just said it's not my decision 🙂.

Copy link
Member

Choose a reason for hiding this comment

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

as shared in my original msg (ref), we should align which versions we shall support for entry point.
I'm OK to have 7.0+

Copy link
Member

@keradus keradus left a comment

Choose a reason for hiding this comment

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

OK to merge (as-is or with bump to 7.0)

thanks for introducing that check

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@keradus keradus merged commit ae4a8eb into PHP-CS-Fixer:master Oct 5, 2024
26 of 27 checks passed
@keradus
Copy link
Member

keradus commented Oct 5, 2024

thank you @Wirone

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.

None yet

4 participants