Skip to content

Update composer and tests to PHP 8.4 #897

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
Closed
wants to merge 2 commits into from
Closed

Update composer and tests to PHP 8.4 #897

wants to merge 2 commits into from

Conversation

phpfui
Copy link

@phpfui phpfui commented Dec 11, 2024

I updated to the latest dependencies. Also up updated for PHP 8.4 nullable parameter depreciation fix.

The unit tests on the master branch had one failure originally and I did not fix that.

Also upgrading PHPUnit removed the withConsecutive() method. I used Rector to fix, but that introduced another failure. Since I don't know (or use directly) this library, I am not sure how to fix this test or what you are actually trying to test. So you should take a look, probably an easy fix if you know what you want.

mnapoli
mnapoli reviewed Dec 11, 2024
View reviewed changes
Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

The build is failing.

composer.json
},
"require-dev": {
"phpunit/phpunit": "^9.5",
"phpunit/phpunit": ">=9.5",
Copy link
Member

Choose a reason for hiding this comment

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

We need to use ^

Copy link
Author

Choose a reason for hiding this comment

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

I can specify each version if you want, but it is good to be on the latest PHPUnit

Copy link
Author

Choose a reason for hiding this comment

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

I think I only tested on PHP 8.4, will get older versions working. Generally I drop unsupported PHP versions, but I should be able to make it all work. The build will still fail for PHP 8.4, because it is currently failing on master for PHP 8.4, not sure how to fix.

Copy link
Member

Choose a reason for hiding this comment

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

Unbound versions mean the build is not reproducible and may break as soon as there's a new major version of the dependency.

tests/IntegrationTest/Definitions/AttributeTest.php
@@ -138,22 +138,22 @@ class ConstructorInjection
{
public $value;
public string $scalarValue;
public stdClass $typedValue;
public ?stdClass $typedOptionalValue;
public \stdClass $typedValue;
Copy link
Member

Choose a reason for hiding this comment

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

No code style changes please.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure what you mean here. \stdClass to just stdClass? \stdClass is sometimes uses, sometimes not in master. Best practice is FQN, so \stdClass everywhere, but I only added where I changed, often to match parameters.

But happy to change.

Copy link
Member

Choose a reason for hiding this comment

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

Let's not change lines unrelated to the PR.

Also I wouldn't call this best practice as they add unnecessary noise to the code. The performance benefit makes sense in very specific cases (and a test is not one of those).

@phpfui
Copy link
Author

phpfui commented Dec 20, 2024

Finally getting back to this.

So the current master branch has the following PHPUnit error on all supported versions of PHP:

There was 1 failure:

1) DI\Test\IntegrationTest\ContainerDebugTest::testEntriesDefinitions
Failed asserting that 'Object (\r\n
    class = DI\Container\r\n
    lazy = false\r\n
    __construct(\r\n
        $definitions = (default value) array (\n
)\r\n
        $proxyFactory = (default value) NULL\r\n
        $wrapperContainer = (default value) NULL\r\n
    )\r\n
)' matches PCRE pattern "/^Object \(\n {4}class = DI\\Container\n/".

C:\websites\PHP-DI\tests\IntegrationTest\ContainerDebugTest.php:80

No idea how to fix this.

Also vimeo/psalm does not yet support 8.4, so that causes PHPUnit only support V9. But PHP 8.4 lists all sorts of depreciation issues, which is why I originally opened this PR. There is an open PR vimeo/psalm for this.

Not sure how you want to proceed.

@zonuexe zonuexe mentioned this pull request Jan 7, 2025
Merged
@andrewnicols
Copy link
Contributor

Is there anything we can do to help move this one along so that we can support PHP 8.4?

Cheers

@mnapoli
Copy link
Member

mnapoli commented Jan 28, 2025

@andrewnicols yes there is, you can pick up this PR and open a new one, the goal is to get the build green

@andrewnicols andrewnicols mentioned this pull request Jan 28, 2025
Merged
andrewnicols added a commit to andrewnicols/PHP-DI that referenced this pull request Jan 28, 2025
@andrewnicols
Include support for PHP 8.4
9ab43f0 
Fixes PHP-DI#897
@phpfui
Copy link
Author

phpfui commented Jan 28, 2025

This is no longer needed as @andrewnicols submitted an updated PR. Closing.

@phpfui phpfui closed this Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnapoli mnapoli mnapoli left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@phpfui @andrewnicols @mnapoli