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

Allow PHP 8.2, drop PHP 7 support #55

Merged
merged 4 commits into from
Dec 20, 2022
Merged

Conversation

IonBazan
Copy link
Contributor

@IonBazan IonBazan commented Nov 4, 2022

Signed-off-by: Ion Bazan [email protected]

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

Description

Allowing PHP 8.2 in composer.json and build matrix.

Tests failing because some php8.2-* extensions are not available in https://launchpad.net/~ondrej/+archive/ubuntu/php/ PPA yet.

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

It looks like CI on 8.2 is unlikely to pass. You can try ignoring platform reqs on 8.2 like this: https://github.com/laminas/laminas-filter/blob/2.24.x/.laminas-ci.json but as the required extensions are missing, it's likely a case of waiting for them to be available in apt.

composer.json Outdated Show resolved Hide resolved
@IonBazan IonBazan force-pushed the patch-1 branch 2 times, most recently from fd79fc5 to b2daff3 Compare November 5, 2022 16:18
@Ocramius
Copy link
Member

Ocramius commented Nov 9, 2022

This should be re-tried: the newer versions of CI include a bunch of 8.2 tooling now.

@Ocramius
Copy link
Member

Hmm, MongoDB missing is a bit of a pain, at this stage: will probably need to wait until after PHP 8.2 is released in Ondrej's PPAs.

@IonBazan IonBazan force-pushed the patch-1 branch 2 times, most recently from 46a344d to 83fff65 Compare December 19, 2022 02:30
@IonBazan IonBazan requested a review from gsteel December 19, 2022 02:36
@IonBazan
Copy link
Contributor Author

@Ocramius tests were failing because DiskFree::$path was set dynamically - solved by adding it there but Psalm is still complaining about some unrelated stuff. I suggest fixing that in a separate PR.

src/Check/DiskFree.php Outdated Show resolved Hide resolved
Signed-off-by: Ion Bazan <[email protected]>
Signed-off-by: Ion Bazan <[email protected]>
@IonBazan IonBazan changed the base branch from 1.18.x to 1.23.x December 19, 2022 03:34
src/Check/DiskFree.php Outdated Show resolved Hide resolved
@IonBazan IonBazan requested review from samsonasik and removed request for gsteel December 19, 2022 05:28
@samsonasik
Copy link
Member

You can fix with run :

composer cs-fix

Psalm need fixing as well https://github.com/laminas/laminas-diagnostics/actions/runs/3728718218/jobs/6323968842#step:4:506

Signed-off-by: Ion Bazan <[email protected]>
@IonBazan
Copy link
Contributor Author

Thanks @samsonasik - forgot to run that before pushing. Regarding Psalm errors, I'll try to fix that in a separate PR to keep the changes scoped.

@IonBazan IonBazan requested review from gsteel and samsonasik and removed request for samsonasik and gsteel December 20, 2022 10:17
@Ocramius
Copy link
Member

The remaining psalm issues are something I fixed in #62, but no time to pursue that further right now.

I suggest re-generating the baseline: please make sure you do so from a compatible environment (docker run --rm -ti -v $(pwd):/app -w /app php:8.0 bash may help)

Signed-off-by: Ion Bazan <[email protected]>
@IonBazan
Copy link
Contributor Author

Thanks for the tips, @Ocramius - turned out I had to pass --set-baseline=psalm-baseline.xml too to add new entries. CI is green now ✅

@Ocramius Ocramius changed the title Allow PHP 8.2 Allow PHP 8.2, drop PHP 7 support Dec 20, 2022
@Ocramius Ocramius added this to the 1.23.0 milestone Dec 20, 2022
@Ocramius Ocramius added Enhancement dependencies Pull requests that update a dependency file labels Dec 20, 2022
@Ocramius Ocramius self-assigned this Dec 20, 2022
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.

LGTM, thanks @IonBazan

@Ocramius Ocramius merged commit ba2d847 into laminas:1.23.x Dec 20, 2022
@Ocramius Ocramius mentioned this pull request Dec 20, 2022
@IonBazan IonBazan deleted the patch-1 branch December 20, 2022 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants