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

PHP 8 union types, [presumably same for 8.1 intersection types]. #52

Open
sjokkateer opened this issue May 14, 2022 · 5 comments
Open
Assignees
Labels
Bug Something isn't working Enhancement
Milestone

Comments

@sjokkateer
Copy link
Contributor

Bug Report

Q A
Version(s) 3.7.x

Summary

When trying to create an object that depends on a union type in its constructor, a call to Injector::create() results in an error.

Current behavior

The Injector::class forwards the resolveParameters() call to the DependencyResolver::resolveParameters() method, which, at L#280 makes a call to Parameter::getType(). Within this method, a call is made to the underlying object's ReflectionParameter::getType() method is made. The returned object by getType() is a ReflectionUnionType::class, which has no method getName() defined, and hence, it crashes the program due to L#63.

The error message:
Error: Call to undefined method ReflectionUnionType::getName()

How to reproduce

Settings:
PHP 8.0.19 (cli)

Class definition:

class Foo
{
    public function __construct(private string|Stringable $s)
    {
    }

    public function getS(): string|Stringable
    {
        return $this->s;
    }
}

Test case:

public function testCreateClassWithUnionTypeParameterAsConstructorArgument(): void
{
    $injector = new Injector();
    
    $s       = 'Hello, World';
    $foo     = $injector->create(Foo::class, ['s' => $s]);
    $actualS = $foo->getS();

    $this->assertEquals($s, $actualS);
}

Expected behavior

An instance of Foo::class constructed with the property $s set to the string value 'Hello, World'.

PS

I am sorry if this is not considered a bug, but I did not know where to otherwise post this matter. Please let me know what you think of the above stated.

@sjokkateer sjokkateer added the Bug Something isn't working label May 14, 2022
@Ocramius
Copy link
Member

This is kinda like a PHP 8.0 feature request. PHP 8.1 will have similar problems with intersection types.

I think that digging for ReflectionParameter#getType() usages is the start of a researcher here: phpstan/psalm checks being improved would highlight these regressions caused by the language.

@sjokkateer
Copy link
Contributor Author

I see and thanks for your swift reply. Would you like me to modify this issue's format into that of a feature request, create a new issue for this matter, or are there some other steps I should take?

@Ocramius
Copy link
Member

IMO just PR if you do any work on supporting union or intersection types 😁

@Ocramius
Copy link
Member

What I can imagine the logic to be:

  • For a union type, search for all types in the union in the DIC
    • if one (and only one) matches, use that
    • If multiple services match, throw an exception that explains the ambiguity
  • For an intersection type, do the same as above, but:
    • All service instances must be checked for compatibility with the intersection type
    • If none matches, throw an exception
    • if multiple natch, throw an exception
    • If exactly one matches, use that

@tux-rampage
Copy link
Member

Checking the type compatibility will add significant runtime cost.

Intersection types will most likely be a combination of interfaces. Multiple classes are mutually exclusive. To achieve this each injection candidate must be fetched from the DIC which will not only increase the runtime cost further, it may also produce unwanted/unexpected instanciations.

For the sake of simplicity I'd suggest the following:

  • Configured param injection takes precedence and no further checks are performed, as for regular types.
  • The DIC will only be checked with has() for existence.
  • For union types:
    • Type preference is resolved then checked for existence
    • if there is more than one match, an exception is thrown.
  • For intersection types:
    • Type preference is resolved
    • if there is a preference, it must match all given types in order to be selected as candidate
    • if there is no matching preference for any type an exception is thrown.

@tux-rampage tux-rampage added this to the 3.15.0 milestone Nov 19, 2024
@tux-rampage tux-rampage self-assigned this Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Enhancement
Projects
None yet
Development

No branches or pull requests

3 participants