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

[5.0] Add return types in final classes #31996

Merged
merged 1 commit into from Jun 23, 2019

Conversation

@dFayet
Copy link
Contributor

commented Jun 11, 2019

Q A
Branch? master
Bug fix? no
New feature? yes/no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets #31981
License MIT
Doc PR symfony/symfony-docs#...

This is the first step for the issue #31981

I have some questions:

  • I have not added type for methods with @inheritdoc annotation, should I?
  • Don't we want to type also functions without @return annotation? (still in final classes)
  • If yes is the answer of the previous one, do we also want the void return type?
  • I have also added the return type in the DependencyInjection PhpDumper, but is it also wanted? (if yes, I will clean a bit the code changed)
  • Should we update the documentation's code samples when they display final classes?

Todo:

@dFayet dFayet requested review from dunglas, lyrixx and sroze as code owners Jun 11, 2019

@dFayet dFayet force-pushed the dFayet:feature/strict_type_final branch 2 times, most recently from 9d057c9 to eff1a66 Jun 11, 2019

@@ -62,10 +62,7 @@ public function attach(string $file, string $name = null, string $contentType =
}
}
/**
* @return $this

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jun 11, 2019

Member

Acutally, this is not the same.

  • $this means it's exactly the same object
  • self means it's may be another instance of this class

This comment has been minimized.

Copy link
@Tobion

Tobion Jun 11, 2019

Member

yes please add the return type but keep the phpdoc in this case

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 11, 2019

Member

We need to settle on a policy here.
Personally, I feel link using self is more wrong than correct, so using object + the annotation might be more accurate / less misleading.
But that's just my opinion :)

This comment has been minimized.

Copy link
@derrabus

derrabus Jun 11, 2019

Contributor

: self is as close as we can get to @return $this at the moment. However, as long as php does not support covariant return types, using : self here might create awkward situations when overriding such a method or when moving it to a superclass.

This comment has been minimized.

Copy link
@Tobion

Tobion Jun 16, 2019

Member

I think typhinting object for $this is bad. For IDEs that don't support $this, having an object return type means nothing. It does not provide typesafety as would self do in most cases. So for users, self is much better.
The argument was it can provide awkward situation when overriding. But this PR is about final classes which should not be overridden. So again would only affect us maintainers and PHP would complain about it directly in PHP <7.4. So not a problem.
So I'm in favor of using self which is the closest to $this.

@nicolas-grekas nicolas-grekas added this to the 5.0 milestone Jun 11, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

I have not added type for methods with @inheritdoc annotation, should I?

yes please if possible

Don't we want to type also functions without @return annotation? (still in final classes)

yes, when possible too (i.e. only one type is returned)

If yes is the answer of the previous one, do we also want the void return type?

I don't think void provides much benefit so I would suggest to not added, that will save us from some merge conflicts.

I have also added the return type in the DependencyInjection PhpDumper, but is it also wanted?

At least not in this PR to me, see my comment on it.

Should we update the documentation's code samples when they display final classes?

sure

@dFayet dFayet force-pushed the dFayet:feature/strict_type_final branch 3 times, most recently from d6204e8 to e2b9039 Jun 11, 2019

@dFayet dFayet force-pushed the dFayet:feature/strict_type_final branch from e2b9039 to 8654241 Jun 15, 2019

@dFayet dFayet changed the title [WIP][5.0] Replace @return annotation by return type in final classes [5.0] Add return types in final classes Jun 15, 2019

@dFayet dFayet force-pushed the dFayet:feature/strict_type_final branch 2 times, most recently from 28697c9 to 9ba130b Jun 16, 2019

@dFayet dFayet force-pushed the dFayet:feature/strict_type_final branch from 9ba130b to 7eb5ca3 Jun 16, 2019

@dFayet dFayet force-pushed the dFayet:feature/strict_type_final branch from 7eb5ca3 to f84d094 Jun 20, 2019

@dFayet dFayet force-pushed the dFayet:feature/strict_type_final branch from f84d094 to 85e568d Jun 21, 2019

@azjezz

azjezz approved these changes Jun 22, 2019

@dFayet dFayet force-pushed the dFayet:feature/strict_type_final branch from 85e568d to ca5ae19 Jun 22, 2019

@Tobion

Tobion approved these changes Jun 23, 2019

@Tobion

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

Thank you @dFayet for fixing reviews comments so fast

@Tobion Tobion merged commit ca5ae19 into symfony:master Jun 23, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

Tobion added a commit that referenced this pull request Jun 23, 2019

feature #31996 [5.0] Add return types in final classes (dFayet)
This PR was merged into the 5.0-dev branch.

Discussion
----------

[5.0] Add return types in final classes

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes/no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | #31981
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

This is the first step for the issue #31981

I have some questions:

-  ~I have not added type for methods with `@inheritdoc` annotation, should I?~
- ~Don't we want to type also functions without `@return` annotation? (still in `final` classes)~
- ~If yes is the answer of the previous one, do we also want the `void` return type?~
- ~I have also added the return type in the `DependencyInjection` PhpDumper, but is it also wanted? (if yes, I will clean a bit the code changed)~
- ~Should we update the documentation's code samples when they display `final` classes?~

Todo:
- [x] Adjust the PR, following the answers of the questions
- [x] Add return type also when there is no `@return`, or with `@inheritdoc`
- [x] [src/Symfony/Component/Debug/ErrorHandler.php#L383](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Debug/ErrorHandler.php#L383) `@return` annotation is not correct according to the return, investigate and adjust if needed
- [x] [src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php#L50](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php#L50) `@return` annotation is not correct according to the return, investigate and adjust if needed
- [x] Do a PR on documentation to add return type on code snippets with final classes => unneeded as they were already typed

Commits
-------

ca5ae19 Replace @return annotation by return type in final classes

chalasr added a commit that referenced this pull request Jun 25, 2019

minor #32144 add missing return type in User class (Tobion)
This PR was merged into the 5.0-dev branch.

Discussion
----------

add missing return type in User class

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Fixed a missing type in #31996 chasing #17201
Every method in this class had a return type but this method.

Commits
-------

7857f2f add missing return type in User class

@dFayet dFayet deleted the dFayet:feature/strict_type_final branch Jun 25, 2019

fabpot added a commit that referenced this pull request Jun 26, 2019

minor #32143 use proper return types in ErrorHandler and ArgumentReso…
…lver (Tobion)

This PR was merged into the 4.4 branch.

Discussion
----------

use proper return types in ErrorHandler and ArgumentResolver

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | tiny
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets |
| License       | MIT
| Doc PR        |

Found those things while reviewing #31996 which missed some return types due to using `return` instead of `return null`.
It's part of fixing #17201 (due to #10717). See also #30869 that somebody was stumbling over.

Commits
-------

2f9121b use proper return types in ErrorHandler and ArgumentResolver
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.