Skip to content

[2.x] Min PHP version 8.1, Symfony 6.4 / 7.1+ #463

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 19 commits into from
Sep 2, 2024
Merged

[2.x] Min PHP version 8.1, Symfony 6.4 / 7.1+ #463

merged 19 commits into from
Sep 2, 2024

Conversation

@andrew-demb
Copy link
Contributor

@andrew-demb andrew-demb commented Aug 25, 2024

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Related tickets fixes #458
Documentation -
License MIT

What's in this PR?

Change the minimum PHP version to 8.1 and Symfony component versions to 6.4 / 7.1+ as requested in #458

Why?

#458

Example Usage

N/A

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix)

To Do

N/A

@andrew-demb
Copy link
Contributor Author

andrew-demb commented Aug 25, 2024

Check code for BC things for old symfony versions.

Didn't find any of them.

@andrew-demb andrew-demb force-pushed the 2.x-increase-min-deps-and-php-version branch from f2efe45 to dc81b99 Compare August 25, 2024 19:25
@andrew-demb
⬆️ [php-http#458] Update min PHP version to 8.1
60d7869
@andrew-demb
📦 Add typehint to parameter in private method
f896933
@andrew-demb
📦 [php-http#458] Update min Symfony version to ^6.4 || ^7.1
3c37a86
@andrew-demb
📦 Hint about always non-null return type
606257f
@andrew-demb
📦 Avoid deprecated DI extension class
053c8c5
@andrew-demb
📦 Fix property type in phpdoc
7ff923b
@andrew-demb
📦 Avoid invalid argument - cast possible float to the integer
8a11bd0
@andrew-demb
📦 Fix property type in phpdoc - there's should be an intersection type
c51791a
@andrew-demb
🐛 Fix property type and return type for parent - can be nullable
82f8822
@andrew-demb
📦 [php-http#458] Introduce property types, use PHP 8.1 features (CPP,…
7d82664 
… short arrow function, readonly properties, etc)
@andrew-demb
📦 [php-http#458] Switch to php-cs-fixer-ga 3.26 and run it
6548642
@andrew-demb
📦 Require phpunit 9 because simple-phpunit is not compatible with p…
0725be7 
…hpunit 10 and `matthiasnoback/symfony-config-test` triggers installation of phpunit 10

From CI
```
PHP Fatal error:  Declaration of PHPUnit\Framework\TestSuite::run(): void must be compatible with PHPUnit\Framework\Test::run(?PHPUnit\Framework\TestResult $result = null): PHPUnit\Framework\TestResult in /home/runner/work/HttplugBundle/HttplugBundle/vendor/phpunit/phpunit/src/Framework/TestSuite.php on line 338
Script vendor/bin/simple-phpunit handling the test event returned with error code 255
```
@andrew-demb andrew-demb force-pushed the 2.x-increase-min-deps-and-php-version branch from dc81b99 to 0725be7 Compare August 25, 2024 19:29
@andrew-demb
Copy link
Contributor Author

andrew-demb commented Aug 25, 2024

I need help with this failed CI check

PHPUnit 9.6.20 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing 
Call to undefined method Http\HttplugBundle\Tests\Unit\ClientFactory\BuzzFactoryTest::getAnnotations()

Remaining indirect deprecation notices (1)
Script vendor/bin/simple-phpunit handling the test event returned with error code 2

I can't find getAnnotations call in the vendor that could lead to such error and cannot reproduce this locally

@andrew-demb andrew-demb mentioned this pull request Aug 25, 2024
Closed
ostrolucky
ostrolucky reviewed Aug 25, 2024
View reviewed changes
dbu
dbu reviewed Sep 1, 2024
View reviewed changes
Copy link
Collaborator

@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.

yay for modern php, looks very nice! thanks for the work!

about the failing lowest version test: we should try with "matthiasnoback/symfony-dependency-injection-test": "^5.1", and if that is not enough add matthiasnoback/symfony-config-test with ^5.2.

i have some minor feedback. do you have time to update these things?

@dbu dbu mentioned this pull request Sep 1, 2024
Merged
@dbu
Copy link
Collaborator

dbu commented Sep 1, 2024

i think we need to add "matthiasnoback/symfony-config-test": ^5.2" to require-dev to fix the lowest build. and change the symfony-dependency-injection-test to only allow ^5.1

@dbu
Copy link
Collaborator

dbu commented Sep 1, 2024

and RE BC hacks: there is one in PluginClientFactoryListener, search the code for Kernel::MAJOR_VERSION to see if there are maybe other places.

@andrew-demb
Copy link
Contributor Author

andrew-demb commented Sep 1, 2024

and RE BC hacks: there is one in PluginClientFactoryListener, search the code for Kernel::MAJOR_VERSION to see if there are maybe other places.

I'm sorry, I don't seem to get it.

I failed to see any compatibility layer with old Symfony versions in PluginClientFactoryListener
And can't find any usage of the Kernel::VERSION* constants usage in the codebase here.

I checked before any usage of deprecated methods from Symfony, method_exists of instanceof for the old versions of Symfony.

@dbu
Copy link
Collaborator

dbu commented Sep 2, 2024

BC hacks

oh, so sorry, that has already been cleaned up in the 2.x branch, i must have looked at 1.x 🤦

thanks a lot for this work, will now merge it 👍

@dbu dbu merged commit c6c246f into php-http:2.x Sep 2, 2024
@andrew-demb andrew-demb deleted the 2.x-increase-min-deps-and-php-version branch September 2, 2024 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbu dbu dbu left review comments

@ostrolucky ostrolucky ostrolucky approved these changes

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

@andrew-demb @dbu @ostrolucky