[5.0] Add return types in final classes #31996

dFayet · 2019-06-11T14:16:44Z

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:

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

lyrixx · 2019-06-11T14:25:57Z

src/Symfony/Bridge/Twig/Mime/WrappedTemplatedEmail.php

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

-    /**
-     * @return $this


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

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

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 :)

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

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.

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

src/Symfony/Component/Debug/Tests/Fixtures/FinalMethod.php

nicolas-grekas · 2019-06-11T15:29:20Z

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

...ony/Component/Form/Extension/Core/DataTransformer/DateTimeImmutableToDateTimeTransformer.php

src/Symfony/Component/HttpKernel/Bundle/Bundle.php

src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php

src/Symfony/Component/Workflow/TransitionBlockerList.php

src/Symfony/Component/Security/Core/User/User.php

src/Symfony/Component/Config/Resource/ClassExistenceResource.php

src/Symfony/Component/Config/Resource/GlobResource.php

src/Symfony/Component/Workflow/TransitionBlockerList.php

src/Symfony/Component/Config/Resource/GlobResource.php

Tobion · 2019-06-23T22:18:37Z

Thank you @dFayet for fixing reviews comments so fast

@return

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  | 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: - [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

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  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  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

…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  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | 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

@dFayet

This PR was merged into the 5.0-dev branch. Discussion ---------- [Lock] Fix return typehint | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | NA | License | MIT | Doc PR | NA Return typehint added in #31996 ping @dFayet Commits ------- e28ffe8 Fix return typehint in Lock

@return

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  | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | symfony#31981 | License | MIT | Doc PR | symfony/symfony-docs#...  This is the first step for the issue symfony#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

dFayet requested review from dunglas, lyrixx and sroze as code owners June 11, 2019 14:16

carsonbot added Status: Needs Review Feature labels Jun 11, 2019

dFayet force-pushed the feature/strict_type_final branch 2 times, most recently from 9d057c9 to eff1a66 Compare June 11, 2019 14:24

lyrixx reviewed Jun 11, 2019

View reviewed changes

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

nicolas-grekas reviewed Jun 11, 2019

View reviewed changes

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Jun 11, 2019

View reviewed changes

src/Symfony/Component/Debug/Tests/Fixtures/FinalMethod.php Outdated Show resolved Hide resolved

derrabus reviewed Jun 11, 2019

View reviewed changes

...ony/Component/Form/Extension/Core/DataTransformer/DateTimeImmutableToDateTimeTransformer.php Show resolved Hide resolved

derrabus reviewed Jun 11, 2019

View reviewed changes

src/Symfony/Component/HttpKernel/Bundle/Bundle.php Outdated Show resolved Hide resolved

derrabus reviewed Jun 11, 2019

View reviewed changes

src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php Outdated Show resolved Hide resolved

derrabus reviewed Jun 11, 2019

View reviewed changes

src/Symfony/Component/Workflow/TransitionBlockerList.php Outdated Show resolved Hide resolved

dFayet force-pushed the feature/strict_type_final branch 3 times, most recently from d6204e8 to e2b9039 Compare June 12, 2019 07:21

dFayet commented Jun 12, 2019

View reviewed changes

src/Symfony/Component/Security/Core/User/User.php Show resolved Hide resolved

fancyweb mentioned this pull request Jun 13, 2019

[ErrorCatcher][ExceptionHandler] Handle \Throwable #31694

Closed

dFayet force-pushed the feature/strict_type_final branch from e2b9039 to 8654241 Compare June 15, 2019 06:42

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

nicolas-grekas approved these changes Jun 15, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 15, 2019

Tobion reviewed Jun 16, 2019

View reviewed changes

src/Symfony/Component/Config/Resource/ClassExistenceResource.php Show resolved Hide resolved

dFayet force-pushed the feature/strict_type_final branch 2 times, most recently from 28697c9 to 9ba130b Compare June 16, 2019 09:42

dFayet force-pushed the feature/strict_type_final branch from 9ba130b to 7eb5ca3 Compare June 16, 2019 10:29

Tobion reviewed Jun 17, 2019

View reviewed changes

src/Symfony/Component/Config/Resource/GlobResource.php Outdated Show resolved Hide resolved

dFayet force-pushed the feature/strict_type_final branch from 7eb5ca3 to f84d094 Compare June 20, 2019 14:43

Tobion reviewed Jun 21, 2019

View reviewed changes

src/Symfony/Component/Workflow/TransitionBlockerList.php Outdated Show resolved Hide resolved

dFayet force-pushed the feature/strict_type_final branch from f84d094 to 85e568d Compare June 21, 2019 07:50

Tobion reviewed Jun 22, 2019

View reviewed changes

src/Symfony/Component/Config/Resource/GlobResource.php Outdated Show resolved Hide resolved

azjezz approved these changes Jun 22, 2019

View reviewed changes

Replace @return annotation by return type in final classes

ca5ae19

dFayet force-pushed the feature/strict_type_final branch from 85e568d to ca5ae19 Compare June 22, 2019 22:57

Tobion approved these changes Jun 23, 2019

View reviewed changes

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

This was referenced Jun 24, 2019

use proper return types in ErrorHandler and ArgumentResolver #32143

Merged

add missing return type in User class #32144

Merged

jlslew mentioned this pull request Jun 24, 2019

Return value of Symfony\Component\Debug\ExceptionHandler::setFileLinkFormat() must be of the type string, null returned #32150

Closed

dFayet deleted the feature/strict_type_final branch June 25, 2019 08:58

jderusse mentioned this pull request Aug 8, 2019

[Lock] Fix return typehint #33035

Merged

azjezz mentioned this pull request Aug 9, 2019

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

Closed

fabpot mentioned this pull request Nov 12, 2019

Release v5.0.0-BETA1 #34339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5.0] Add return types in final classes #31996

[5.0] Add return types in final classes #31996

dFayet commented Jun 11, 2019 •

edited

Loading

lyrixx Jun 11, 2019

Tobion Jun 11, 2019 •

edited

Loading

nicolas-grekas Jun 11, 2019 •

edited

Loading

derrabus Jun 11, 2019

Tobion Jun 16, 2019

nicolas-grekas commented Jun 11, 2019

Tobion commented Jun 23, 2019 •

edited

Loading

[5.0] Add return types in final classes #31996

[5.0] Add return types in final classes #31996

Conversation

dFayet commented Jun 11, 2019 • edited Loading

lyrixx Jun 11, 2019

Choose a reason for hiding this comment

Tobion Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

derrabus Jun 11, 2019

Choose a reason for hiding this comment

Tobion Jun 16, 2019

Choose a reason for hiding this comment

nicolas-grekas commented Jun 11, 2019

Tobion commented Jun 23, 2019 • edited Loading

dFayet commented Jun 11, 2019 •

edited

Loading

Tobion Jun 11, 2019 •

edited

Loading

nicolas-grekas Jun 11, 2019 •

edited

Loading

Tobion commented Jun 23, 2019 •

edited

Loading