Contributor

Correct me if I'm wrong, but I don't think the tests currently check for the interaction between ? and = null in inheritance?

Contributor Author

Indeed, I haven't added tests for this part. Will look into it later.

Member

The patch misses inherited method compatibility rules described in RFC.
You'll probably need just uncomment corresponding block in zend_inheritance.c.
RFC should mention a corresponding BC break.

Contributor Author

I have spot checked method compatibility and didn't see any issue with the patch. Can you provide examples for both situations, @dstogov?

Member

See the second example from https://wiki.php.net/rfc/nullable_types#parameters

From: Levi Morrison notifications@github.com
Sent: Friday, April 29, 2016 2:07:05 AM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Nullable types (#1893)

I have spot checked method compatibility and didn't see any issue with the patch. Can you provide examples for both situations, @dstogovhttps://github.com/dstogov?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1893#issuecomment-215588431

Contributor Author

I see an issue with the formatting in the error string but the behavior looks to be correct. Where is the backwards compatibility break?

Member

You propose to disable overriding nullable with non-nullable, so the following code (from your email) won't work any more (it works in 7.0).

From: Levi Morrison notifications@github.com
Sent: Friday, April 29, 2016 3:23:37 AM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Nullable types (#1893)

I see an issue with the formatting in the error string but the behavior looks to be correct. Where is the backwards compatibility break?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1893#issuecomment-215600292

Member

Oh, I see - Bob just silently reintroduced the BC break bypassing the RFC [😊]

I reverted this. This break is actually a part of your RFC.

From: Dmitry Stogov
Sent: Friday, April 29, 2016 11:14:10 AM
To: php/php-src; php/php-src
Cc: Mention
Subject: Re: [php/php-src] Nullable types (#1893)

You propose to disable overriding nullable with non-nullable, so the following code (from your email) won't work any more (it works in 7.0).

From: Levi Morrison notifications@github.com
Sent: Friday, April 29, 2016 3:23:37 AM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Nullable types (#1893)

I see an issue with the formatting in the error string but the behavior looks to be correct. Where is the backwards compatibility break?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1893#issuecomment-215600292

Contributor

I'm surprised that example works in 7.0 at all, it seems like an LSP violation.

Contributor Author

Dmitry,

The following code fails with an error since PHP 5.3+ ^{(https://3v4l.org/kRKUN)}:

interface Fooable {
    function foo(Fooable $f = null);
}
interface StrictFoo extends Fooable {
    function foo(Fooable $f);
}

Thus this is not a BC break:

interface Fooable {
    function foo(?Fooable $f);
}
interface StrictFoo extends Fooable {
    function foo(Fooable $f);
}

The BC break you are thinking of has to do with changing the default parameter in this subclass so it isn't null anymore. That has to do with default parameters, not nullable parameters, and is therefore not part of this RFC.

Does that make sense?

Member

Sorry, I'm far from computers till Wednesday. The example I sent you works on PHP 7.0.5.

-------- Original Message --------
From:Levi Morrison
Sent:Fri, 29 Apr 2016 17:34:01 +0300
To:php/php-src
Cc:Dmitry Stogov ,Mention
Subject:Re: [php/php-src] Nullable types (#1893)

Dmitry,

The following code fails with an error since PHP 5.3+ (https://3v4l.org/kRKUN):

interface Fooable {
function foo(Fooable $f = null);
}
interface StrictFoo extends Fooable {
function foo(Fooable $f);
}

Thus this is not a BC break:

interface Fooable {
function foo(?Fooable $f);
}
interface StrictFoo extends Fooable {
function foo(Fooable $f);
}

The BC break you are thinking of has to do with changing the default parameter in this subclass so it isn't null anymore. That has to do with default parameters, not nullable parameters, and is therefore not part of this RFC.

Does that make sense?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1893#issuecomment-215737327

Contributor

Hi,
I think error messages should be adjusted as well, now it does not say anything about whether the argument is nullable or not. For example:

(function (?int $x) {})();
// Fatal error: Uncaught TypeError: Argument 1 passed to {closure}() must be of the type integer, none given

(function () : ?bool {})();
// Fatal error: Uncaught TypeError: Return value of {closure}() must be of the type boolean,

Member

Good point.

From: Michael Moravec notifications@github.com
Sent: Wednesday, May 11, 2016 3:39:16 AM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Nullable types (#1893)

Hi,
I think error messages should be adjusted as well, now it does not say anything about whether the argument is nullable or not. For example:

(function (?int $x) {})();
// Fatal error: Uncaught TypeError: Argument 1 passed to {closure}() must be of the type integer, none given

(function () : ?bool {})();
// Fatal error: Uncaught TypeError: Return value of {closure}() must be of the type boolean,

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1893#issuecomment-218332152

Contributor

Perhaps it could say "must be either null or of the type integer", or even "must be either of the type null or of the type integer".

Member

I would change the error message from "must be of the type X" to "must be of the type X or null"

From: Andrea Faulds notifications@github.com
Sent: Wednesday, May 11, 2016 12:12:46 PM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Nullable types (#1893)

Perhaps it could say "must be either null or of the type integer", or even "must be either of the type null or of the type integer".

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1893#issuecomment-218404288

Contributor

Shouldn't there be a test to ensure that the following is not allowed?

function nullableVoid(): ?void {}

Contributor

How about:

... must be either of the type integer or null, ...

Imho null should be the latter in the sentence since it's not a primary type.

@theodorejb: And maybe also nullable null (?null) as well, although it's not a valid type hint now, but who knows if union types get accepted.

Contributor Author

?void is currently allowed in the patch. I planned on handling it after the Union Types RFC has been voted on. I don't want to write code to reject ?void if it's just going away.

Contributor

Reverting a change made by a previously accepted RFC could be quite controversial. You might want to make a separate RFC for that.