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

Resolve variadic parameters as an empty array #26950

Closed
aldemeery opened this issue Dec 24, 2018 · 12 comments · Fixed by #43985
Closed

Resolve variadic parameters as an empty array #26950

aldemeery opened this issue Dec 24, 2018 · 12 comments · Fixed by #43985

Comments

@aldemeery
Copy link

aldemeery commented Dec 24, 2018

consider these two classes

namespace App;

class Param { }

class Test
{
    protected $params = [];

    public function __construct(Param ...$params)
    {
        $this->params = $params;
    }
}

now when calling

app()->make(App\Test::class);

a ReflectionException will be thrown with a message Internal error: Failed to retrieve the default value

while doing a

$test = new App\Test();

without passing any arguments will work just fine.

On the other hand if the constructor was like this:

...
public function __construct($params = [])
...

calling

app()->make(App\Test::class);

will also work just fine.

So, I am suggesting to add the ability to resolve variadic parameters as empty arrays

PHP version: 7.2
Laravel version: 5.7.16 .

@imanghafoori1
Copy link
Contributor

I do not get any errors.

@aldemeery
Copy link
Author

I will update the issue with my php and laravel versions and how to reproduce the issue exactly

@koenhoeijmakers
Copy link
Contributor

Whats the usecase for a class that is resolved by the containee that injects a variadic parameter?

@seriquynh
Copy link
Contributor

I don't think making a constructor as you mentioned is a good idea.

@aldemeery
Copy link
Author

@koenhoeijmakers I am working on a project where I have a RuleBag class, that acts as small container for Rule classes, and it looks something like this...

...
class RuleBag implements \ArrayAccess, \Countable, \IteratorAggregate
{
    protected $container = [];
    
    // Of course rules can be set using a setter, but the class can 
    // be initialized with some initial rules.
    public function __construct(Rule ...$rules)
    {
        $this->container = $rules;
    }

    ...
}

and then I am injecting this RuleBag into other classes...

...
class ProductValidator
{
    protected $rules;

    public function __construct(RuleBag $rules)
    {
        $this->rules = $rules;
    }
    
    ...
}

Now you obviously see that RuleBag can't be resolved because of the variadic parameter in its constructor, which led me to modify it to be like this...

...
class RuleBag implements \ArrayAccess, \Countable, \IteratorAggregate
{
    protected $container = [];
    
    public function __construct($rules = [])
    {
        // Manually validate that each element in the array is of type `Rule`
        $this->validateRules($rules);
        $this->container = $rules;
    }

    ...
}

@aldemeery
Copy link
Author

@XuanQuynh Can you elaborate why you think so?

cause in my opinion if you can resolve a class with this constructor

...
public function __construct($params = []);
...

then it makes sense you can resolve a class with this constructor

...
public function __construct(Param ...$params);
...

because $params in both cases is an array with a default value of []

@seriquynh
Copy link
Contributor

I don't really want to dive into your example. I see no benefits this case. In fact, you may prefer Param ...$params to $params = []. I think it's your coding style.

@aldemeery
Copy link
Author

@XuanQuynh
Yes I know this is my coding style!
And I am no arguing if this has benefits or not!

I am just saying that if you can resolve $arr = [] because it has a default value, then it makes sense to be able to resolve Arr ...$arr because it has the SAME default value.

@seriquynh
Copy link
Contributor

@aldemeery Ok. I see. For me, I just prefer the simpler way. If this is still helpful for you, you may persuade the core developers of Framework.

@Korko
Copy link

Korko commented Dec 28, 2018

Sadly, this is not a Laravel bug (those fixable quickly here) but core php: https://3v4l.org/E7Ysf

@aldemeery
Copy link
Author

@Korko thanks for the update. That's definitely something to consider.
Closing this since it's not specific to laravel.

@Tofandel
Copy link
Contributor

Tofandel commented Sep 2, 2022

@Korko You can check if it's isVariadic and infer that the default value is [] it's a specific case

See #43985

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

Successfully merging a pull request may close this issue.

6 participants