Skip to content

Conversation

Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented Sep 18, 2018

This PR proposes to deprecate the legacy signature of declaring nullable types by simply using NULL default value without explicit ? sign:

function (int $foo = null) {}

in favor of:

function (?int $foo = null) {}

The former signature is a relic from pre-7.1 era where nullable types didn't exist.

We also had an interesting discussion about this in past, i.e.: doctrine/common#805 (comment)
It also came up on Twitter recently: https://twitter.com/malukenho/status/1042025943873859584

Typed properties (coming in PHP 7.4) explicitly disallow the former declaration of implicit nullable types. I believe it'd be confusing to keep this legacy behavior, supporting it only for parameters and not typed properties - thus this deprecation should also target PHP 7.4.

@nikic
Copy link
Member

nikic commented Sep 18, 2018

I'd recommend a separate RFC for this one. This is a pretty major deprecation which is likely doing to be controversial. It's probably better to handle this separately and early (I don't expect that we'll be voting the bulk-deprecation RFC soon.)

@Majkl578
Copy link
Contributor Author

Alright then.
My feeling is that introducing new syntax that doesn't support this (typed properties) while keeping it elsewhere (parameters) would be a bad move for PHP.

Typed properties RFC states that:

We consider this to be a legacy behavior, which we do not wish to support for newly introduced syntax.

Should this legacy behaviour stay, I think typed properties must support it too. Otherwise it'd be just another candidate for #PHPSadness (inconsistent syntax).

I'll work on a separate RFC for this. 👍

@nikic
Copy link
Member

nikic commented Sep 18, 2018

Part of the motivation of requiring explicit nullable types is technical in nature. There is a somewhat minor but annoying issue relating the the automatic nullability promotion: A null default value can be the result of a constant expression which is not evaluable at compile-time.

function test(array $param = VAL) {}
const VAL= null; // Could be in a different file
test(null);

This code is valid, because we have special handling to check at runtime whether the default value evaluates to null and treat the parameter as nullable in that case.

However, this is imperfect, because it only affects which values the function accepts, but not inheritance checks or reflection. In particular, the following code

class A {
    function test(array $param = VAL) {}
}
class B extends A {
    function test(array $param = VAL2) {}
}
define('VAL', null);
define('VAL2', 'notnull');

is legal, even though it effectively restricts a parameter from ?array to array, which would usually be prohibited by our LSP checks. We can't resolve the default value for this check, because it is only known at runtime, which inheritance (in this instance) happens at compile-time due to early-binding.

This is something you might want to include in your RFC.

My own 2c are that it's too early. While I'm in favor of phasing this out long-term, deprecating it now means that it's not possible to write code that is compatible (deprecation-free) with both PHP 7.0 and PHP 7.4, which for me is too restrictive.

@guilhermeblanco
Copy link

@nikic Deprecations are a good thing to happen during minors, otherwise you'd have to rely on majors to do so. This means we'd only be able to prevent implicit nullability by PHP 9, which is a decade ahead of us.

I'd be fine to add deprecation notice on 7.X and remove support by 8.X

@Majkl578
Copy link
Contributor Author

Majkl578 commented Sep 18, 2018

My own 2c are that it's too early. [...] it's not possible to write code that is compatible (deprecation-free) with both PHP 7.0 and PHP 7.4.

Understood, it may be an issue for some libraries.
We can do a two-phase deprecation (I can include that in RFC too, as separate voting options):

  1. soft-deprecation (now/7.4) - documented, but no E_DEPRECATED
  2. hard-deprecation (8.0/8.x) - E_DEPRECATED
  3. removal (9.0) - E_COMPILE_ERROR

@php-pulls
Copy link

Comment on behalf of carusogabriel at php.net:

Labelling

@php-pulls php-pulls added the RFC label Sep 18, 2018
@ondrejmirtes
Copy link
Contributor

I understand the motivation and I personally don't write type $x = null anymore, but I don't like this change in the language, it's too opinionated and a BC break for the sake of BC break.

  • ?type $x means nullable
  • type $x = null means nullable and optional (this never stop being true even in post-7.1 era)
  • ?type $x = null also means nullable and optional

So people that write type $x = null don't do anything wrong, they just saved a character and the meaning of their code is still the same! (playing the devil's advocate here).

I think that requiring ?type = null is a matter of a coding standard (which you can do today using Slevomat CS) and it shouldn't be enforced by the language.

@stof
Copy link
Contributor

stof commented Sep 20, 2018

another reason to write type $x = null might be that the min supported version of the code is lower than PHP 7.1. I think triggering a deprecation for this syntax in a 7.x version is too early, regarding the actual upgrade rate in the ecosystem, which means that many packages still support PHP 7.0 or 5.6, but would like to run on latest PHP 7.x without triggering deprecations everywhere.

So if this gets deprecated, I suggest giving time for it (#3535 (comment) might be fine)

@Majkl578
Copy link
Contributor Author

Majkl578 commented May 1, 2019

There's a related Twitter thread: https://twitter.com/SaraMG/status/1123624550632017920

@Majkl578 Majkl578 force-pushed the deprecate-implicit-nullable-parameters branch 4 times, most recently from 7d52439 to 6921e48 Compare May 7, 2019 21:45
@Majkl578 Majkl578 force-pushed the deprecate-implicit-nullable-parameters branch from 6921e48 to 060f6a7 Compare May 24, 2019 21:15
@Majkl578 Majkl578 force-pushed the deprecate-implicit-nullable-parameters branch from 060f6a7 to 727c62e Compare March 17, 2020 22:22
@cmb69
Copy link
Member

cmb69 commented Dec 28, 2021

@Majkl578, what's the status here? Did you start an RFC?

@ondrejmirtes, see #3535 (comment) for why this isn't just a BC break for the sake of a BC break. :)

@iluuu1994
Copy link
Member

I'll close this for now. It doesn't look like there's an associated RFC either. Feel free to reopen when development continues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants