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

Converted the generic concept of User entity to a complete aggregate object #4

Open
wants to merge 1 commit into
base: feature/value-types
Choose a base branch
from

Conversation

Ocramius
Copy link
Member

This patch moves all the responsibilities around existence checks and state mutations into the User object, which was now renamed to Aggregate, to make it clear that it is no longer just an entity with an assigned identifier, but it is now closing around its (now private) state, and protecting it also from outside read operations.

Here, we also introduce a couple domain components:

  • a "domain service" (NotifyOfIntrusionDetection) responsible of signaling authentication failures to external components
  • a few "read models" (EmailIsRegistered, IsUserBlocked) used during domain interaction with the aggregate in order to take business decisions.

These are implemented with technical components through the Infrastructure namespace.

@Ocramius Ocramius added the enhancement New feature or request label Oct 25, 2018
@ragboyjr
Copy link

@Ocramius, this is great stuff. What are you’re thoughts about keeping the domain pure and keeping IO out of it?

@Ocramius
Copy link
Member Author

@ragboyjr I've documented that at http://ocramius.github.io/blog/on-aggregates-and-external-context-interactions/

TL;DR: I/O is isolated to:

  • repository
  • read model
  • domain service

Aggregate can use domain services and read model (when passed at call time). In an ORM-style application, this means that some 2 phase commit situations will occur:

public function pay(PaymentGateway $gateway) : void 
{
    $this->paymentResult = $gateway->pay($this->amount, $this->account);
}

In a CQRS/ES context, this is a bit more clear, since this interaction would first raise an event, and then a second reaction to that may cause the I/O (still in the aggregate, but only around the actual I/O operation), which reduces the 2 phase commit issues by isolating the I/O into an independent transaction. For more of that, see ShittySoft/symfony-live-berlin-2018-cqrs-es-workshop#5

@ragboyjr
Copy link

@Ocramius i've read that article many times, it's a great reference!

However, you left this comment in here that made me think that your viewpoints on coding like this have changed: http://ocramius.github.io/blog/on-aggregates-and-external-context-interactions/#comment-3242618769

@ragboyjr
Copy link

I guess another thing that isn't clear with this example is proper way of handling db transactions within an aggregate method.

// doctrine entity that's persisted
class ImportProductRequest {
    
    private $state; // created, importing, finished

    public function importProducts(GenerateProducts $generateProducts, callable $persist, callable $flush) {
        $this->startImport(); // mark as importing
        $flush();
        foreach ($generateProducts($this) as $productDTO) {
            $this->incrementProductsImported();
            // create product entity from DTO, call persist
            // call flush after every 50 records
        }
        $this->endImport(); // mark as finished
        $flush();
    }
}

I've been using something like the above when I need to handle certain type of logic within an Aggregate, but when I read anything on DDD, it feels like this type of persistence logic should go into a service; however, I don't want to expose state of the ImportRequest Entity which basically manages an import of products.

@Ocramius
Copy link
Member Author

However, you left this comment in here that made me think that your viewpoints on coding like this have changed: http://ocramius.github.io/blog/on-aggregates-and-external-context-interactions/#comment-3242618769

It changed some stuff in my thought process, but the aggregate is still performing I/O. Specifically, aggregates perform the I/O, but it is vital to split interactions that perform I/O, and requests to perform I/O. The process managers (in event sourcing) are responsible for wrapping around I/O, and retry failed operations, while I previously let this responsibility solely to commands.

I guess another thing that isn't clear with this example is proper way of handling db transactions within an aggregate method.

One aggregate, one transaction. You don't touch more than one aggregate per business interaction. If you do, split into further commands that are executed independently. If you have bulk operations, I think that (for now) you reached the limits of where atomic operations on the domain suffice: a service will be required, yes, and the implementation may be very technical.

@ragboyjr
Copy link

Gotcha, good to know. It's been amazing how following this type of programming design has made my systems so much easier to test and develop.

It's very very easy to test all of the paths in your domain, and then you just wire it all together in a service and maybe just do some smoke or integration tests to make sure it's wired properly but don't really have to worry about mocking/interacting all of the different paths in those higher level components.

Thanks again for all the resources you (and people like you) provide for learning!

) : void {
Assert::true($password->verify($this->passwordHash));

if ($blockedUsers->__invoke($this->email)) {
Copy link

Choose a reason for hiding this comment

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

Can I ask.. Why magic method __invoke is used, but it is not called like this $blockedUsers($this->email)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question: this is because of an IDE issue. This is a known issue of PHPStorm, which cannot detect usages of callable objects used with the syntax that you proposed.

While it is true that I shouldn't please my tools too much, the IDE is such a massive central point in my day-to-day operations (especially around refactoring/inspection) that I'd rather keep the __invoke explicit until the bug is solved (should already be for PHPStorm 2019.02)

Copy link

Choose a reason for hiding this comment

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

Wow such a quick reply :) Ok I understand. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants