[3.x] refactor: Add more BC types to method returns and private properties#1779
[3.x] refactor: Add more BC types to method returns and private properties#1779
Conversation
Rules applied: - ReturnNullableTypeRector - FlipTypeControlToUseExclusiveTypeRector
Rules applied: - ReadOnlyPropertyRector - TypedPropertyFromStrictConstructorRector - ReturnTypeFromStrictTypedPropertyRector - StringReturnTypeFromStrictStringReturnsRector
Rules applied: - ReadOnlyPropertyRector - TypedPropertyFromStrictConstructorRector
Rules applied: - ReturnTypeFromStrictNativeCallRector - ReturnTypeFromStrictNewArrayRector
Since it was introduced in e16301e in 2014, ContextFactory has had a strict `Context $context` typehint on the argument to `initializeInstance`. This means that although theoretically it could have taken any `$class` name to `createContext`, and returned any object from `createInstance`, in practice this would have failed on the type of argument passed to `initializeInstance`. Therefore, it is not a BC break to be strict that the original `$class` arg passed into `createContext` must be a `class-string<Context>` and the return of `createInstance` must be `Context`.
Rules applied: - ReturnTypeFromStrictTypedCallRector
And use first-class over string callable.
Rector gave `askQuestion` a mixed return type, because the symfony method has a mixed return type. However, the actual return type depends entirely on the question you pass in (for example, if you register validators or normalizers to cast the answer, or provide choices that are not strings). If you pass in a list of string choices (as we do) then you will always receive a string answer. We can therefore be strict about this return type, which allows us to also be strict about the return of the public method.
Because this branch removes more redundant phpdoc, so the line numbers in the expected output are now incorrect relative to the source files.
| } | ||
|
|
||
| public function getException() | ||
| public function getException(): ?Exception |
There was a problem hiding this comment.
should this be Exception or Traversable ? How are we handling Error in Behat ?
There was a problem hiding this comment.
Do you mean Throwable?
There was a problem hiding this comment.
#833 introduced support for Throwable by wrapping it in an Exception (to avoid a BC break on all the things that are typed as taking / returning Exception).
So this is correct for 3.x - and I think probably 4.x, we could consider a change in future but I'm not sure it's worth the BC break for now.
|
@acoulton As with your previous PR for adding types, please let me review this one carefully. I don't think I will be able to check it today, will try to do it tomorrow |
@carlos-granados no problem at all. |
carlos-granados
left a comment
carlos-granados
left a comment
There was a problem hiding this comment.
Finally found the time to look into this (crazy Xmas....). I looked into all the individual commits, thanks for splitting them up. Everything looks fine
|
Thanks very much @carlos-granados (and feliz año nuevo!) - no worries, the holidays were pretty busy for me too. I won't have much time the next few days but hopefully I'll get a chance to work on the next iteration at some point next week. |
3 participants
This is a second pass at adding backwards-compatible return types that can be safely merged into the 3.x series, as work towards #1744
As previously, I used Rector to apply type coverage rules (up to level 32) and manually reviewed / filtered the commits to exclude changes that would have been a BC break. I also manually improved a couple of signatures where we know more about the return type than Rector was able to detect.
As part of this, Rector identified that some private properties are always initialised from the constructor with a strict-typed parameter. It was therefore able to add property typehints (and in some cases
readonly, unless the property can also be assigned another way). Some of these are in non-final classes, but as the property is private there's still no BC issue here.It looks like this PR gets us about as far as Rector can go with return types at the moment - level 33 starts to introduce strict typing to method parameters (starting with private methods). I'll look at that in a separate PR.