[3.0][BC Break] Require PHP 7.2, added parameter and return types, enable strict types #805
Conversation
…& Persistence\AbstractManagerRegistry to 7.2
…ataFactory} to 7.2
…ice, StaticReflectionService} to 7.2
…SymfonyFileLocator} to 7.2
| * @return int | ||
| */ | ||
| public function compareTo($other); | ||
| public function compareTo(object $other): int; |
There was a problem hiding this comment.
All of these BC breaks need to be documented, together with a migration path :-\
There was a problem hiding this comment.
@Ocramius What do you suggest to do? Would it be ok to simply grep signature changes for now? Ithink creating an extended sheet of changes would be contraproductive ATM, since Common 3.0 might change a lot.
| * @return array The event listeners for the specified event, or all event listeners. | ||
| */ | ||
| public function getListeners($event = null) | ||
| public function getListeners(?string $event = null): array |
There was a problem hiding this comment.
shouldn't this read string $event = null without the ? prefix ?
If the parameter has a default value of null anyway, is the ? not redundant ?
There was a problem hiding this comment.
No. null is a default value, and strictly speaking, it's an invalid default value for the type. These are valid:
string $event=> must be string, must be given in function callstring $event = ''=> must be string, can be omitted in which case an empty string is used?string $event=> must be string or null, must be given in function call?string $event = null=> must be string or null, can be omitted in which casenullis used
string $event = null is invalid, as you'd expect a string but receive null.
There was a problem hiding this comment.
I disagree in saying that string $event = null is invalid because you might receive a null. If that were the case then ?string $event should also be considered invalid because you can also receive a null.
I understand the difference between 3 and 4. And it definitely has an impact on how the method can be called. But between 4 and 5 (string $event = null), I don't see any difference in possible usages. The value might be null in both cases. And the value might be missing in both cases.
I'd even argue that they are not only technically, but also semantically, identical. Doesn't it all boils down to code style preferences ?
If that is the case, and you can express the same technical and semantical strictness, then I'd argue in favor of the most concise syntax: string $event = null
There was a problem hiding this comment.
The difference is that for one, the scalar type hint says "I want a string", while for the other it says "I want a string or null". The "feature" where adding a default type of null makes anything nullable should not overrule a clear scalar type declaration, which in this case is ?string.
There was a problem hiding this comment.
Fair enough, you might have convinced me :)
There was a problem hiding this comment.
The fact that string $event = null works is sort of a flaw of PHP 7.0 where nullable types didn't exist and nullability was implied by default null value.
Anyhow, when I talk about this difference with other people, I usually use this example to show why explicit ? matters (besides consistence):
function foo(?int $bar = null) {} // accepts int or null
function bar(?int $arg = 123) {} // accepts int or nullvs.
function foo(int $arg = null) {} // accepts int or null
function bar(int $arg = 123) {} // oops, accepts only int, no null here!This is even more obvious in inheritance:
class A {
public function foo(int $arg = null) {}
}
class B extends A {
public function foo(int $arg = 123) {} // this is an error, LSP violation
}
// PHP Warning: Declaration of B::foo(int $arg = 123) should be compatible with A::foo(?int $arg = NULL)Rule of thumb: Always use ? to sign whether type is nullable or not. 😉
There was a problem hiding this comment.
You had me at "consistence", but the rest is a very good explanation. Thank you for taking the time to clarify this rule of thumb.
There was a problem hiding this comment.
@Majkl578 great example, I'll be using that from now on. Thanks!
|
Tests re-triggered with 7.2 on Travis, green. |
|
Not applicable anymore, but served its purpose well back then. 😉 |
… default value is null It is more explicit and this discussion [0] from two years ago from the doctrine/repo also points out it can hide a LSP violation. Now seems to be a good time to introduce this rule as we start to aggressively type scalar values so we might encounter the case more often. To test, you can review the code of the sniff [1], apply the new ruleset and allow to autofix this rule then launch `make phpcbf`. The diff should be identical with this contribution. [0] doctrine/common#805 (comment) [1] https://github.com/slevomat/coding-standard/blob/5.0.4/SlevomatCodingStandard/Sniffs/TypeHints/NullableTypeForNullDefaultValueSniff.php Change-Id: Ia7477be1ee75b40e02ccd4d097b9de3fcca64e4c
4 participants
Requires at least PHP 7.2.0alpha3 (due to
objecttype being used).Excluded from migration:
Possible TODOs:
Tests are green. 💚
Travis is failing because it doesn't provide PHP 7.2 version yet - tracked in travis-ci/travis-ci#7989.
Currently targets master, but should definitely go into develop branch (which doesn't exist yet - create one so we can retarget the PR 😎 ).
cc @Ocramius @lcobucci