symfony / symfony

Sponsor

Sponsor symfony/symfony
Notifications
Star 25.7k
Fork 8.3k

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.

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

Fix return types for PHP 8.1 #42260

Merged

derrabus merged 1 commit into symfony:4.4 from derrabus:bugfix/return-type-will-change

Aug 4, 2021

+194 −42

Member

derrabus commented Jul 26, 2021

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #42199, Fix #42231, Part of #41552
License	MIT
Doc PR	N/A

derrabus requested review from chalasr, wouterj, xabbuh and yceruto as code owners

carsonbot added Status: Needs Review Bug labels

carsonbot added this to the 4.4 milestone

This was linked to issues Jul 26, 2021

[Console] PHP 8.1 deprecation notice for HelperSet::getIterator() #42199

Closed

[Finder] PHP 8.1 deprecation notices #42231

Closed

derrabus force-pushed the derrabus:bugfix/return-type-will-change branch from 9c45520 to 38d60d1

This was referenced Jul 26, 2021

[Console] PHP 8.1 deprecation notice for HelperSet::getIterator() #42199

Closed

[Finder] PHP 8.1 deprecation notices #42231

Closed

nicolas-grekas reviewed

Member

nicolas-grekas left a comment

Did you use some tool to generate this? It'd be cool to document how we did so that others can do the same I their libs.

...ridge/Doctrine/Tests/Validator/Constraints/UniqueEntityValidatorTest.php Show resolved Hide resolved

Member Author

derrabus commented Jul 26, 2021

Did you use some tool to generate this? It'd be cool to document how we did so that others can do the same I their libs.

No, this was manual work. I used PhpStorm to find all implementations of Countable, IteratorAggregate and friends. For each case I checked individually whether I can add a return type directly or need to add the attribute. I also checked for each occurrence if the @return annotation is equivalent or covariant to the return type in PHP 8.1.

Contributor

sebastianbergmann commented Jul 27, 2021

No, this was manual work.

Would be nice if that did not have to be the case. CC @TomasVotruba

Contributor

TomasVotruba commented Jul 27, 2021

@sebastianbergmann Depends how much time it took to do. If its < 30 minutes, than automation would take longer.

Member Author

derrabus commented Jul 27, 2021

@sebastianbergmann Depends how much time it took to do. If its < 30 minutes, than automation would take longer.

I was definitely faster than 30 min. 😎

Contributor

sebastianbergmann commented Jul 27, 2021

@sebastianbergmann Depends how much time it took to do. If its < 30 minutes, than automation would take longer.
I was definitely faster than 30 min. 😎

Sorry for the noise.

Contributor

TomasVotruba commented Jul 28, 2021

@sebastianbergmann Thanks for the ping. If this would be something that every Symfony user has to change in their projects too, saved times is enourmous.

derrabus force-pushed the derrabus:bugfix/return-type-will-change branch from 38d60d1 to 1e171ca

derrabus mentioned this pull request

Fix IteratorAggregate return type on HelperSet, fatal error with PHP 8.1 #42318

Closed

jrfnl mentioned this pull request

GH Actions: turn on testing against PHP 8.1 box-project/box#559

Merged

This comment has been hidden.

Sign in to view

Member Author

derrabus commented Aug 3, 2021

@wouterj This PR already includes that change. Code reviews welcome. 🙂

wouterj approved these changes

Member

wouterj left a comment

Oh, missed that. Looks good!

src/Symfony/Component/Intl/Data/Util/ArrayAccessibleResourceBundle.php Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels

kylekatarnls approved these changes


          Fix return types for PHP 8.1

3eca446

derrabus force-pushed the derrabus:bugfix/return-type-will-change branch from 1b6624b to 3eca446

Member Author

derrabus commented Aug 4, 2021

@symfony/mergers Merging this PR would remove some noise from our CI. Any objections against it?

Nyholm approved these changes

Member

Nyholm left a comment

Im happy with this PR.

I dont know if this is complete or not. But if we missed some iterator we can fix them in a follow up PR.

OskarStark approved these changes

derrabus merged commit 5b1fc10 into symfony:4.4

9 of 11 checks passed

9 of 11 checks passed

Tests (7.1)

Details

Tests (7.2)

Details

Psalm

Details

Tests (8.0)

Details

Tests (8.0)

Details

Tests (7.4, high-deps) Tests (7.4, high-deps)

Details

Tests (8.0, low-deps)

Details

Tests (8.1, experimental) Tests (8.1, experimental)

Details

fabbot.io Some changes should be done to comply with our standards.

Details

Travis CI - Pull Request Build Passed

Details

continuous-integration/appveyor/pr AppVeyor build succeeded

Details

derrabus deleted the derrabus:bugfix/return-type-will-change branch

Member Author

derrabus commented Aug 4, 2021

Thanks everyone. ❤️

derrabus mentioned this pull request

Fix return types for PHP 8.1 #42379

Merged

Member Author

derrabus commented Aug 4, 2021

Follow-up for the 5.3 branch: #42379

Tobion added a commit that referenced this pull request


          minor #42379 Fix return types for PHP 8.1 (derrabus)

e9b764b

This PR was merged into the 5.3 branch.

Discussion
----------

Fix return types for PHP 8.1

| Q             | A
| ------------- | ---
| Branch?       | 5.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #41552
| License       | MIT
| Doc PR        | N/A

Follow-up of #42260.

Commits
-------

ab3c43f Fix return types for PHP 8.1

wouterj mentioned this pull request

[Console] Support with PHP8.1 #42415

Closed

kubawerlos mentioned this pull request

DX: test on PHP 8.1 FriendsOfPHP/PHP-CS-Fixer#5776

Draft

3 of 4 tasks

julien-boudry mentioned this pull request

Implement PHP 8.1 new features julien-boudry/Condorcet#54

Open

This was referenced Aug 30, 2021

Release v4.4.30 #42789

Merged

Release v5.3.7 #42790

Merged

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