Commander
Commander copied to clipboard
laracasts
When the parameters within the construct of the Command class are many
Hi guys, this is a not real issue, but i want to tell you what i mean. Sometimes we need to process a lot of data, and not only a few like in the larabook's lesson. So in this situation the mapInputToCommand method doesn't works like we expect, if we think that we have an array like params for the construct, similar to something like that:
public function __construct(array $attributes)
{
$this->attributes = $attributes;
}
what can we do ? I must simply ignore the mapInput method ? Or is a mistake think like that ?
I'm having quite the same issue, for example the CreatePurchaseOrderCommand class, where there are a lot of properties and it doesn't really make sense to throw everything in the constructor.
So, what I did was modifying the mapInputToCommand method to also look into the public properties of the class when the constructor is not available. So now for the class which has a lot of properties, I just define those properties as public and not define the constructor for that class.
Here is the code:
// replace this code in the CommanderTrait.php
protected function mapInputToCommand($command, array $input)
{
$dependencies = [];
$class = new ReflectionClass($command);
$parameters = ($hasConstructor = !is_null($class->getConstructor()))
? $class->getConstructor()->getParameters()
: $class->getProperties(\ReflectionProperty::IS_PUBLIC);
foreach ($parameters as $parameter)
{
$name = $parameter->getName();
if (array_key_exists($name, $input))
{
$dependencies[] = $input[$name];
}
elseif ($hasConstructor && $parameter->isDefaultValueAvailable())
{
$dependencies[] = $parameter->getDefaultValue();
}
else
{
throw new InvalidArgumentException("Unable to map input to command: {$name}");
}
}
return $class->newInstanceArgs($dependencies);
}
Oh i understood, i think that can be a good resolution in that case. I'll try it.
@Mdpproduction Sorry, my initial solution does not work because I overlook 'newIntanceArgs()'. Please see PR#14 instead, it should work now.
@ratiw Ya, sorry dude, i forgot to warn you about that, anyway i fixed it myself with something similar like your code. Yesterday night i did tried another change inside that method, maybe isn't in line with dto pattern but works like map by type.
I still find this a little bit repetitive. Just wondering if you add 3 new fields to your "Orders" table... Would you have to check your $fillable attribute in your model, also check your command bus? This is the only thing annoying me about using this package.
@IsraelOrtuno You're right about this. But maybe this is the way it works the DTO, i know is a bit annoying if you have a lot of properties, to set inside the command Class. Maybe we can ask to Jeffrey more info for this particular case.
@Mdpproduction What I've done so far... Created a $attributes property into an abstract command that every command extends from, modified the validator to validate $command->attributes array and the mapInputToCommand just sets that attribute property to the input.
@IsraelOrtuno I was thinking about the same thing you mentioned after reading through the discussion on another PR#15 regarding the SRP violations. I like the idea of the command being able to validate before it gets created, so only valid command gets pass into the command bus.
What do you think about my solution? https://github.com/ratiw/Commander/blob/196555f354d4e4df13a8f60a2627aa6992e768c0/src/Laracasts/Commander/CommanderTrait.php
Any suggestion and comment are very welcome as I can also learn on this. :)
@ratiw I found a problem to this aproach. When writing tests I'm instantiating the CommandHandler and passing the Command, so without the constructor I can't do it. On my case this solution is handy when I'm updating a entity, because I can pass just the attributes I need to update. My solution, for now, is pass the ID and a DATA array with attributes I need to update.
@bruno-barros I don't quite understand what you mean. Can you please share your test code so that I can also have a look and learn from it?
Of course!
class CreateAdminCommandTest extends TestCase
{
public function setUp()
{
parent::setUp();
$this->handler = App::make('Namespace\CreateAdminCommandHandler');
}
public function test_something()
{
$this->handler->handle(new CreateAdminCommand('name', 'email'));
}
}
So, to test the behavior with diferent inputs I'm instatiating the command like this. If the command has no constructor I could not pass the data. The TestCase has no access to CommandBusTrait to run the $this->execute() method.
@bruno-barros I think you could do something like this.
public function test_something()
{
$command = new CreateAdminCommand;
$command->name = 'Some Name';
$command->email = '[email protected]';
$this->handler->handle($command);
}