Scenario should allow for setting arbitrary ID for given events.

[simensen]

Here is my use case:

    /**
     * @test
     */
    public function it_should_suspend()
    {
        $userId = UserId::generate();

        $this->scenario
            ->given([UserWasRegistered::with($userId, new Person)], $userId)
            ->when(SuspendUser::with($userId))
            ->then([
                UserWasSuspended::with($userId),
            ]);
    }

Without being able to specify an ID for the given() events, I was getting an error from the event store saying that no stream could be found for the given ID.

There was already notes in the class saying that maybe hardcoding the ID to 1 wasn't all that good of an idea. :)

// todo: the hardcoded ID is probably wrong....

It might be more consistent to not have a default ID and to require the aggregate root ID to be the first argument instead. I implemented it this way first as it would be minimal impact on existing code and as a proof of concept.

[sstok]

👍

[simensen]

Merged simensen#1 from @wjzijderveld. It helped make this whole thing more explicit and still leaves it BC.

My only concern about this is that it means someone can call this and might expect that it works but it won't:

$this->scenario
  ->given($eventsBefore)
  ->withAggregateId($userId)
  ->when($command)
  ->then($eventsAfter);

[fritsjanb]

This line can be removed again :)

[simensen]

Done!

[fritsjanb]

This pull made us realize an inconsistency in our test setup.

According to AggregateRoot getId() should return a string, yet in this scenario we use the integer 1 as the default value. Since PHP is PHP everything still works, but I guess this would be a good time to change the default 1 to '1'.

Though this is a minor BC break in the Scenario all tests still run - if you run assertEquals() on an Event with id 1 and an Event with id '1' phpunit counts them as equals.

What is your opinion on this?

[simensen]

@fritsjanb I'm wondering if we can remove the requirement for getId on AggregateRoot altogether. I haven't tried implementing this yet, but this is what I have in mind:

https://gist.github.com/simensen/d982fac5550d3b5f8698

Basically, adding something like getIdForAggregateRoot to the RepositoryInterface and removing getId from the AggregateRoot interface. The repository (that already needs to be extended? this might not be entirely true...) would be responsible for asking its aggregate root for its ID in a way that the aggregate root expects.

[fritsjanb]

@simensen Short answer: could you create a separate issue for this?

Long answer: The fact remains that your aggregate needs a function to expose its identity. There is only one reason for this: it allows us to store the events.

Of course you could move the responsibility for this to the RepositoryInterface (I do think this is an elegant solution), but you would still have to implement a function in your AggregateRoot with the sole purpose of exposing an id for storage.

True, it allows you to expose a getFooId() instead of a getId(), but is there really that much added value to this?

Regarding the extending of the Repository: this is not entirely true. In our application we do in fact extend it (for typehinting purposes and it makes testing easier), but at this point it is not yet required.

At the moment our repositories look like this:

<?php

use Broadway\EventHandling\EventBusInterface;
use Broadway\EventSourcing\EventSourcingRepository;
use Broadway\EventStore\EventStoreInterface;

class FooRepository extends EventSourcingRepository
{
    public function __construct(EventStoreInterface $eventStore, EventBusInterface $eventBus, array $eventStreamDecorators = array())
    {   
        parent::__construct($eventStore, $eventBus, '\Qandidate\Project\Foo\Foo', $eventStreamDecorators);
    }   
}

but you could just instantiate an EventSourcingRepository directly.

Anyways, it would be nice if you could create a separate issue for this :)

[kelvinj]

Moved the discussion about the aggregate root here: #34

[asm89]

👍, nice API :)

[fritsjanb]

@kelvinj could you still change the default int 1 to a '1'? :)

[kelvinj]

@kelvinj could you still change the default int 1 to a '1'? :)

I think that one's intended for you @simensen

[simensen]

@fritsjanb The most recent change to #19 makes it so that ID can be an int, a string, or an object that implements __toString. I think that means that the default value in this case can be left at 1 (int)? Let me know if you disagree.

FWIW, I think it is a fine assumption that someone might want to use an int for their ID. As far as I know, (string) 5 === '5' so I think this is a safe assumption.

[kelvinj]

It'd be good to see #18 and #19 getting merged soon.

[E1379]

👍

[fritsjanb]

@simensen agreed with leaving 1 as an int

Merging!

[simensen]

@fritsjanb Yay! Awesome. :)