Abstract creating commands from requests

[Zales0123]

This is the first approach to make creating commands from requests more abstract. It still requires some love, but I think it's a good step forward for even better code structure in this, yet awesome, plugin 🐃

Features

The main change is the introduction of CommandRequestParser object, which is responsible for creating a command request object based on the passed request and some identifier. Right now this ID is a FQCN of command, but I believe it can be anything else or even can be passed by some request parameter. Also, all command requests implement CommandRequestInterface now.

There are still some problematic requests:

• AddCouponRequest requires some additional methods for validation
• Put*ItemToCartRequest can be created from request or array 🦀
• EstimateShippingCostRequest does not even result in any command xD
  but I think we can take care of them in separated PR's.
• I did not change anything with address-related requests, as I believe this whole logic should be changed to CRUD 🚀

Things to improve:

• command request identifier (it's passed explicitly right now, could be somehow parametrized, see above)
• command request locator configuration (it has a big request-command map passed, so customization requires overriding the whole service definition - maybe we can provide some tag which with a user can tag their custom request-command pair and it would be added to this map automatically?)
• we should think how to unify also these problematic requests

Future:

As lots of logic is duplicated in lots of actions, I'm sure that the natural next step is creation of some default, basic action, that would consist of parsing a request and validating it. It should allow us to reduce amount of actions drasticly.

PHP version

I've bumped a PHP version to ^7.2 as I use object type hint in CommandRequestInterface. It would still be required if we decide to switch from tactician bus to Symfony Messenger, but I understand it can results in some tensions 😄

I would be happy to see your opinion about this change and/or iterate over it to make ShopApiPlugin one step close to the stable version 🎉

[mamazu]

Nice work, this streamlines the process of handling an API request even further.

[lchrusciel]

Thanks a lot for picking this up! 👍 a few more improvements and we are ready to tag 1.0 🎉

[lchrusciel]

Thanks, Mateusz! 🥇

[pamil]

I think there's a design flaw in the proposed (and now merged) HTTP Request -> Command cycle:

1. Request is defined as a stateful service while it's a data structure - also makes it incompatible to work with PHP-PM or any other asynchronous platform

2. You can use populateData and setUserEmail on it separately:

$request = new SetDefaultAddressRequest();

// first action
$request->populateData($httpRequest);
$request->setUserEmail('some@email.com');

// second action in the same process
$request->populateData($httpRequest);
/* user is not logged it so skip setUserEmail 
 * user email is still set to 'some@email.com'
 */

3. We expose those domain-request objects in the flow only to get the validation done easier in HTTP action controllers.

4. In case there are additional methods like setUserEmail, we assume the returned request object from the locator would be an instance of an exact class.

[pamil]

My idea would be to keep those request objects internal and abstract the way a HTTP Request is transformed into a Command:

interface CommandProvider
{
  /**
   * @throws InvalidRequest
   */
  public function provide(string $commandIdentifier, Request $httpRequest, ?UserInterface $user): object;
}

Then a controller, eg. the one setting the default address, could transfrom from:

try {
    /** @var ShopUserInterface $user */
    $user = $this->loggedInUserProvider->provide();
} catch (TokenNotFoundException $exception) {
    return $this->viewHandler->handle(View::create(null, Response::HTTP_UNAUTHORIZED));
}

/** @var UserEmailBasedCommandRequestInterface $setDefaultAddressRequest */
$setDefaultAddressRequest = $this->commandRequestParser->parse($request, SetDefaultAddress::class);
$setDefaultAddressRequest->setUserEmail($user->getEmail());

$validationResults = $this->validator->validate($setDefaultAddressRequest);
if (0 !== count($validationResults)) {
    return $this->viewHandler->handle(
        View::create($this->validationErrorViewFactory->create($validationResults), Response::HTTP_BAD_REQUEST)
    );
}

if ($user->getCustomer() !== null) {
    $this->bus->handle($setDefaultAddressRequest->getCommand());
    return $this->viewHandler->handle(View::create(null, Response::HTTP_NO_CONTENT));
}

return $this->viewHandler->handle(View::create(['message' => 'The user is not a customer'], Response::HTTP_BAD_REQUEST));

To:

try {
    /** @var ShopUserInterface $user */
    $user = $this->loggedInUserProvider->provide();
} catch (TokenNotFoundException $exception) {
    return $this->viewHandler->handle(View::create(null, Response::HTTP_UNAUTHORIZED));
}

try {
  $this->bus->handle($this->commandProvider->provide(SetDefaultAddress::class, $request, $user));

  return $this->viewHandler->handle(View::create(null, Response::HTTP_NO_CONTENT));
} catch (InvalidRequest $exception) {
  // It's an exception thrown by command provider
  return $this->viewHandler->handle(
    View::create($this->validationErrorViewFactory->create($validationResults), Response::HTTP_BAD_REQUEST)
  );
}

Or even if we change user provider to return null if none found:

try {
  $this->bus->handle($this->commandProvider->provide(SetDefaultAddress::class, $request, $this->loggedInUserProvider->provide()));

  return $this->viewHandler->handle(View::create(null, Response::HTTP_NO_CONTENT));
} catch (InvalidRequest $exception) {
  // It's an exception thrown by command provider
  return $this->viewHandler->handle(
    View::create($this->validationErrorViewFactory->create($validationResults), Response::HTTP_BAD_REQUEST)
  );
} catch (UnauthorizedAccess $exception) {
  // It's an exception thrown by a command bus (command handler exactly)
  return $this->viewHandler->handle(View::create(null, Response::HTTP_UNAUTHORIZED));
}

[pamil]

This way we enforce SRP:

1. Controller translates HTTP request into a command and translates the exceptions into HTTP response (HTTP-Application-HTTP communication)
2. Command provider gently creates a command while providing user-friendly validation
3. Command bus triggers the application logic

[lchrusciel]

I can totally agree, the decision was too hasty. We need a better solution. Nevertheless, I cannot find a validation moment in the answer you have proposed. Validation based on exception is also a bad design for me. Separate service is bad too. The core of these changes is to create a command and provide User-friendly exception if it is not possible.

But we may have:

interface CommandProvider
{
    public function provide(Request $httpRequest): object;
    public function validate(Request $httpRequest): ConstraintViolationListInterface;
}

Which is just an improvement of your proposal.

With the same rules as Validator provides: A list of constraint violations If the list is empty, validation succeeded. I prefer to decouple it from Symfony Validator, but this may be the second step of transformation. For example, we may provide some DTO, which can be created super easy from Symfony Validator, but also in a different way.

Command provider will be responsible for creating and validating HTTP request, without assuming what is below. One will be aware of the user, and others may generate uuid from another service. Internally it may use some DTO for validation or raw values (https://symfony.com/doc/current/validation/raw_values.html). Providers will be kind of factories with validation feature. If one would like to change command dispatched in one of the controllers, they will need to write new provider, command, and handler.

For now, I'm reverting this PR.

[lchrusciel]

Implementation proposal: #380