Update ConstructableFromArrayTrait.php #25
Conversation
Eliminates problems that arise when classes appear instead of simple parameter types
There was a problem hiding this comment.
Tkx for the PR.
However, I see some issues in your proposed changes.
Also, the idea is that the array contains the class arguments, ready to be injected. So if a parameter is a class, the array should contain the instantiated class already. Otherwise we might end up creating a DIC because not always the class can be instantiated with a single argument.
Nevertheless, if you feel this is a valid use case, can you please make a test for it?
I understand this trait is not tested explicitly, but it is tested through other use cases. Since your proposal is handling an edge case that does not exist in this codebase, we need an explicit test for it.
:)
| if ($reflectionParameter->isVariadic() && \is_array($argument)) { | ||
| $class = $reflectionParameter->getClass(); | ||
| if ($class && ($className = $class->getName()) && class_exists($className)) { | ||
| $argumentList[] = new $className($argument); |
There was a problem hiding this comment.
It seems like, at this point, $argument does not exist, right?
You removed line 48, which gets the argument.
Also, what happens if the argument is already an instance of the specified class? It looks like your code will try to instantiate the class with and instance of that same class as the argument.
And what happens if that class needs to be instantiated with several arguments?
It feels like we would be going in the direction of creating a DIC...
There was a problem hiding this comment.
I share your thoughts. Apparently, without DIС, unfortunately can not do
There was a problem hiding this comment.
On the other hand, these are models. They cannot have initiation requiring a DIC
Eliminates problems that arise when classes appear instead of simple parameter types