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

Feature/create article #20

Merged
merged 2 commits into from
Jun 24, 2020
Merged

Feature/create article #20

merged 2 commits into from
Jun 24, 2020

Conversation

jaljo
Copy link
Owner

@jaljo jaljo commented Mar 11, 2020

The command bus logic has been introduced here #16

This PR is an attempt to decouple domain related components from Symfony ones using the command pattern in a very simple use case: writing an article.

In this scenario, the controller creates and validate the form, map its content on a command (DTO), then throw it to the bus. The bus resolves the command handler, which instanciates an entity and persist it.

The major concern I have about the implementation is more about the folder structure: everything is at the same level:

src
  |_Command
  |_Controller
  |_Entity
  |_EventListener
  |_Form
  |_JsonDefinition
  |_Migrations
  |_Repository

whereas we should probably vace clear layers identified by the project structure, such as

src
  |_Command
  |_Domain (previously Entity)
  |_Symfony
      |_Controller
      |_EventListener
      |_Form
      |_JsonDefinition
  |_Infrastructure
      |_Migrations
      |_Repository

What do you think ?

@jaljo jaljo changed the base branch from master to feature/handlers March 11, 2020 09:00
@jaljo jaljo force-pushed the feature/create-article branch 2 times, most recently from af5cd4a to 0467f39 Compare March 11, 2020 09:57

public static function write(string $title, string $content, string $slug): self
{
return new self($title, $content, $slug);
}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read somewhere that it was more readable, in a business logic, to have Article::write() rather than 'new Article()' because "creating" and article has little meaning, its part of the abstraction glossary we created with the RAD paradigm.

Do you agree ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree, as long as it makes sense, semantically speaking. BTW, this is called a named constructor, and it’s an important part of DDD. I would not use this systematically though, but only when it’s appropriate, as it’s the case here.

It’s very true that every time you write some part of your domain model, you should think more about the semantics of your domain rather than the semantics of the programming language being used. Sticking to the semantics of your domain, in DDD, basically means: «How would the user describe that action, in their own words?», and indeed, in this context it makes more sense to «write an article» rather than «create a new article».

Copy link

@fabschurt fabschurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The major concern I have about the implementation is more about the folder structure: everything is at the same level, whereas we should probably vace clear layers identified by the project structure

Absolutely 😉 And there are no «official» rules for that matter, you have to figure out what makes the most sense to you 🙂 You can take a look at WeShop’s codebase to have my point of view on the subject, and you can take a look here to have Pedro’s point of view. Personally, I like to have clear Application and Domain layers (we’re on the same page with Pedro about this), but for the rest, I just scatter infrastructure-related stuff and vendor-cutomization-related stuff over specific, meaningful folders. I don’t say I’m right about this, but it’s what makes sense to me 🙂

config/services.yaml Outdated Show resolved Hide resolved
src/Command/CreateArticle.php Outdated Show resolved Hide resolved
src/Command/CreateArticle.php Outdated Show resolved Hide resolved
src/Command/CreateArticleResult.php Outdated Show resolved Hide resolved
src/Command/Handler/CreateArticle.php Outdated Show resolved Hide resolved

public static function write(string $title, string $content, string $slug): self
{
return new self($title, $content, $slug);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree, as long as it makes sense, semantically speaking. BTW, this is called a named constructor, and it’s an important part of DDD. I would not use this systematically though, but only when it’s appropriate, as it’s the case here.

It’s very true that every time you write some part of your domain model, you should think more about the semantics of your domain rather than the semantics of the programming language being used. Sticking to the semantics of your domain, in DDD, basically means: «How would the user describe that action, in their own words?», and indeed, in this context it makes more sense to «write an article» rather than «create a new article».

@jaljo jaljo requested a review from fabschurt March 24, 2020 09:31
config/orm/Article.orm.yml Outdated Show resolved Hide resolved
config/packages/doctrine.yaml Outdated Show resolved Hide resolved
config/services.yaml Outdated Show resolved Hide resolved
return new JsonResponse(
["error" => $exception->getMessage()],
Response::HTTP_INTERNAL_SERVER_ERROR
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je sais pas ce que tu en penses, mais personnellement, j’aime bien que mes actions soient view-agnostic, ce qui veut dire concrètement que je préfère leur faire renvoyer un format de données générique, par exemple un array, et ensuite je me démerde avec Symfony (avec un kernel listener, ou bien SensioFrameworkExtraBundle par exemple) pour qu’il transforme cette array en une réponse HTTP à posteriori.

Autre possibilité si on veut être davantage explicite et ne pas utiliser la «Symfony magic»: créer un service, qu’on injecterait dans chaque action, à qui on passerait l’array de sortie, et qui lui se démerderait pour formater ça pour la vue. L’important, à mon avis, étant de s’abstraire quoi qu’il arrive du format de sortie, et de ne pas générer les réponses directement dans le body des actions, mais de déléguer cette tâche à un service, afin de réduire le couplage.

À ce sujet, je sais pas si tu connais le pattern ADR (Action-Domain-Responder), qui est un genre de réinterprétation de MVC mais davantage adapté au web. Je trouve ça à la fois simple et complètement censé. En l’occurrence, ce dont je parlais ci-dessus correspond donc au Responder de ce modèle. Ce mini-site résume très bien le truc: http://pmjones.io/adr/

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merci beaucoup pour ce petit pattern, je ne le connaissait pas. Très sensé en effet, j'applique !

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be done in another pr ;)

$createArticle = new Command\CreateArticle();

$form = $this->formFactory->create(ArticleType::class, $createArticle);
$form->submit($request->request->all());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ça me semble un peu cavalier comme manière de submit un form 😄 Généralement, je crois qu’il est plutôt conseillé d’utiliser $form->handleRequest($request).

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmh je le fait comme ca parce que handleRequest() aime pas trop qu'on lui envoit un payload en json, même si il a été décomposé auparavant.

->add('content', TextareaType::class, [
'constraints' => [
new Asserts\NotBlank(),
new Asserts\NotNull(),
Copy link

@fabschurt fabschurt Apr 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je crois que NotNull est redondant avec NotBlank 😉

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, il me semble pas 🤔 IIRC NotBlank verifie que le string n'est pas vide, alors que NotNull verifie que la valeur n'est pas null.

src/Domain/Command/CreateArticle.php Outdated Show resolved Hide resolved
{
return $this->repository->findAllPublished();
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je sais plus si je t’en avais parlé, et c’est évidemment à toi de voir si tu veux t’emmerder à ce point-là, mais personnellement, je crois que dans mes prochains projets, je ne vais plus utiliser des commandes et un command bus pour les lectures. Si on respecte un minimum CQRS, c’est de toute façons illogique par définition. J’imaginais par exemple utiliser des «Presenters», qui seraient de bêtes services, qui la plupart du temps encapsuleraient juste un repository Doctrine certainement, et feraient le passe-plat pour les lectures. On pourrait permettre aux contrôleurs d’utiliser les repositories directement, mais c’est un peu touchy, dans la mesure où un repository peut être utilisé pour lire ET modifier le state du modèle. Je sais pas trop. En fait je me dis que ça peut vite devenir over-engineered aussi, donc bon… Bon, au pire, oublie ce que j’ai dit, mais clairement j’ai l’intuition qu’utiliser un command bus pour les lectures, ce n’est pas fait pour ça.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je suis entièrement d'accord, c'est overkill. La ca relève davantage d'une volonté d'avoir un traitement similaire pour tous les endpoints, mais en effet ca n'a pas de sens d'avoir des commandes pour ca. Je pense que dans les prochaines PRs, le controlleur aura directement accès au repository, je n'utiliserais le commande bus que pour les ecritures.

Base automatically changed from feature/handlers to master June 24, 2020 15:10
@jaljo jaljo merged commit c73683d into master Jun 24, 2020
@jaljo jaljo deleted the feature/create-article branch June 24, 2020 15:13
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 this pull request may close these issues.

None yet

2 participants