Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Foo|null reflection consistent with php-src #901

Closed
Ocramius opened this issue Dec 5, 2021 · 0 comments · Fixed by #902
Closed

Make Foo|null reflection consistent with php-src #901

Ocramius opened this issue Dec 5, 2021 · 0 comments · Fixed by #902

Comments

@Ocramius
Copy link
Member

Ocramius commented Dec 5, 2021

Currently, when running reflection on an A|null type, BetterReflection produces a Roave\BetterReflection\Reflection\ReflectionUnionType:

var_dump(
    get_class(
        (new DefaultReflector(new StringSourceLocator(
            <<<'PHP'
<?php
interface A {}
final class AClass {
    private A|null $typed;
}
PHP
            ,
            (new BetterReflection())->astLocator()
        )))
            ->reflectClass('AClass')
            ->getProperty('typed')
            ->getType()
    )
);

produces

string(53) "Roave\BetterReflection\Reflection\ReflectionUnionType"

In PHP-SRC, this behavior is different: https://3v4l.org/gMA4T#v8.1rc3

<?php

interface A {}
interface B {}

class Implementation
{
    function foo(A|null $param) { throw new Exception(); }
    function bar(A|B|null $param) { throw new Exception(); }
}

var_dump((new ReflectionParameter([Implementation::class, 'foo'], 0))->getType());
var_dump((new ReflectionParameter([Implementation::class, 'bar'], 0))->getType());

produces:

object(ReflectionNamedType)#2 (0) {
}
object(ReflectionUnionType)#1 (0) {
}

This means that a UnionType AST node composed of just null plus another type should be converted into a ReflectionNamedType, for the sake of compatibility with upstream.

This is ugly, but will (for now) avoid some bad issues in downstream handling (presently blocking Roave/BackwardCompatibilityCheck#324 )

@Ocramius Ocramius added this to the 5.0.0 milestone Dec 5, 2021
Ocramius added a commit that referenced this issue Dec 5, 2021
…tionNamedType`

Fixes #901

## Context

Currently, when running reflection on an `A|null` type, BetterReflection produces a `Roave\BetterReflection\Reflection\ReflectionUnionType`:

```php
var_dump(
    get_class(
        (new DefaultReflector(new StringSourceLocator(
            <<<'PHP'
<?php
interface A {}
final class AClass {
    private A|null $typed;
}
PHP
            ,
            (new BetterReflection())->astLocator()
        )))
            ->reflectClass('AClass')
            ->getProperty('typed')
            ->getType()
    )
);
```

produces

```
string(53) "Roave\BetterReflection\Reflection\ReflectionUnionType"
```

In PHP-SRC, this behavior is different: https://3v4l.org/gMA4T#v8.1rc3

```php
<?php

interface A {}
interface B {}

class Implementation
{
    function foo(A|null $param) { throw new Exception(); }
    function bar(A|B|null $param) { throw new Exception(); }
}

var_dump((new ReflectionParameter([Implementation::class, 'foo'], 0))->getType());
var_dump((new ReflectionParameter([Implementation::class, 'bar'], 0))->getType());
```

produces:

```
object(ReflectionNamedType)#2 (0) {
}
object(ReflectionUnionType)#1 (0) {
}
```

This means that a `UnionType` AST node composed of just `null` plus another type should be converted into
a `ReflectionNamedType`, for the sake of compatibility with upstream (this patch does that).

This is ugly, but will (for now) avoid some bad issues in downstream handling (presently blocking Roave/BackwardCompatibilityCheck#324 )
Ocramius added a commit that referenced this issue Dec 5, 2021
…tionNamedType`

Fixes #901

## Context

Currently, when running reflection on an `A|null` type, BetterReflection produces a `Roave\BetterReflection\Reflection\ReflectionUnionType`:

```php
var_dump(
    get_class(
        (new DefaultReflector(new StringSourceLocator(
            <<<'PHP'
<?php
interface A {}
final class AClass {
    private A|null $typed;
}
PHP
            ,
            (new BetterReflection())->astLocator()
        )))
            ->reflectClass('AClass')
            ->getProperty('typed')
            ->getType()
    )
);
```

produces

```
string(53) "Roave\BetterReflection\Reflection\ReflectionUnionType"
```

In PHP-SRC, this behavior is different: https://3v4l.org/gMA4T#v8.1rc3

```php
<?php

interface A {}
interface B {}

class Implementation
{
    function foo(A|null $param) { throw new Exception(); }
    function bar(A|B|null $param) { throw new Exception(); }
}

var_dump((new ReflectionParameter([Implementation::class, 'foo'], 0))->getType());
var_dump((new ReflectionParameter([Implementation::class, 'bar'], 0))->getType());
```

produces:

```
object(ReflectionNamedType)#2 (0) {
}
object(ReflectionUnionType)#1 (0) {
}
```

This means that a `UnionType` AST node composed of just `null` plus another type should be converted into
a `ReflectionNamedType`, for the sake of compatibility with upstream (this patch does that).

This is ugly, but will (for now) avoid some bad issues in downstream handling (presently blocking Roave/BackwardCompatibilityCheck#324 )
Ocramius added a commit that referenced this issue Dec 5, 2021
…on types

When an union type of `T|null` or `?T` is met, BetterReflection will (from now on)
keep a `ReflectionUnionType` internally, while exposing a `ReflectionNamedType`
with `ReflectionNamedType#allowsNull() === true` only at adapter level.

While this is a BC break, it leads to a much cleaner API around handling `null`
types, and inspecting types for type analysis.

Ref: #902 (comment)
Ref: Roave/BackwardCompatibilityCheck#324
Ocramius added a commit that referenced this issue Dec 5, 2021
…ion-types-are-turned-into-nullable-reflection-named-types

Ensure that simple union types like `A|null` yield a nullable `ReflectionNamedType`
@Ocramius Ocramius self-assigned this Dec 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant