!!! TASK: Rework AbstractDoctrineProjection for composibility & expli…

[bwaidelich]

…citness

This change slightly flattens the inheritance hierarchy of the doctrine
projectors and makes interaction with their state more explicit.

This is a breaking change because it affects the public API of the
AbstractDoctrineProjector and subclasses.
The public methods get(), add(), update() and remove() have
been moved to a sub object state.
Adjusting the code is straight-forward:

Previously:

final class SomeProjector extends AbstractDoctrineProjector
{
    public function whenSomeEvent(SomeEvent $event)
    {
        $this->add(new Object());
        // ...

Now:

final class SomeProjector extends AbstractDoctrineProjector
{
    public function whenSomeEvent(SomeEvent $event)
    {
        $this->state->add(new Object());
        // ...

Related: #136

[bwaidelich]

DoctrineProjectionPersistenceManager was renamed to DoctrineProjectionState and is no longer a "singleton".
The persistAll() call is now invoked in the shutdownObject() lifecycle method of that class

[bwaidelich]

Not related, but a left-over it seems

[bwaidelich]

Code moved a few lines down

[albe]

tbh, I'm not quite sure about this change (apart from getting rid of AbstractBaseProjector and the Package). I understand the intention but it somehow feels awkward and I'm unsure this is the right abstraction.

I mean State is a very abstract concept, but we're deep down inside a very specific (doctrine) implementation where "State" does not have a meaning. Doctrine is an ORM and deals with Objects (Entities) managed through an EntityManager inside a UoW, so why should we abstract that away at this level? If the user decides to use a Doctrine Projector, he most likely wants to deal with a relational Entity Model anyway and most likely already knows some Doctrine specifics.

It would make sense if our generic Projector implementation had a ProjectionStateInterface dependency, which in turn would be implemented by a DoctrineProjectionState in order to get a "DoctrineProjector". The remaining AbstractDoctrineProjector contains nothing doctrine specifc any more apart from instanciating a DoctrineProjectionState so it should then be moved up one level.

Alternative suggestion: How about we just completely get rid of the DoctrineProjectionPersistenceManager/DoctrineProjectionState intermediary? If you look closely, the AbstractDoctrineProjector just delegates a few methods, so IMO it does not have much value anyway and only adds an layer of indirection which you need to follow to understand the code. Let our Abstract*Projectors be very specific implementations for the underlying persistence mechanic and not try to introduce an unnecessary layer of abstraction that possibly just complicates other Projector implementations and/or the concept.

[albe]

Guess this could be removed then

[bwaidelich]

State is a very abstract concept, but we're deep down inside a very specific (doctrine) implementation

Au contraire, we're in the middle of high-level user land projection code ;)
And our part of the code is likely to be extended (see #136) so even though the risk of name clashes is probably quite low in that area, we're clearly mixing framework and user code in one class implementation.

But more importantly: Projection state & projector are not the same and I think it's not a good idea to mix up the two concepts..
I'd be fine with renaming the somewhat ambiguous method names (e.g. from get()/add().. to sth like fetchFromState()/addToState()) if you guys dislike the new layer

Alternative suggestion: How about we just completely get rid of the DoctrineProjectionPersistenceManager/DoctrineProjectionState intermediary?

Only my take, but as a general rule of thumb I think we should embrace SRP and try to avoid framework updates that change user land classes (including their base classes!). Ignoring that has lead to many issues in the past.

I'm all for getting rid of some Abstract classes though. Also the ProjectorInterface::getReadModelClassName() method doesn't really make sense to me (and it could easily be removed IMO). Could be done in a separate change I guess..

[albe]

Au contraire, we're in the middle of high-level user land projection code ;)

I beg to differ, but I guess it depends on the PoV. My point is that we are in an framework level convenience class, that is only concerned about helping the application to persist some state in the form of doctrine ORM models. This class could easily be completely replaced by some application implementation of how to persist the projection only via DBAL or whatever the user wants.

But more importantly: Projection state & projector are not the same and I think it's not a good idea to mix up the two concepts..

The distinction is already made via Read Models, which are the units of the projection state in this case. I don't see any benefit in introducing another layer of abstracting in the form of some intermediary "State" class, especially if we don't make that a formal concept of the projection (i.e. an interface).

Let's rather not focus on generalizing the Projection concept too much, but instead provide some helpful base implementations for a few concrete persistence methods. For the doctrine case I see that as:
"when I got an event in my handler method, I just want to get some Read Model Entity persisted/updated/deleted."

try to avoid framework updates that change user land classes (including their base classes!). Ignoring that has lead to many issues in the past.

Fair point, but I'm not fully convinced yet this is really an issue in this case. If we provide very flat single class base implementations of projection state persisters, which do not implement a generic framework concept/interface, I don't see the coupling as an issue on our side. The coupling happens to the persistence method, e.g. Doctrine, and we just provide a slim wrapper around that for convenience. We could document those as "Please copy this class into your code base and don't inherit it".

[bwaidelich]

Thanks for your valuable feedback!
I'm not 100% sure whether we are on the same track, let's talk about this in a weekly. In any case: It's nothing I consider urgent

[albe]

Yes I'll gladly talk this through more. I'm on vacation the next 2 weeks though, so that will have to wait a bit :) Feel free to discuss with the others and come to a conclusion though.

[robertlemke]

Hey! So far I basically agree with @albe's stand point: when you choose to use a Doctrine projector base implementation, it doesn't has to hide the fact that it's an ORM, and adding an additional abstraction (state) is somewhat confusing.

As we discussed during our last meeting, I'd like to introduce a createQuery method which allows for executing bulk operations in a Doctrine projector. As far as I can tell, that would move the projector's API just into the opposite direction than you intend with this change. All I can tell from my practice so far, and as far as I understand your pull request, more abstraction would be more difficult and blocks a few very real-world needs (like, for example, deleting all projection rows with a specific field value).

[bwaidelich]

Thanks for your feedback. I have to admit: I wasn't completely happy with the additional state injection, too.
But I still think, that the current MyProjector::get() is confusing (and that's what I heard from other people at the Conference as well). Would you agree to give those API methods a more explicit name like loadFromState() or something like that?

[robertlemke]

But I still think, that the current MyProjector::get() is confusing (and that's what I heard from other people at the Conference as well). Would you agree to give those API methods a more explicit name like loadFromState() or something like that?

Question is, if we need to come up with projector-generic method names or if we rather choose names specifically for the projector type. I'd rather do the latter.

However, Doctrine-specific method names would mean find($identifier) instead of get(), persist($object) instead of add() and no distinction between add() and update().

So, maybe a compromise would be to use the well-established method names from Flow's Doctrine Persistence Manager. The Persistence Manager provides methods like: add(), update(), remove(), isNewObject(), getObjectByIdentifier(), and createQueryForType(). I'd adjust them slightly for our purpose because the projector only supports one type: add(), update(), remove(), isNew(), getByIdentifier(), and createQuery(). In my opinion using find instead of get would risk users mixing up the finder methods in the Finder with the retrieval method in the projector.

[bwaidelich]

Thanks for the discussion.
After using more projections in actual projects, I now tend to avoid the AbstractDoctrineProjector for anything that is a bit more complex but instead implement the ProjectorInterface and have a custom ProjectionState class that interacts with the DB directly (DBAL).

[albe]

Interesting, if you find any good practices emerging, feel asked to share :)