Add PRE_CLONE/POST_CLONE event dispatch handling to sub referenced entities

Rerolling patch for 2.x branch and 2.0.0-alpha1

Attaching interdiff for patches in #5 ✨ Add PRE_CLONE/POST_CLONE event dispatch handling to sub referenced entities Needs work

We're working with a MR here, no patches. I'll rebase the MR against 2.x.

Rerolling patch for 2.x branch and 2.0.0-beta2

We're working with a MR here, no patches.

Patch rerolled for 2.x branch and 2.0.0-beta2

Did you not read the comment I just posted? We're working with a MR here, no patches. This is the third time I’ve had to say this in this issue, please stop re-rolling patches.

With this patch/fix, the hooks are firing for the cloned referenced entities also. Using this fix on a site without any issue so, changing status to RTBC.

We are waiting this changes to be release since long. We must have to upgrade the latest release of this module on our site. Hence created patch.

Merged, released to be followed shortly! Thanks for working on this!

This issue seems to address same problem as #3089105: When cloneReferencedEntities is called it should dispatch events → (other issue patch also includes tests). Maybe we can close the other one (copying changes and credits from there) as this targets 2.x branch.

jonathanshaw → changed the visibility of the branch 3058025-2 to hidden.

I believe the approach here is better than #3089105: When cloneReferencedEntities is called it should dispatch events → therefore I have closed that one as a duplicate.

What's better about the approach here is that is moves the events into the cloneEntity method, whereas the other approach keeps the events in the form, but also calls them when cloning child entities. The approach here avoids complicating the code base, and keeps in the direction of seperating the API from the UI.

I've moved the test across from #3089105: When cloneReferencedEntities is called it should dispatch events → and suggest maintainers credit contributors to that issue.

I think this MR may have caused tests to fail previously when it was committed becaue it was buggy: in the committed version the pre-clone event dispatching was both left in the form and moved into the handler, so the dispatch was duplicated.

jonathanshaw → changed the visibility of the branch 3058025-add-preclonepostclone-event to hidden.

I squashed some commits to make it easier to rebase this branch. The branch now applies to 2.x again.

Phpunit tests are failing: https://git.drupalcode.org/issue/entity_clone-3058025/-/jobs/2022248#L81

Also pipeline should be green for every job( along with phpcs and phpstan) except phpunit next major as per the latest change in 2.x-dev

I think passing $already_cloned variable would be really useful for modifying.

What do you think?

At least for cases when some extra referenced entities might be cloned.
In the current implementation we aren't able to know if a referenced entity was already cloned or not.

And you guys are breaking current implementation of users which already use existing implementation of event dispatching.

I think you have to implement another type of event if you really want to have it dispatched for references.

I think passing $already_cloned variable would be really useful for modifying.

What do you think?

I think you might be right, but possibly this would be better done in a follow-up as it would presumably change the event for all invocations, references or not.

I don't really understand why cloneEntity() gets called on an already cloned entity in the first place.

And you guys are breaking current implementation of users which already use existing implementation of event dispatching.

I think you have to implement another type of event if you really want to have it dispatched for references.

You're right that is a risk. I think the natural pattern developers woud expect is single event, called in both circumstances. So maybe the thing to do is to create a new event and deprecate the old one, so we're not left with 2 forever.

I'd like to see what the maintainers say about this.

Regarding passing $already_cloned array to EntityCloneEvent

I think it's possible without breaking any functionality.

See an example with an optional parameter.
EntityCloneEvent::__construct(...)

public function __construct(EntityInterface $entity, EntityInterface $cloned_entity, array $properties = [], array $already_cloned = []) {

Adding getter and setter methods would be great as well.

I think it's possible without breaking any functionality.

I agree my concern is more with DX, trying to explain to developers working with the event what this means, why it is set or not set in this or that circumstance.