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

PHP 8.3 support added #15

Merged
merged 3 commits into from
Nov 21, 2023
Merged

PHP 8.3 support added #15

merged 3 commits into from
Nov 21, 2023

Conversation

glo71317
Copy link
Contributor

Q A
Documentation yes/no
Bugfix yes/no
BC Break yes/no
New Feature yes/no
RFC yes/no
QA yes/no

Description

@samsonasik
Copy link
Member

laminas-file should no longer be supported imo

@froschdesign
Copy link
Member

@samsonasik
When someone provides a pull request and their help, we merge it - just like before.

@glo71317 glo71317 force-pushed the php8.3_support branch 2 times, most recently from 19d086c to 170201d Compare November 8, 2023 09:00
@glo71317
Copy link
Contributor Author

@froschdesign Can you take a look the PR if all looks fine to you please proceed for merging?

phpcs.xml Outdated Show resolved Hide resolved
@weierophinney
Copy link
Member

My recommendation is to not do any CS changes in this PR, and we can open a new PR to do CS changes later.

If the proposed changes fail the CS rules already present in this PR, then they need to follow and use the existing rules present here to keep it focused.

@gsteel
Copy link
Member

gsteel commented Nov 16, 2023

@glo71317
If you can avoid upgrading laminas-coding-standard in this patch - i.e. if ~1.0.0 is installable on 8.1 through 8.3, then reverting those changes would be the easiest course of action.

If this is not possible, we'll need to upgrade laminas-coding-standard to ~2.5.0 in a separate patch and fix all of the coding standard violations before attempting to add support for PHP 8.3

@Ocramius
Copy link
Member

@gsteel what about removing the dependency, and re-introducing it in a new PR later?

@gsteel
Copy link
Member

gsteel commented Nov 16, 2023

@Ocramius - I guess removing the dep here would also work, but as @weierophinney suggested that the patch should pass existing CS rules, it would make sense to just keep with version 1.0 here - CS wasn't failing - it's the schema validation that's failing - CS was running fine on 8.0 as far as I could tell, so it's less work to just stay on 1.0 right?

@glo71317
Copy link
Contributor Author

glo71317 commented Nov 17, 2023

@Ocramius @gsteel I have updated laminas-coding-standard:"~1.0.0" instead of "~2.5.0", Now PHPCodeSniffer checks are failing. earlier was also same failure then i decided to update laminas-coding-standard:"~2.5.0". So, Can you please suggest?

@glo71317
Copy link
Contributor Author

@Ocramius @gsteel As you suggested to avoid to suppress the CS Changes, I have updated now Can you please guide us how to proceed on this PR?

@gsteel
Copy link
Member

gsteel commented Nov 20, 2023

@Ocramius and @weierophinney

I've just looked into this - it's not possible to run phpcs@2.7 on PHP 8.0 anyway, leave alone 8.1.

Last year, almost to the day #13 was merged without CS running either.

Given that tests are passing, do you think we can merge this as-is, and defer CS tooling upgrades for another year, or perhaps discuss continued maintenance of this component in the next TSC meet?

@froschdesign
Copy link
Member

@localheinz

Given that tests are passing, do you think we can merge this as-is, and defer CS tooling upgrades for another year, or perhaps discuss continued maintenance of this component in the next TSC meet?

You are a maintainer for this package; what are your thoughts?

@Ocramius Ocramius self-assigned this Nov 21, 2023
@Ocramius Ocramius added this to the 2.13.0 milestone Nov 21, 2023
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Even with red CI CS checks, I'm happy with this, as CS is not as relevant here, IMO.

Will squash-merge, as a warning to PR authors.

Thanks @glo71317!

@Ocramius Ocramius merged commit 54b354b into laminas:2.13.x Nov 21, 2023
8 of 11 checks passed
@localheinz
Copy link
Member

@froschdesign

Apologies for the delay, I’m traveling in Canada right now and only have my phone with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants