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

GH Actions: PHP 8.4 has been released #440

Merged

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Nov 22, 2024

Add builds against PHP 8.4.

Ref: https://www.php.net/releases/8.4/en.php

@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 22, 2024

Failing builds on PHP 8.4 look to be related to: shivammathur/setup-php#882

@theseer theseer merged commit 0ac9180 into phar-io:master Nov 22, 2024
7 of 43 checks passed
@theseer
Copy link
Member

theseer commented Nov 22, 2024

Thank you so much!

@theseer
Copy link
Member

theseer commented Nov 22, 2024

And of course i now have to update PHPUnit to make things work. There goes my morning...

@jrfnl jrfnl deleted the feature/ghactions-update-for-php-8.4-release branch November 22, 2024 14:50
@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 22, 2024

@theseer Oh, I'm sorry, I thought PHPUnit 9 was work fine on PHP 8.4 ? Want me to have a look ? At least the setup-php issue should be solved now.

So far, a quick look at https://github.com/phar-io/phive/actions/runs/11968709436/job/33368034308 shows me there are still some implicitely nullable issues to be solved in Phive, more than anything...

The notices from Prophecy can be safely ignored (and don't show when using the Composer installed PHPUnit version). IIRC Prophecy released a PHP 8.4 compatible version a few days ago, so I think all that's needed there is a PHPUnit 9 release (possibly also 8) with updated dependencies (to get a fully compatible PHAR file).

@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 22, 2024

Looking more closely, looks like the issues are nearly all related to Prophecy (double generation).

Running with an updated Prophecy version leaves just one error, which, to me, looks to be coming from the mikey179/vfsstream dependency

1) PharIo\Phive\XmlFileTest::testSavesXmlToFile
PharIo\Phive\PharInstallerException: org\bovigo\vfs\vfsStream::create(): Implicitly marking parameter $baseDir as nullable is deprecated, the explicit nullable type must be used instead

@theseer
Copy link
Member

theseer commented Nov 22, 2024

@theseer Oh, I'm sorry, I thought PHPUnit 9 was work fine on PHP 8.4 ? Want me to have a look ? At least the setup-php issue should be solved now.

Why sorry? It's not your fault ;)

PHPUnit 9 does work fine of course, but the bundled prophecy doesn't. At least not the version I have ;)

And it's totally my fault to not have migrated the tests away from it long ago.

@theseer
Copy link
Member

theseer commented Nov 22, 2024

Looking more closely, looks like the issues are nearly all related to Prophecy (double generation).

Yeah, noticed that as well. Luckily nothing in my/our production code.

Running with an updated Prophecy version leaves just one error,

Problem ist, I want to keep using the phar version of PHPUnit and I do not feel like going through the hassle to, for a limited time until migrated away, installing things via composer to have phpunit-prophecy work...

which, to me, looks to be coming from the mikey179/vfsstream dependency

1) PharIo\Phive\XmlFileTest::testSavesXmlToFile
PharIo\Phive\PharInstallerException: org\bovigo\vfs\vfsStream::create(): Implicitly marking parameter $baseDir as nullable is deprecated, the explicit nullable type must be used instead

I didn't look into that one yet. Is there an update for it already that we just lag?

In general, I'd like to raise minimum versions for the next release: I guess it's time to say goodbye to PHP 7.4 and require 8.1. With that, we could upgrade to PHPUnit 10. That would require a few additional changes though, for instance fix the at() usage in some tests.

Opinions? :)

@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 24, 2024

Running with an updated Prophecy version leaves just one error,

Problem ist, I want to keep using the phar version of PHPUnit and I do not feel like going through the hassle to, for a limited time until migrated away, installing things via composer to have phpunit-prophecy work...

I understand and agree. Considering that PHPUnit previously already released a PHPUnit 9.x version (including new PHAR) with updated dependencies for PHP 8.4, I would kind of expect the same to happen again now Prophecy has finally tagged a PHP 8.4 compatible release.

You are better placed to know whether that is likely to happen or not though. If a new PHPUnit 9.x will happen, then this is just a waiting game until the tag and there's no reason to move away from using the PHAR file.

Keep in mind, Prophecy only tagged that PHP 8.4 compatible release five days ago: https://github.com/phpspec/prophecy/releases/tag/v1.20.0

which, to me, looks to be coming from the mikey179/vfsstream dependency

1) PharIo\Phive\XmlFileTest::testSavesXmlToFile
PharIo\Phive\PharInstallerException: org\bovigo\vfs\vfsStream::create(): Implicitly marking parameter $baseDir as nullable is deprecated, the explicit nullable type must be used instead

I didn't look into that one yet. Is there an update for it already that we just lag?

I just checked. There is. See PR #441.

In general, I'd like to raise minimum versions for the next release: I guess it's time to say goodbye to PHP 7.4 and require 8.1. With that, we could upgrade to PHPUnit 10. That would require a few additional changes though, for instance fix the at() usage in some tests.

Opinions? :)

Well, I'm not your typical user, so take my opinions on this with a grain of salt.

As a general rule of thumb, I'm of the opinion that tooling should generally support a relatively wide range of PHP versions (for all sorts of reasons).

Having said that, I don't think the typical reasons for that opinion apply in this case. I mean, even if PHIVE itself doesn't support PHP < 8.1 anymore, it can still install PHAR files which do have a lower minimum PHP version, so it wouldn't be blocking projects using PHIVE from having a lower minimum supported PHP version.

The only "inconvenience" it would cause would be to devs, which may be running their dev-environment on a lower PHP version. Those would need to temporarily switch PHP versions to run a phive install and then switch back. I don't consider that a huge issue though as - in contrast to Composer - I don't expect PHIVE users to run installs all that often and I also expect most devs to run on more recent PHP versions for their dev environment anyway, even when still supporting older PHP versions.

I'm not sure you have any install/download stats which could help inform your decision ? Then again, even if you do, I kind of expect those wouldn't include info on the PHP version of the user who downloaded PHIVE, so I'm not sure if stats would help in this case.

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.

2 participants