Open adamsafr opened 4 years ago
dave-redfern
commented
4 years ago @adamsafr Would this return the DTO from the FormRequest or is this in place of the form request so you type hint the UpdateEmployeeProfileRequest instead of the request on the controller and get a validated DTO? It would help if you added the controller implementation as well so I can see all of the intention. It's a little difficult with these few classes to get my head around the flow.
adamsafr
commented
4 years ago @dave-redfern ok :smile:
This is another implementation, but using DTO object with annotations instead of FormRequest. We create a new action argument resolver which checks if an argument implements RequestDTOInterface. Then the argument resolver deserializes json to DTO object.
class RequestDTOResolver implements ArgumentValueResolverInterface
{
private ValidatorInterface $validator;
private ValidationErrorsNormalizer $errorsNormalizer;
private DenormalizerInterface $denormalizer;
public function __construct(
ValidatorInterface $validator,
ValidationErrorsNormalizer $errorsNormalizer,
DenormalizerInterface $denormalizer
) {
$this->validator = $validator;
$this->errorsNormalizer = $errorsNormalizer;
$this->denormalizer = $denormalizer;
}
/**
* {@inheritDoc}
*/
public function supports(Request $request, ArgumentMetadata $argument)
{
return (new ReflectionClass($argument->getType()))->implementsInterface(RequestDTOInterface::class);
}
/**
* {@inheritDoc}
*/
public function resolve(Request $request, ArgumentMetadata $argument)
{
try {
$data = Json::decode($request->getContent());
} catch (JsonDecodeException $e) {
throw new BadRequestHttpException($e->getMessage());
}
if (!is_array($data)) {
throw new BadRequestHttpException('Invalid json string given.');
}
$input = array_merge(
$request->files->all(),
$request->query->all(),
$request->request->all(),
$data
);
$dto = $this->denormalizer->denormalize($input, $argument->getType(), null, [
Context::REQUEST_DENORMALIZATION => true,
AbstractObjectNormalizer::DISABLE_TYPE_ENFORCEMENT => true,
]);
$violations = $this->validator->validate($dto);
if ($violations->count() > 0) {
throw new ValidationFailedException(
$this->errorsNormalizer->normalize($violations)
);
}
$this->executeAfterValidation($dto);
yield $dto;
}
private function executeAfterValidation(object $object): void
{
if (!$object instanceof AfterValidationInterface) {
return;
}
$object->afterValidation();
// ...
}
}class ProfileController extends BaseController
{
public function updateAction(Employee $employee, UpdateEmployeeProfileRequest $request): array
{
$employee->updateProfile($request->firstName, $request->lastName, $request->phone, $request->timezone);
// ...
}
}There are several moments that I don't like in the current bundle:
Assert\Collection constraint. It does not work like Laravel's validation constraint. I have to write boilerplate code for optional fields and for array fields. I was thinking about how I can simplify this syntax:
public function rules()
{
return [
'reason' => [
new Assert\NotBlank(),
new Assert\Choice(Mission::REASONS),
],
'slots' => [
new Assert\NotBlank(),
'*' => [ // An alternative for Assert\All
'startDate' => [ // Here we don't have to write new Assert\Collection
new Assert\NotBlank(),
new Assert\DateTime(),
],
'endDate' => [
new Assert\NotBlank(),
new Assert\DateTime(),
],
],
],
'languages' => [
new Assert\NotBlank(),
'*' => [
new Assert\NotBlank(),
new Assert\Type(['type' => 'string']),
new Assert\Uuid(),
],
],
];
}@adamsafr ah! so I was guessing about right. I have to agree that some of the SF constraints are a bit weird/hard to use; and I've had similar battles with trying to validate optional arrays of key: value pairs or where a key should be null or a value. I was not aware of the * trick, that would save a fair chunk of typing.
I do have a few comments to offer you:
This should probably be an addition to the standard form request, instead of replacing it entirely. I think that there is something to be said for being able to decide how to handle the request, but have standardised error responses from the API.
You are using DTOs in your example, I prefer to create Command objects and then dispatch those via a command bus, moving all the domain logic into the entity / aggregate root. Means that the command can be dispatched in any context not just within the controller. If you've not looked at that, I would suggest at least having a read about it. Makes for very clean controllers.
Personally I am not a fan of annotations - other than in unit tests for setting the group. I dislike having comments affect code execution and moving responsibility out of the code directly. For example: in my entities I use beberlei/assertion library to have explicit domain validation instead of using comment assertions. That way my entities will always be valid regardless of how they are used. I quite like the separation of concerns you have right now and the affordance of being able to inject other services into the form request handler for complex validation logic outside of the standard constraints by using the callback constraint.
Finally I would use events instead of interfaces/methods for the afterValidation. That way you can have multiple listeners be able to operate on the subject. To be honest I cannot think of a particular use case off the top of my head, but it would allow greater flexibility as well as removing the logic from the DTO and into a manageable service. You could add a before validation event as well to e.g.: deny if the token has insufficient privileges if not using the security bundle.
Ultimately this is your project so I suggest implementing the things that you need 😄
I've implemented request DTO objects on my current job project. It looks like this:
Also supports:
@dave-redfern hello :slightly_smiling_face: What do you think about it ?