Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add return types everywhere possible#42496

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedAug 12, 2021•
edited
Loading

QA
Branch?6.0
Bug fix?no
New feature?yes
Deprecations?no
Tickets#40154
LicenseMIT
Doc PR-

This patch is generated byDebugClassLoader with a few manual tweaks when tests failed. These tweaks were isolated in#42490 for 5.4 when applicable.

As of now, tests pass for all components in no-deps mode, but forSecurity/Http,Security/Core andSecurityBundle, mostly because#41613 should be merged first. Tests onSerializer don't pass either yet (help welcome.)

As a first step, I'd like we make these no-deps jobs green.

Then we'll be left with cross versions tests (high-deps/low-deps). Because adding return types is a BC break, I expect these cross versions tests to require us to break compatibility between 5.4 and 6.0 components. I anticipate that in some cases, adding all possible return types will hit the community too hard. That's why I think we should review case-by-case before marking a component as incompatible with another. Instead, we could decide tonot add the corresponding return type. The canonical example of this isCommand::execute(): I'm not sure adding the native return type in 6.0 is worth the cost it will create. We might better wait for 7.0 to add it.

drupol, owenvoke, and xorik reacted with heart emoji
@nicolas-grekasnicolas-grekas added this to the6.0 milestoneAug 12, 2021
@nicolas-grekasnicolas-grekasforce-pushed thereturn-types-all branch 3 times, most recently from5188230 to49f871fCompareAugust 12, 2021 15:54
@nicolas-grekasnicolas-grekasforce-pushed thereturn-types-all branch 2 times, most recently frombb11086 toa95fd2bCompareAugust 12, 2021 17:36
nicolas-grekas added a commit that referenced this pull requestAug 13, 2021
This PR was merged into the 6.0 branch.Discussion----------Add return types - batch 1/n| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       |#40154| License       | MIT| Doc PR        | -Extracted from#42496 for components that shouldn't be extended very often.Commits-------1564887 Add return types - batch 1/n
nicolas-grekas added a commit that referenced this pull requestAug 13, 2021
This PR was merged into the 6.0 branch.Discussion----------Add return types - batch 2/n| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       |#40154| License       | MIT| Doc PR        | -Extracted from#42496 for components that shouldn't be extended very often.Commits-------606775c Add return types - batch 2/n
nicolas-grekas added a commit that referenced this pull requestAug 13, 2021
This PR was merged into the 6.0 branch.Discussion----------Add return types - batch 3/n| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       |#40154| License       | MIT| Doc PR        | -Extracted from#42496 for components that shouldn't be extended very often.Commits-------aef9069 Add return types - batch 3/n
nicolas-grekas added a commit that referenced this pull requestAug 13, 2021
This PR was merged into the 6.0 branch.Discussion----------Add return types - batch 4/n| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       |#40154| License       | MIT| Doc PR        | -Extracted from#42496Not all possible return types are patched for the attached components, to save breaking BC cross-components, for now at least.Commits-------bad9b7d Add return types - batch 4/n
nicolas-grekas added a commit that referenced this pull requestAug 13, 2021
This PR was merged into the 6.0 branch.Discussion----------Add return types - batch 5/n| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       |#40154| License       | MIT| Doc PR        | -Extracted from#42496Not all possible return types are patched for the attached components, to save breaking BC cross-components, for now at least.Commits-------00f08e4 Add return types - batch 5/n
nicolas-grekas added a commit that referenced this pull requestAug 16, 2021
This PR was merged into the 6.0 branch.Discussion----------Add return types - batch 6/n| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       |#40154| License       | MIT| Doc PR        | -Extracted from#42496Not all possible return types are patched for the attached components, to save breaking BC cross-components, for now at least.Commits-------df6b42e Add return types - batch 6/n
@nicolas-grekasnicolas-grekasforce-pushed thereturn-types-all branch 5 times, most recently from2069380 to6536675CompareAugust 17, 2021 16:51
nicolas-grekas added a commit that referenced this pull requestAug 19, 2021
This PR was merged into the 6.0 branch.Discussion----------[Serializer] add return types| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       |#40154| License       | MIT| Doc PR        | -Extracted from#42496Not all possible return types are patched for the attached components, to save breaking BC cross-components, for now at least.Serializer's test fails. Help wanted 🙏Commits-------b2090d0 [Serializer] add return types
nicolas-grekas added a commit that referenced this pull requestAug 20, 2021
This PR was merged into the 6.0 branch.Discussion----------[Security] add return types| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       |#40154| License       | MIT| Doc PR        | -Extracted from#42496Not all possible return types are patched for the attached components, to save breaking BC cross-components, for now at least.Commits-------8871327 [Security] add return types
@nicolas-grekasnicolas-grekasforce-pushed thereturn-types-all branch 3 times, most recently from4a1506b tod7fbc2cCompareAugust 20, 2021 12:41
@nicolas-grekas
Copy link
MemberAuthor

This PR is the last remaining one on the return type topic \o/

Unless I made a mistake, adding any of the return types here will break a cross-versions dependency between Symfony components.

I'm going to need help to decide, case by case, one of those:

  • add the native return type on 5.4 when possible, if possible (eg a final class that we missed, etc)
  • skip adding the return type and postpone to 7.0
  • add the return type and accept the BC break.

Help wanted.

@nicolas-grekasnicolas-grekasforce-pushed thereturn-types-all branch 2 times, most recently fromf4ae43d toea96144CompareAugust 24, 2021 19:48
@nicolas-grekasnicolas-grekasforce-pushed thereturn-types-all branch 2 times, most recently from78cc0f5 to19f28cbCompareAugust 25, 2021 16:02
nicolas-grekas added a commit that referenced this pull requestAug 25, 2021
…o native types (nicolas-grekas)This PR was merged into the 6.0 branch.Discussion----------[CI] Ensure that all possible ``@return`` are turned into native types| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       |#40154| License       | MIT| Doc PR        | -This PR is the final step to close the topic of return types for Symfony 6.It replaces#42496 by *not* adding return types to the classes/interfaces that break cross-component deps.It enforces that all ``@return`` from 5.4 are turned into native types in 6.0, with the known list of exceptions from#42496.Commits-------2b5cd7a [CI] Ensure that all possible ``@return`` are turned into native types
@nicolas-grekas
Copy link
MemberAuthor

Continued in#42735

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

Reviewers

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

@jderussejderusseAwaiting requested review from jderusse

@lyrixxlyrixxAwaiting requested review from lyrixx

@OskarStarkOskarStarkAwaiting requested review from OskarStark

@srozesrozeAwaiting requested review from sroze

@wouterjwouterjAwaiting requested review from wouterjwouterj is a code owner

@xabbuhxabbuhAwaiting requested review from xabbuhxabbuh is a code owner

@ycerutoycerutoAwaiting requested review from ycerutoyceruto is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

6.0

Development

Successfully merging this pull request may close these issues.

2 participants

@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp