!!! BUGFIX: Make property mapper use add/remove methods on collection types

[albe]

This change will make the ObjectConverter use dedicated add_/remove_ methods on all collection type properties if available and a setter is missing, instead of always depending on setting the whole collection property at once.

    /**
     * @return Collection<RelatedEntity>
     */
    public function getValues()
    {
        return $this->values;
    }

    /**
     * @param RelatedEntity $value
     * @return void
     */
    public function addValue(RelatedEntity $value)
    {
        $value->setParent($this);
        $this->values->add($value);
    }

    /**
     * @param RelatedEntity $value
     */
    public function removeValue(RelatedEntity $value)
    {
        $value->setParent(null);
        $this->values->removeElement($value);
    }

This is documented as the recommended way to implement collection type relations. Note that the add/remove methods use the singularized form of the collection property name.

This is necessary for following use cases:

• whenever you have a *ToMany relation with orphanRemoval (all such relations to a non-AR have orphanRemoval, see https://github.com/neos/flow-development-collection/issues/1127#issuecomment-703278393) in order to prevent accidential data loss
• you want to add/remove items on the inverse side of a doctrine relation (i.e. the "one" side of a OneToMany relation)
• you want to remove items from a collection via web form and have special handling code in the remove* method
• you want to track entity changes by logging accesses to add/remove methods
• you have special domain rules for a collection property that are enforced per single item removal/addition
• you want to prevent other models from completely overwriting a collection property on your entity (no need for set* collection setter)

This will allow a more clean domain modelling with clear change rules on collection properties and enforce those rules within property mapping and your domain model.

Note: If you have a domain model with a collection property and a setter for that collection, you are highly advised to replace the setter method with an add and remove method for single items of the collection. If you already have add/remove methods and a setter, you are advised to remove the latter. Since with ORM 2.6 overwriting a collection property with orphanRemoval will execute a DELETE * FROM x WHERE x.parent = {uuid} statement, even if no elements were actually removed from the collection. If those elements are not properly readded, those elements will be deleted. Under any circumstance it leads to unintended operations on the database.

Resolves #2159

[albe]

:+1: from my side, please review

[albe]

Rebased and added documentation.

[kitsunet]

Left a comment, but looks pretty good. This little thing is something I would also like to split off soonish, and I guess really alone. It is not exactly related to the rest of the Reflection stuff in that namespace.

:+1:

[bwaidelich]

Left a few comments, too. Apart from those I struggle a little bit with this to be honest.. It adds new dependencies to the Doctrine Inflector and TypeHandling and, more importantly, it introduces some more magic and profoundly changes the behavior. Don't get me wrong: I understand your reasoning and see the need for this, but I wonder if we could achieve it in a more backwards-compatible and straightforward way

[albe]

Thanks for your feedback. I'll push a new commit soon.

@bwaidelich The thing is that this behaviour is really a missing thing in Flow and even Symphony added this behaviour with their 2.5 release. Without this you will get a lot of problems once you deal with e.g. a OneToMany relation to a non-aggregate or otherwise depend on specific behaviour to happen for every single item change on your collection property, which is a common case (e.g. some business rules that apply). Yes, this is rather a huge change in behaviour, but that's why I targeted master. If you have any idea though on how this could look in a b/c manner, I'd gladly try to follow your advice!

The TypeHandling is already a dependency on the ObjectAccess class, so I don't consider that a problem - actually it makes sense to be a dependency.

Regarding the dependency on Inflector - yes, that's a bit bad, but it was the closest thing that is already a dependency of flow (and not using an inflector would just feel awkward for add_/remove_ method names). Maybe this could be wrapped out into another class/utility to at least keep the Reflection stuff away from the Doctrine.Common dependency.

[albe]

Regarding the "\Traversable = collection type": I'm really not sure if that is the intent of the TypeHandling method to allow all implementations of \Traversable, since that only allows foreach traversal. So this now restricts the usage of the add/remove methods more than necessary, but in practice I guess all collection type properties will be of array, doctrine collection or SplObjectStorage type anyway...

[albe]

Hm... not sure why that PHP 7 + mysql Travis build failed, but seems unrelated to the change.

[kitsunet]

Restarted the job, was just a hiccup

[kitsunet]

ok, by now this must be rebased on the master with the split packages, but that makes just easier as TypeHandling and ObjectAccess are in the sam package...

[albe]

Straigth rebase to current master. With this there is now a small dependency issue with the split utility package. As is, the Utility.ObjectHandling now has a back-dependency on TYPO3.Flow which is bad (Inflector) - hence the Inflector Utility should be moved to the Utility.ObjectHandling package, in which case it adds an dependency to doctrine/common anyway. So I guess the Utility\Inflector class is rather unnecessary now. Any other ideas for that @kitsunet?

[bwaidelich]

Sorry for being the grinch, but I still have my issues with this one.. I won't block it of course, but IMO there are some smells about this:

• It's still breaking in case you implemented your own add*() and/or remove*() methods with custom logic
• There's still a dependency to the Doctrine Inflector – and creating an empty class in Flow that just extends it in order to hide this dependency doesn't improve things ;)
• It kinda feels wrong to have this lowlevel, Doctrine-specific logic within this very "general purpose" convenience class.

Again, I do see the need for this, but IMO this should be moved to another place.

As far as I see it, this is mostly about property mapping for web forms, right? So what about adding that logic to the PersistentObjectConverter? Most of the code could be copied and the functional test could even stay as is..

PS: In general I would argue that one should never expose domain models to the view in the first place. When working with simple DTOs (and therefor simple types like arrays) this problem doesn't occur. But I am aware that this is common practice (even in our very own code and tutorials), so I just mention it for the sake of completeness.

[albe]

Thanks once more for the feedback, I'm really open to it, as long as we somehow find a way to get this done.

It's still breaking in case you implemented your own add_() and/or remove_() methods with custom logic

Maybe this PR should be marked breaking, because it changes behaviour yes.

As far as I see it, this is mostly about property mapping for web forms, right?

Yes, that's indeed the main culprit, because the current property mapping requires a set{Collection} setter on your model, which is bad, be it a domain entity or a simple DTO. It loses information of the real intent (apart from being plain broken for removal of OneToMany elements).

So what about adding that logic to the PersistentObjectConverter?

Could be a way, though it might duplicate a little bit of code.

There's still a dependency to the Doctrine Inflector

As said, having non-inflected add_/remove_ methods would just feel awkward, e.g. addChildren/removeChildren on a children collection property is plain misleading. I find it ugly too, but that's how language works and as we should all know by now from all the DDD talks - clean programming is all about language. Do you see any other way to get correct method names without that dependency? (apart from c&p the logic)

creating an empty class in Flow that just extends it in order to hide this dependency doesn't improve things

Well, it would have moved the outward dependency higher up, so we could have changed the inflector implementation without changing the Reflection stuff. True, a simple extend is the worst way to do that, but the dependency tree has changed anyway now.

In general I would argue that one should never expose domain models to the view in the first place.

That's the pure theory PoV and I would share that, but reality is just different. It's simply much easier to bind your view to a domain model in a CRUDy application and get things done (tm).

[bwaidelich]

Thanks for your response, I'm glad you didn't take my veto the wrong way! Just one little note: I don't think the (dependency to the) Doctrine Inflector is a problem at all in general, it's a very useful tool. If it was used in the realm of persistence (e.g. in the PersistentObjectConverter) that would just be a better fit for it IMO.

Why do you think that would add duplication?

[albe]

Never taking your feedback personal, so no worries :) In the end, I even rather have destructive feedback than no feedback at all

Regarding duplication: I just thought that there would be some code from ObjectAccess needed to be copied to the ObjectConverter, but after thinking again I guess that was an overestimation.

I'll move this to the Converter and push a follow up, hopefully today.

[kitsunet]

I like!

[kitsunet]

(In converter)

[albe]

Ok, here is a first draft of refactoring that behaviour into the *ObjectConverter. It kind of feels weird to have methods that deal with property setting within the converter now, but I guess it still fits better there than in the ObjectAccess after all. Also, this should not have such big of a performance impact globally any more.

[albe]

Any thoughts on this implementation @bwaidelich @kitsunet?

[kitsunet]

I like this direction! If you address your own comments and rebase it should be pretty much good to go.

[bwaidelich]

@albe Thanks for taking care. I wonder why you modified the ObjectConverter!? I think this should only affect the PersistentObjectConverter

[kitsunet]

IMHO makes sense, this can very well apply to non persisted objects like DTOs.

[bwaidelich]

I'm afraid we're talking cross purpose here.. My point of moving this functionality to the "persistence realm" was to prevent broken/magic behavior. IMO we could have some special converter / trait / interface that you can use if you explicitly want to have this behavior but otherwise I wouldn't automatically apply this. Another option might be: Keep it in the general ObjectConverter but only activate it via a TypeConverter flag!?

[kitsunet]

Last option sounds good to me. But I also wouldn't be against moving it to persistence (object converter)

[albe]

Thanks once more for the feedback.

I'm afraid we're talking cross purpose here.. My point of moving this functionality to the "persistence realm" was to prevent broken/magic behavior.

IMO the behaviour is only "magic" because we didn't update collections in property mapping like this until now, but instead always just overwrote collections. The base idea is not specific to persistence, but to general DDD domain modelling, it just only shows the broken current behaviour in persisted entities.

IMO we could have some special converter / trait / interface that you can use if you explicitly want to have this behavior

The Opt-In (per domain model) to this behaviour is to actually have both the specific add* and remove* methods on your entities, so I don't see a reason to additionaly have to configure a special TypeConverter Option in each action or have a marker interface (there's a reason flow doesn't use marker interfaces for domain models, right?) apart from b/c concerns.

If that is your main concern, maybe this can be handled in a two-step: only activate this new behaviour with a feature flag in Settings for now and, unless this starts to backfire somehow, enable the flag by default in the next major release.

What do you think @bwaidelich?

[bwaidelich]

Hey, I need to get into this more, but just to clarify my concerns: I don't question the correctness of this feature. The danger I see with this is that it changes the behavior in a way that is not immediately visible. Introducing flags to change behavior on that level could be quite confusing.. Imagine a package sets this flag (or two installations have different default settings). So suddenly the system behaves slightly different, but you might only realize in a very specific scenario.

So lets get back to the core of the original issue: We want to make it easier to work entities that have OneToMany & ManyToMany relations in web forms. Do you agree that this is the main concern?

[albe]

So lets get back to the core of the original issue:

Sure.

We want to make it easier to work entities that have OneToMany & ManyToMany relations in web forms.

Not only in web forms, but that's the most common (probably 95%+) use case, yes.

The most problematic issue with this is dealing with OneToMany relations on entities, where it's currently not possible to remove items by submitting the One side without adding a lot of boilerplate code in the controller. For doctrine the Many side is always the owning side of such a relation, therefore to remove an item one has to either submit the removed item(s) seperately (imagine the javascript code for that) and manually update the relation on each, or fetch the original persisted One item, iterate all related items and find the ones that are currently missing. Btw. the same would apply also without doctrine and a simple DTO, where you would not be able to do anything on/for removed items, like log them, derive essential business intel from it or just have a business rule apply to maybe not remove items of a specific type under some circumstances - in essence you lose change information unless you work around that information loss.

By making Flow follow add/remove semantics, you can put any such logic into those methods, where it belongs.

[albe]

Do you think we can find a solution to this for 3.3 LTS @bwaidelich or would you rather consider this so (subtle) breaking that it should only go into 4.0 (with proper documentation)?

[bwaidelich]

Sorry for the late feedback, I lost track

Do you think we can find a solution to this for 3.3 LTS

With an opt in approach as described in April 2016 (https://github.com/neos/flow-development-collection/pull/112#issuecomment-211019221) it should be possible to add this in a non-breaking fashion

[albe]

I could think about an interface on a class to opt-in to the behaviour (e.g. UseAddRemoveMethodsInterface), but I'm unsure about the ObjectConverter option switch. As stated, it would be necessary to add this configuration at every place where the model including the methos is used, which is backwards.

[bwaidelich]

What about a marker interface and the corresponding type converter? That way we at least would not have to interfere with the objecthandling package