@trowski I would like to see this in 7.1 please. It looks like the CI failed due to the rand() changes and I agree 100% that not making these changes is a bigger problem than breaking BC.

Please ensure these get merged before beta3.

There should also be a method to detect whether the type is nullable or not. There is allowsNull now, but it serves different purpose. The only way would be some smelly code like this: ((string) $type)[0] === '?').
(Well, whole parameter/return reflection api is kinda messy, but at least this. :))

You can't actually tell at runtime if it's implicit or explicitly null.
This would require changing allow_null from a boolean to an enum.

On Tue, Aug 9, 2016, 6:57 PM Michael Moravec notifications@github.com
wrote:

There should also be a method to detect whether the type is nullable or
not. There is allowsNull now, but it serves different purpose. The only way
would be some smelly code like this: ((string) $type)[0] === '?').
(Well, whole parameter/return reflection api is kinda messy, but at least
this. :))

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2068 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAPdhOAhPxmt0AgYYQrq9gijE7SSP7Cmks5qeSHqgaJpZM4JgjqB
.

That is strange.
NULL as a default value does not imply nullable type, i.e. signatures foo(?int $i) and foo(int $i = NULL) are not semantically equivalent and are not compatible with each other (and warning is produced).

In such case, this patch is broken:

class Test {
    function foo(int $z = null) {}
}

$rp = new ReflectionParameter(['Test', 'foo'], 'z');
var_dump((string) $rp->getType()); // oops, "?int", but should be "int"

@Majkl578 In your example, (string) $rp->getType() should return ?int as foo(?int $i = null) and foo(int $i = null) are semantically equivalent. The latter syntax is only retained for compatibility.

To detect foo(?int $i) vs. foo(?int $i = null), use ReflectionParameter::isOptional().

function foo(?int $i) {}
function bar(?int $i = null) {}

$parameter = new ReflectionParameter('foo', 'i');
var_dump($parameter->getType()->allowsNull()); // bool(true)
var_dump($parameter->isOptional()); // bool(false)

$parameter = new ReflectionParameter('bar', 'i');
var_dump($parameter->getType()->allowsNull()); // bool(true)
var_dump($parameter->isOptional()); // bool(true)
var_dump($parameter->getDefaultValue()); // NULL

Merged via 622d2f4. Thanks!

@trowski Well, I still disagree, having default value and requiring value are two different things (for caller, not callee).
There still seems to be another fundamental problem with this patch though -- compatibility with FQCN and nullable. I'm testing doctrine/common and its proxy generator component against PHP-7.1 branch right now. Since proxied classes are always generated with FQCN return types / type hints (thus prefixed with backslash), the produced code with this patch results in parse error anyway.
I.e. reconstructing the following:

public function returnsNullableObject() : ?\stdClass

results in:

public function returnsNullableObject(): \?stdClass

This unfortunately makes your original claim void:

After the changes in this PR, these projects work as expected with nullable types.

So libraries still will have to manually resolve this issue manually. And in order to not change method signature in proxied class (because there is no way to differentiate between nullable type and default null value, or both), it'd still require a hack such as ((string) $returnType)[0] === '?'. 😢

Also there is a bug (not sure whether caused by this patch or not, will need to bisect later):

namespace Foo;
function x(?\stdClass $y) {}

var_dump((new \ReflectionParameter('Foo\x', 'y'))->getClass()); // Fatal error: Uncaught ReflectionException: Class Foo\stdClass does not exist

EDIT:

Forgot to reply to NULL detection. How would you propose to solve this issue:

function x(?int $a = null) {}
function y(int $a = null) {}

foreach (['x', 'y'] as $fn) {
        echo $fn, ":\n";
        $rp = new \ReflectionParameter($fn, 'a');
        var_dump($rp->getType()->allowsNull());
        var_dump($rp->isOptional());
}

x:
bool(true)
bool(true)
y:
bool(true)
bool(true)

Looks like I focused on scalar types when I was experimenting with PHPUnit, etc. There does seem to be an issue with ReflectionType::__toString() when dynamically generating code in that it does not append a \ to the beginning of the fully-qualified name. Perhaps it should (and of course nullables would append ?\). @dshafik @nikic Thoughts on further changing __toString() here? Would there be BC concerns?

@Majkl578 You still seem confused about function (?int $a = null) { ... } vs function (int $a = null) { ... }. These two are semantically equivalent. There is no way to determine which sytanx was used to define the function because it does not matter. Both result in a function that accepts a nullable int with a default value of null. Also, there is no need to use hacks such as ((string) $returnType)[0] === '?' as $returnType->allowsNull() will indicate if the type is nullable.

@Majkl578 Handling of ?\stdClass parameter types fixed in 1397f75, thanks.

@trowski Interesting question. Usually I'd say that leading backslashes have nothing lost in strings, but this situation is special in that the output of ReflectionType::__toString() is likely to be used for code-generation, where this does matter. Not making the class names fully qualified means that anyone doing codegen based on ReflectionType is required to reimplement the name formatting logic. Currently, this is hardly worth mentioning, but thinking forward to types like (callable(A|(B[])): ?callable(C))|string, this will become unpleasant. So I'd say that yes, we should fully qualify names in __toString(). Of course this does break BC for people already manually prepending the backslash, but I don't think we can properly integrate nullable in ReflectionType without requiring some form of code-adjustment on behalf of ReflectionType consumers.

@morrisonlevi Your thoughts on this?