Skip to content

Update PHP-DI to support PHP 8.4#899

Merged
mnapoli merged 1 commit intoPHP-DI:masterfrom
andrewnicols:UpdateComposer
Jan 28, 2025
Merged

Update PHP-DI to support PHP 8.4#899
mnapoli merged 1 commit intoPHP-DI:masterfrom
andrewnicols:UpdateComposer

Conversation

@andrewnicols
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
@williamdes
Copy link
Copy Markdown
Contributor

williamdes commented Feb 2, 2025

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
Copy Markdown
Member

mnapoli commented Feb 2, 2025

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

@williamdes
Copy link
Copy Markdown
Contributor

williamdes commented Feb 2, 2025

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
Copy Markdown

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
Copy Markdown

esetnik commented Feb 3, 2025

I think this resolves #896 as well

@andrewnicols
Copy link
Copy Markdown
Contributor Author

andrewnicols commented Feb 4, 2025

Yup @esetnik , it does.

@andrewnicols
Copy link
Copy Markdown
Contributor Author

andrewnicols commented Feb 4, 2025

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
Copy Markdown
Contributor

williamdes commented Feb 4, 2025

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
Copy Markdown
Contributor Author

andrewnicols commented Feb 4, 2025

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