Skip to content

Update PHP-DI to support PHP 8.4 #899

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 28, 2025
Merged

Update PHP-DI to support PHP 8.4 #899

merged 1 commit into from
Jan 28, 2025

Conversation

andrewnicols
Copy link
Contributor

@andrewnicols andrewnicols commented Jan 28, 2025 •
edited
Loading

PHP 8.4 came out in November.

This commit originally built upon #897 but after further investigation and rebasing, I discovered that the approach in 897 covered more than is necessary so I've created a new approach.

#897 modified the composer.json to use the latest compatible version of PHPUnit (>=9.5) when it is perfectly safe and stable to pin to 9.6 (^9.6) which supports PHP 7.3 through to PHP 8.4.

As a result the changes to stop using willReturnConsecutive() are no longer required (though they will be in the future in some form), and there is no need to make many of the other changes.

This changset instead:

  • modifies the phpunit dependency to ^9.6
  • modifies the laravel/serializable-closure to ^1.0 || ^2.0
  • add a build on PHP 8.4

I believe this is all that is required to make this compatible with PHP 8.0 to PHP 8.4.

@andrewnicols andrewnicols marked this pull request as ready for review January 28, 2025 13:57
@phpfui
Copy link

phpfui commented Jan 28, 2025

@andrewnicols glad to see you stepped up on this PR. I ended up going with a different package which eliminated my need for this fix. I will close the original PR now.

williamdes
williamdes approved these changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Thanks, I will import this diff into Debian then
I originally did think there was more needed for 8.4

@mnapoli
Copy link
Member

mnapoli commented Jan 28, 2025

Awesome thank you! The coding style warnings are unrelated so let's not be blocked by those.

@mnapoli mnapoli merged commit 98ddc81 into PHP-DI:master Jan 28, 2025
7 of 8 checks passed
@williamdes
Copy link
Contributor

I am working on Debian packaging the last version and test_closure_containing_class_name_not_fully_qualified should be marked as not compatible with PHP 8.4

1) DI\Test\IntegrationTest\Definitions\FactoryDefinitionTest::test_closure_containing_class_name_not_fully_qualified with data set "compiled" (DI\ContainerBuilder Object (...))
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'DI\Test\IntegrationTest\Definitions\FactoryDefinitionTestClass'
+'FactoryDefinitionTestClass'

Fix it is you can

@mnapoli
Copy link
Member

mnapoli commented Feb 2, 2025

Weird, the tests passed in CI for PHP 8.4, any idea?

@williamdes
Copy link
Contributor

Weird, the tests passed in CI for PHP 8.4, any idea?

First idea goes to laravel serializable closure still v1 in Debian

@phpfui
Copy link

phpfui commented Feb 2, 2025

The test is probably wrong. You should be able to get FQN from all versions of PHP.

I would submit a PR, but away skiing.

@esetnik
Copy link

esetnik commented Feb 3, 2025

I think this resolves #896 as well

@andrewnicols
Copy link
Contributor Author

Yup @esetnik , it does.

@andrewnicols
Copy link
Contributor Author

Weird, the tests passed in CI for PHP 8.4, any idea?

First idea goes to laravel serializable closure still v1 in Debian

Laravel/Serializable-closure @ v1 is not compatible with PHP 8.4 so how is Debian handling that?

@williamdes
Copy link
Contributor

Weird, the tests passed in CI for PHP 8.4, any idea?

First idea goes to laravel serializable closure still v1 in Debian

Laravel/Serializable-closure @ v1 is not compatible with PHP 8.4 so how is Debian handling that?

I was about to say: patches!
But there is no patch
And this package does not have autopackage tests: https://sources.debian.org/src/php-laravel-serializable-closure/1.3.0-1/debian/

I guess nobody did catch this, I need to run the code with a basic test and report the failure.
Let's see
Most probably it will end up in an upgrade, or backport of the compatibility fixes with patches

@andrewnicols
Copy link
Contributor Author

I don'tthink that 1.3.x is actually incompatible, just that compatibility was not declared and it is not tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@williamdes williamdes williamdes approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@andrewnicols @phpfui @mnapoli @williamdes @esetnik