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

Some update to Domain namespace #11961

Merged
merged 17 commits into from Jan 24, 2019

Conversation

Projects
None yet
3 participants
@sarjon
Copy link
Member

sarjon commented Dec 31, 2018

Questions Answers
Branch? develop
Description? Some renaming and command/query updating to accept only scalar/native values.
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? See changes, not sure if QA is needed.

This change is Reviewable

@sarjon sarjon force-pushed the sarjon:update-domain branch from a92b575 to ed84c80 Jan 4, 2019

@sarjon sarjon changed the title [WIP] Some update to Domain namespace Some update to Domain namespace Jan 4, 2019

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 4, 2019

@matks @eternoendless i have some questions regarding our current Domain:

  1. Should we have some naming convention for bulk action commands/handlers? For example:
  • "BulkDeleteCategoriesCommand" - with bulk prefix and plurar form of our entity name
  • "BulkDeleteCategoryCommand" - with bulk prefix and singular form of our entity name
  • "DeleteCategoriesCommand" - without bulk prefix but with plurar form of our entity name
  1. We have "Meta" subdomain, for me it feels like using legacy name is wrong since we never use it in our language when we are speaking about it. What is more, in Back Office it is called multiple other names, like: "Seo & URLs" or "Pages", maybe there are more names. wdyt, should we keep "Meta"?
  2. Similar with "Group" subdomain, is it the right name? For me it seems that it lacks context since it is Customer group. Who knows what groups may emerge later on. On the other hand, it's like a separate subdomain that only references Customer, so im a bit lost here. Wdyt?
  3. Is it ok to use Dto inside Dto? For example, there is this big ViewableCustomer Dto. Inside that, there are smaller Dtos that contains specific information. wdyt?
  4. After some investigation i started wondering whether Commands/Queries should contain VOs inside? maybe we should just stick with native values instead? As I understand now, Command/Query is like a message that passes data from one place to another. However, when using VOs, they add behavior to those messages, is it ok? For me, it seems that flow should be something like this:
    DTO (command/query with no behavior) => goes to => Domain (behavior is here) => comes out as => DTO (for queries, with no behavior)
  5. Regarding our Domain structure, i find that its more common to have Domain/Model and inside our BCs. What is more, we could have (just an idea) something like SharedKernel for very very generic code (e.g. Email VO, DomainException and similar.) instead of having Exception namespace directly under our Domain. does that make sense?
  6. Another random thought is, wouldnt it be better to rename let's say Domain/Customer/Dto to Domain/Customer/ReadModelor something similar so it would be the place where DTOs that are returned by read model (Queries) would live?
  7. Is it ok to execute 2 or more Queries on request (lets say from controller)?

@matks matks added the migration label Jan 10, 2019

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 17, 2019

We have "Meta" subdomain, for me it feels like using legacy name is wrong since we never use it in our language when we are speaking about it. What is more, in Back Office it is called multiple other names, like: "Seo & URLs" or "Pages", maybe there are more names. wdyt, should we keep "Meta"?
Similar with "Group" subdomain, is it the right name? For me it seems that it lacks context since it is Customer group. Who knows what groups may emerge later on. On the other hand, it's like a separate subdomain that only references Customer, so im a bit lost here. Wdyt?
Is it ok to use Dto inside Dto? For example, there is this big ViewableCustomer Dto. Inside that, there are smaller Dtos that contains specific information. wdyt?
After some investigation i started wondering whether Commands/Queries should contain VOs inside? maybe we should just stick with native values instead? As I understand now, Command/Query is like a message that passes data from one place to another. However, when using VOs, they add behavior to those messages, is it ok? For me, it seems that flow should be something like this:
DTO (command/query with no behavior) => goes to => Domain (behavior is here) => comes out as => DTO (for queries, with no behavior)
Regarding our Domain structure, i find that its more common to have Domain/Model and inside our BCs. What is more, we could have (just an idea) something like SharedKernel for very very generic code (e.g. Email VO, DomainException and similar.) instead of having Exception namespace directly under our Domain. does that make sense?
Another random thought is, wouldnt it be better to rename let's say Domain/Customer/Dto to Domain/Customer/ReadModelor something similar so it would be the place where DTOs that are returned by read model (Queries) would live?
Is it ok to execute 2 or more Queries on request (lets say from controller)?

  1. I like "BulkDeleteCategoriesCommand"
  • the Bulk prefix is very meaningful, it clearly indicates what the class does
  • the plurar form seems logical

We can make it a naming convention, but it's not the most important to be enforced ;)

  1. We can update the "Meta" name but we need to find a suitable replacement. "SEO and URL" is not relevant as there is more than SEO and URL in this page. You can modify Apache config, generate the robots.txt file ... maybe "Routing" ?

  2. Group, however, can be improved into CustomerGroup IMO. It's fine to have it separated from Customer. We have categories for products, but we dont put them into Product folder. However we could rename them ProductCategories if needed although I dont think it will be necessary as it is obvious for people what they are.

  3. Yes, DTOs inside DTOs are fine. Even DTOs inside DTOs inside DTOs. A DTO is just structured data. You can use them as much you want. Think about it as improved arrays : you have getter, setters, sometimes extended getters with a few logic in it, and most important: you know what you are dealing with, which is not the case for arrays. In fact if we were to build a software that would change very few and not extensible, we could replace all arrays with a DTO. This would make the software robust and very readable.

And the split of the big Customer DTO in smaller DTOs is good for the class readability.

  1. ValueObjects on the other hand are more than structured data. Think about it as prestashop custom types. We have php primitive types: string, integer, float ... and we add new types for our domain, which is a e-commerce domain. A string is always a valid string. "1.0" is a string while 1.0 is not a string. An integer is always a valid integer. 3 is an integer, 3.5 is not a valid integer. See ? When you construct one of those types, you know that once constructed it is valid. It's the same for ValueObjects: we want types that we know when we use them that they are valid. DTOs are just structured data, they can carry not valid data such as a birthday in the future. ValueObjects cannot.

Both DTOs and ValueObjects are OK :) and I think it's fine to use ValueObjects to build our Commands as it validates the Command earlier. We can accept DTO or primitive types, but if the data is not valid, it will be rejected just later.

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 17, 2019

@matks regarding your 5. answer, i guess it depends on how we think about VOs. From what I understand, VO is not only about making sure that some string named $email is actually an email. It's also about encapsulating behavior, similar as Entity in DDD except VO does not have identity. This is what I understand from reading various resources about DDD, however, now im thinking that VO in our PretaShop context (at least for now) is all about validation only (no behavior).

On the other hand, if i think about Commands, for me it seems that its the same DTO, except its responsible for transfering data from outside world to our Domain (where our behavior lives).
Now, if we start adding VOs to Commands, it seems that we are leaking our Domain behavior to the outside world and making our commands mutable, for example:

$command = new AddProductToCartCommand(//...);

// you can access domain behavior from Controller
$command->getValueObject()->someDomainBehavior();

// command may be modified in unexpected way
// before being dispatched
// but if there wasnt any VOs inside
// we are sure that once command is created it cannot be modified (immutable)
$this->commandBus->dispatch($command);

So as mentioned above, i think that Commands should only validate primitive types (e.g. that string is actually string, or int[] is actually array of integers) and actual VO creation should happen in Handlers.

Of course it may not be applicable for PrestaShop or my understanding may be wrong, but what do you think?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 17, 2019

  1. I'm not sure that I get it correctly 😅 can you rephrase it ?

  2. "DTOs" is a suitable name as it describes exactly what it is: structured data types. "ReadModel" is more intent-oriented as it carries the meaning of "what is this class for ?". DTOs can be used for everything.
    ReadModel is quite vague but if we find something better, maybe yes.

  3. This is where the CQRS theory meets the reality 😅 . In theory 1 request should provide you all the data needed for your usecase, which is here to display data in a BO page.
    However following this approach we will get Queries sometimes 95% identical that differ only for 1 parameter, which is a waste of code. So here there is no single answer, it will depend on the usecase. We have to look for a balance between the quantity of duplicate code, how much it is reused, how many Queries we need to dispatch in a Controller (for example one for KPIs, one for the data, one for the header ..?), how fast this makes the page ...

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 17, 2019

I'm not sure that I get it correctly 😅 can you rephrase it ?

sure, basically my question has 2 points:

  1. Shouldnt our BoundedContexts (thats what Product, Category & etc. that lives under Domain is, right?) shouldnt live under Domain/Model instead?
  2. Lets say we have shared code across BoundedContexts. For now we have exceptions that live under Domain/Exception, but it seems that exception is another BoundedContext, which is not, right? Lets say we have some generic Email VO, where do we put it? I see there is this concept SharedKernel which is the place for very very generic code that are reusable for all BoundedContexts and it seems that Domain/Model for would be right for BCs and Domain/SharedKernel for very generic domain code. does that make sense?
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 18, 2019

regarding your 5. answer, i guess it depends on how we think about VOs. From what I understand, VO is not only about making sure that some string named $email is actually an email. It's also about encapsulating behavior, similar as Entity in DDD except VO does not have identity. This is what I understand from reading various resources about DDD, however, now im thinking that VO in our PretaShop context (at least for now) is all about validation only (no behavior).

Yes right now it is the case, we use them mainly for validation. But if needed we can add behavior. However the behavior added will need to keep VOs immutables.

On the other hand, if i think about Commands, for me it seems that its the same DTO, except its responsible for transfering data from outside world to our Domain (where our behavior lives).

It's a bit more than that but yeah, it works: Commands can be seen as DTOs.

Now, if we start adding VOs to Commands, it seems that we are [...] making our commands mutable, for example:

VOs should be immutable. Our current VOs are immutable and it relies on us being watchful that it remains true. Same happens for Commands: they should be immutable, and it relies on us to make sure the implementation follows this principle.

So if we use immutable VOs inside Commands we keep them immutable.

DateTime is a VO for example ;) .

Now, if we start adding VOs to Commands, it seems that we are leaking our Domain behavior to the outside world

Yes, we leak the domain outside in order to make it simpler to use. We have to think about module devs and agency devs. These class are very useful for them. Additionnally it helps them to build Commands.
I dont think / plan we're going to apply DDD fully to PrestaShop, because of multiple items: its opensource nature and the modules which must boost code reuse, the transition from 1.6 to 1.7 that must be step-by-step, the fact that this is not a project that relies into a specific server but rather CMS / framework to be built on.

We must consider DDD, take the concepts that make sense and useful for PrestaShop and check that picking them and not the whole concept does not hurt the whole system.

Also, we are still in the early uses of DDD at PrestaShop. Right now I dont see the project following 100% DDD concepts but this might change.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 18, 2019

  1. Shouldnt our BoundedContexts (thats what Product, Category & etc. that lives under Domain is, right?) shouldnt live under Domain/Model instead?

It could, but it's not necessary. See Symfony comoonents: https://github.com/symfony/symfony/tree/master/src/Symfony/Component
You'll see "util" folders such as Debug next to very feature-oriented folders such as Validator or Messenger

  1. Lets say we have shared code across BoundedContexts. For now we have exceptions that live under Domain/Exception, but it seems that exception is another BoundedContext, which is not, right? Lets say we have some generic Email VO, where do we put it? I see there is this concept SharedKernel which is the place for very very generic code that are reusable for all BoundedContexts and it seems that Domain/Model for would be right for BCs and Domain/SharedKernel for very generic domain code. does that make sense?

Argh, the shared code issue 😅 always a nightmare to deal with.
Errr ... I guess the answer "we'll see when we have some" is not good, right ?

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 20, 2019

@matks thanks a lot for your feedback, it is really helps understanding this in context of PrestaShop. 👍

You'll see "util" folders such as Debug next to very feature-oriented folders such as Validator or Messenger

well, they are all reusable Components and thus they fall under same category (solve one specific problem) so, in my opinion, i dont think that calling some components "util" and others not is correct. For example, Debug component proves debugging features as Validator provides validation features.

I guess the answer "we'll see when we have some" is not good, right ?

actually, we've been thinking if Email VO from Customer BC can be reused in lets say Employee BC or should we have some common namespace (as i refered to SharedKernel) for VOs (like Email) and other very generic classes, that can be used in multiple BCs?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 21, 2019

@matks thanks a lot for your feedback, it is really helps understanding this in context of PrestaShop. 👍

You'll see "util" folders such as Debug next to very feature-oriented folders such as Validator or Messenger

well, they are all reusable Components and thus they fall under same category (solve one specific problem) so, in my opinion, i dont think that calling some components "util" and others not is correct. For example, Debug component proves debugging features as Validator provides validation features.

I guess the answer "we'll see when we have some" is not good, right ?

actually, we've been thinking if Email VO from Customer BC can be reused in lets say Employee BC or should we have some common namespace (as i refered to SharedKernel) for VOs (like Email) and other very generic classes, that can be used in multiple BCs?

@eternoendless I'd like some help on how to handle these 😉

@sarjon sarjon force-pushed the sarjon:update-domain branch from cdc6883 to 7eda278 Jan 23, 2019

@matks

matks approved these changes Jan 24, 2019

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 24, 2019

Thanks @sarjon

@matks matks added this to the 1.7.6.0 milestone Jan 24, 2019

@matks matks merged commit df56681 into PrestaShop:develop Jan 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 24, 2019

No need for QA as this is just folders / files being moved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment