Skip to content

Conversation

@Chris53897
Copy link
Contributor

@Chris53897 Chris53897 commented Nov 10, 2025

bump checkout action 4 => 6
bump min versions
drop support for PHPUnit < 9
allow PHPUnit > 9
change test filenames to get rid of warnings from PHPUnit
PHP-CS-Fixer: add --diff to show the needed changes
add PhpCsFixer Rule for trailing_comma_in_multiline

@dbu
Copy link
Contributor

dbu commented Nov 10, 2025

uh, that build pipeline (and probably the code) needs some more love apart from php 8.5. do you have time to look at it? i am open to ditch old php versions, people can just keep using 1.1.0 if they are stuck on legacy PHP.

@Chris53897
Copy link
Contributor Author

Step 1: Allow PHPUnit > 9 after php-http/client-integration-tests#61 is merged.

@Chris53897
Copy link
Contributor Author

Which PHP version should it be bumped to? 8.1 ?

@dbu
Copy link
Contributor

dbu commented Nov 11, 2025

Which PHP version should it be bumped to? 8.1 ?

if 8.0 just works we can also keep supporting it for now. maybe you can disable the fail_fast option so we see what is going on. and once the build with newer php versions works again, we can remove all that don't work but keep those that do work.

@Chris53897
Copy link
Contributor Author

Static analysis could not be fixed, because trailing commas are only allowed since PHP 8.0.
The rule needs to be changed. If support for PHP 7.4 is still relevant.

@Chris53897
Copy link
Contributor Author

For the lowest run this could maybe added "guzzlehttp/psr7": "^2.1" (Version?)

@dbu
Copy link
Contributor

dbu commented Dec 9, 2025

  • can you please disable that rule in the cs fixer configuration? don't want to drop php 7.4 support just because of the cs fixer.
  • lowest run looks good to me as is - is there a problem with it as it currently is defined? we should not change the composer config in that test run, if we can avoid it. we want to ensure this package is correctly declaring its lowest version requirements.

@Chris53897
Copy link
Contributor Author

@dbu Ready for review
I managed to get it working with PHP 7.3
No there is not a problem in my eyes with the lowest run.
I was not sure what the goal would be for the lowest supported versions.

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks a lot for the work, great!

is the #[\Override] annotation important for consumers? should i tag a new release or is this "just" to make sure the tests work fine again and test with PHP 8.5?

@dbu dbu merged commit ffeed58 into php-http:1.x Dec 9, 2025
12 checks passed
@Chris53897
Copy link
Contributor Author

Chris53897 commented Dec 9, 2025

I just checked. Psalm wants them to be added. I guess there is no new version needed.
https://github.com/php-http/guzzle7-adapter/actions/runs/20069714472/job/57568003922?pr=20

Error: src/Client.php:51:5: MissingOverrideAttribute: Method Http\Adapter\Guzzle7\Client::sendasyncrequest should have the "Override" attribute (see https://psalm.dev/358)
Error: src/Promise.php:73:5: MissingOverrideAttribute: Method Http\Adapter\Guzzle7\Promise::then should have the "Override" attribute (see https://psalm.dev/358)
Error: src/Promise.php:78:5: MissingOverrideAttribute: Method Http\Adapter\Guzzle7\Promise::getstate should have the "Override" attribute (see https://psalm.dev/358)
Error: src/Promise.php:83:5: MissingOverrideAttribute: Method Http\Adapter\Guzzle7\Promise::wait should have the "Override" attribute (see https://psalm.dev/358)

@Chris53897 Chris53897 deleted the patch-1 branch December 9, 2025 15:57
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.

3 participants