8000 [5.0] set return type declartion to `self` instead of `object` when the type is `$this` by azjezz · Pull Request #33100 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[5.0] set return type declartion to self instead of object when the type is $this #33100

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
Closed
wants to merge 1 commit into from
Closed

[5.0] set return type declartion to self instead of object when the type is $this #33100

wants to merge 1 commit into from

Conversation

azjezz
Copy link
Contributor
@azjezz azjezz commented Aug 9, 2019
Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32179
License MIT

please see : #31996 (comment)

@azjezz
Copy link
Contributor Author
azjezz commented Aug 9, 2019

cc @Tobion

@nicolas-grekas nicolas-grekas added this to the 5.0 milestone Aug 9, 2019
@derrabus
Copy link
Member
derrabus commented Aug 9, 2019

For the record: I‘m not 100% against this, but I‘m unsure, if this is a wise move as long as we need to support php version below 7.4. Using self as return type makes overriding those methods awkward. And moving such a method into parent class while maintaining BC is nearly impossible.

@nicolas-grekas
Copy link
Member

Thank you @azjezz for opening.
I've the same opinion as @derrabus: we need a consistent policy for methods that return static or $this. It cannot be self to me, because as soon as inheritance is involved, you cannot move the signatures around easily anymore, as you need constant care to replace self by the corresponding FQDN, and vice-versa.
Technically, object will prevent us from returning null by mistake. self won't provide anything on top. Or: if you do the kind of mistakes that makes a difference between object and self here, your code is seriously doomed and you'll notice the first time you'll run it.

I'm for rejecting this PR and consistently using object in this situation.

@Tobion
Copy link
Contributor
Tobion commented Aug 10, 2019

Closing then. phpstorm makes use of the @return $this anyway. So it does not matter much to me.

@Tobion Tobion closed this Aug 10, 2019
@azjezz azjezz deleted the object-to-self branch August 10, 2019 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Needs Review
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
5 participants
@azjezz @derrabus @nicolas-grekas @Tobion @carsonbot
0