Skip to content
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
merged 1 commit into from Aug 4, 2021
Merged

Fix return types for PHP 8.1 #42260

merged 1 commit into from Aug 4, 2021

Conversation

derrabus
Copy link
Member

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

@carsonbot carsonbot added this to the 4.4 milestone Jul 26, 2021
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 derrabus force-pushed the bugfix/return-type-will-change branch from 9c45520 to 38d60d1 Compare July 26, 2021 13:43
This was referenced Jul 26, 2021
Closed
Closed
nicolas-grekas
nicolas-grekas reviewed Jul 26, 2021
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

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.

src/Symfony/Bridge/Doctrine/Tests/Validator/Constraints/UniqueEntityValidatorTest.php Show resolved Hide resolved
@derrabus
Copy link
Member Author

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.

@sebastianbergmann
Copy link
Contributor

No, this was manual work.

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

@TomasVotruba
Copy link
Contributor

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

@derrabus
Copy link
Member Author

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

@sebastianbergmann
Copy link
Contributor

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

@TomasVotruba
Copy link
Contributor

@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 derrabus force-pushed the bugfix/return-type-will-change branch from 38d60d1 to 1e171ca Compare July 28, 2021 16:11
@derrabus derrabus mentioned this pull request Jul 29, 2021
Closed
@jrfnl jrfnl mentioned this pull request Aug 2, 2021
Merged
@wouterj

This comment has been minimized.

Sign in to view
@derrabus
Copy link
Member Author

derrabus commented Aug 3, 2021

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

wouterj
wouterj approved these changes Aug 3, 2021
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Oh, missed that. Looks good!

src/Symfony/Component/Intl/Data/Util/ArrayAccessibleResourceBundle.php Show resolved Hide resolved
@derrabus derrabus force-pushed the bugfix/return-type-will-change branch from 1b6624b to 3eca446 Compare August 4, 2021 20:31
@derrabus
Copy link
Member Author

derrabus commented Aug 4, 2021

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

Nyholm
Nyholm approved these changes Aug 4, 2021
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

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.

@derrabus derrabus merged commit 5b1fc10 into symfony:4.4 Aug 4, 2021
@derrabus derrabus deleted the bugfix/return-type-will-change branch August 4, 2021 21:31
@derrabus
Copy link
Member Author

derrabus commented Aug 4, 2021

Thanks everyone. ❤️

@derrabus derrabus mentioned this pull request Aug 4, 2021
Merged
@derrabus
Copy link
Member Author

derrabus commented Aug 4, 2021

Follow-up for the 5.3 branch: #42379

Tobion added a commit that referenced this pull request Aug 4, 2021
@Tobion
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 wouterj mentioned this pull request Aug 6, 2021
Closed
@kubawerlos kubawerlos mentioned this pull request Aug 8, 2021
Merged
4 tasks
@julien-boudry julien-boudry mentioned this pull request Aug 25, 2021
Closed
13 tasks
This was referenced Aug 30, 2021
Merged
Merged
@froschdesign froschdesign mentioned this pull request Oct 5, 2021
Merged
SpacePossum added a commit to PHP-CS-Fixer/PHP-CS-Fixer that referenced this pull request Dec 1, 2021
@SpacePossum
feature #5776 DX: test on PHP 8.1 (kubawerlos)
075082e 
This PR was merged into the master branch.

Discussion
----------

DX: test on PHP 8.1

Changes in `composer.json` must be reverted, this require:

- [x] [justinrainbow/json-schema](https://github.com/justinrainbow/json-schema) to be released with the fix: justinrainbow/json-schema#666
- [x] [mikey179/vfsstream](https://github.com/bovigo/vfsStream) new v1 version to be released
- [x] [mikey179/vfsstream](https://github.com/bovigo/vfsStream) another version (with bovigo/vfsStream#261) to be released
- [x] [symfony/symfony](https://github.com/symfony/symfony) new version (with symfony/symfony#42260) to be released

Commits
-------

a4358ec Test on PHP 8.1
@jakzal jakzal mentioned this pull request Dec 21, 2021
Merged
@v4ssi404 v4ssi404 mentioned this pull request Mar 21, 2022
Closed
@dmohns dmohns mentioned this pull request Apr 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@wouterj wouterj wouterj approved these changes

@OskarStark OskarStark OskarStark approved these changes

@Nyholm Nyholm Nyholm approved these changes

@kylekatarnls kylekatarnls kylekatarnls approved these changes

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

@xabbuh xabbuh Awaiting requested review from xabbuh xabbuh is a code owner

@yceruto yceruto Awaiting requested review from yceruto yceruto is a code owner

Assignees
No one assigned
Labels
Bug Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

[Finder] PHP 8.1 deprecation notices [Console] PHP 8.1 deprecation notice for HelperSet::getIterator()
9 participants
@derrabus @sebastianbergmann @TomasVotruba @wouterj @nicolas-grekas @OskarStark @Nyholm @kylekatarnls @carsonbot