I'm looking to modify the values within the paragraphs of a cloned node before cloning.

I notice if I clone an entity, I can use my own event subscriber to subscribe to EntityCloneEvents::PRE_CLONE and EntityCloneEvents::POST_CLONE. This works great to modify fields on the cloned entity itself.

The dispatch code in EntityCloneForm::submitForm() handles this nicely:

    $duplicate = $this->entity->createDuplicate();

    $this->eventDispatcher->dispatch(EntityCloneEvents::PRE_CLONE, new EntityCloneEvent($this->entity, $duplicate, $properties));
    $cloned_entity = $entity_clone_handler->cloneEntity($this->entity, $duplicate, $properties);
    $this->eventDispatcher->dispatch(EntityCloneEvents::POST_CLONE, new EntityCloneEvent($this->entity, $duplicate, $properties));

However, if the cloned entity has referenced entities (say, a paragraph), the event subscriber isn't fired. This code, in ContentEntityCloneBase::cloneReferencedEntities, doesn't have the event dispatch code:

        $cloned_reference = $referenced_entity->createDuplicate();
        /** @var \Drupal\entity_clone\EntityClone\EntityCloneInterface $entity_clone_handler */
        $entity_clone_handler = $this->entityTypeManager->getHandler($referenced_entity->getEntityTypeId(), 'entity_clone');
        $entity_clone_handler->cloneEntity($referenced_entity, $cloned_reference, $child_properties['children'], $already_cloned);

It looks like ContentEntityCloneBase doesn't easily inherit EntityCloneForm's EventDispacher, so it's not easy to add these extra PRE_CLONE and POST_CLONE lines there.

I could try to write up a quick patch for this, but am worried about how this eventDispatcher should best be passed down to the recursive levels and am hoping to get a little guidance.

Can eventDispatcher be passed as a parameter from EntityCloneForm:::submitForm() down to ContentEntityCloneBase::cloneEntity()? Or is there a more elegant way to pass that event dispatcher around the cloning process?

CommentFileSizeAuthor
#15 3058025-15-move-preclonepostclone-event-dispatcher-2.0.0-beta4.patch6.79 KBjaydip makawana
#12 3058025-11-move-preclonepostclone-event-dispatcher-2.0.0-beta2.patch6.84 KBnitin shirole
#9 3058025-8-move-preclonepostclone-event-dispatcher-2.0.0-beta2.patch6.84 KBnitin shirole
#6 interdiff_patch_2.x__2.0.0-alpha1.txt2.34 KBsumit saini
#5 3058025-5-move-preclonepostclone-event-dispatcher-2.0.0-alpha1.patch6.77 KBsumit saini
#5 3058025-5-move-preclonepostclone-event-dispatcher-2.x.patch6.84 KBsumit saini

Issue fork entity_clone-3058025

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

TrevorBradley created an issue. See original summary.

DieterHolvoet made their first commit to this issue’s fork.

DieterHolvoet opened merge request !27

dieterholvoet’s picture

dieterholvoet commented
Status: Active » Needs review
sumit saini’s picture

sumit saini commented
StatusFileSize
new 3058025-5-move-preclonepostclone-event-dispatcher-2.x.patch6.84 KB
new 3058025-5-move-preclonepostclone-event-dispatcher-2.0.0-alpha1.patch6.77 KB

Rerolling patch for 2.x branch and 2.0.0-alpha1

sumit saini’s picture

sumit saini commented
StatusFileSize
new interdiff_patch_2.x__2.0.0-alpha1.txt2.34 KB

Attaching interdiff for patches in #5

dieterholvoet’s picture

dieterholvoet commented

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

Rajeshreeputra made their first commit to this issue’s fork.

nitin shirole’s picture

nitin shirole commented
StatusFileSize
new 3058025-8-move-preclonepostclone-event-dispatcher-2.0.0-beta2.patch6.84 KB

Rerolling patch for 2.x branch and 2.0.0-beta2

nitin shirole’s picture

nitin shirole commented
dieterholvoet’s picture

dieterholvoet commented

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

nitin shirole’s picture

nitin shirole commented
Version: 8.x-1.x-dev » 2.0.0-beta2
StatusFileSize
new 3058025-11-move-preclonepostclone-event-dispatcher-2.0.0-beta2.patch6.84 KB

Patch rerolled for 2.x branch and 2.0.0-beta2

dieterholvoet’s picture

dieterholvoet commented

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.

sumit saini’s picture

sumit saini commented
Status: Needs review » Reviewed & tested by the community

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.
Can a new tag be released including this fix?

jaydip makawana’s picture

jaydip makawana commented
StatusFileSize
new 3058025-15-move-preclonepostclone-event-dispatcher-2.0.0-beta4.patch6.79 KB

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.

rajeshreeputra’s picture

rajeshreeputra
he/him
Location Pune
commented

Test fails with latest code, not sure, if we add this in 2.x. Reverted the merge.

rajeshreeputra’s picture

rajeshreeputra
he/him
Location Pune
commented
Status: Reviewed & tested by the community » Fixed

  • Rajeshreeputra committed 5cbfafdb on 2.x 
    Revert "Issue #3058025 by DieterHolvoet, Rajeshreeputra, sumit_saini,...
rajeshreeputra’s picture

rajeshreeputra
he/him
Location Pune
commented
Status: Fixed » Reviewed & tested by the community
sumit saini’s picture

sumit saini commented
Status: Reviewed & tested by the community » Needs work
Related issues: +#3089105: When cloneReferencedEntities is called it should dispatch events

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 this as duplicate of #3089105.

jonathanshaw made their first commit to this issue’s fork.

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

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Needs work » Needs review

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 opened merge request !59

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

dieterholvoet’s picture

dieterholvoet commented

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

ankitv18’s picture

ankitv18 commented

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

ankitv18’s picture

ankitv18 commented
Status: Needs review » Needs work
ankitv18’s picture

ankitv18 commented
Version: 2.0.0-beta2 » 2.x-dev
rowen92’s picture

rowen92 commented

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 inside an even subscriber if a referenced entity is already cloned or not.

rowen92’s picture

rowen92 commented

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.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

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.

rowen92’s picture

rowen92 commented

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.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

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.

pbonnefoi’s picture

pbonnefoi commented

Message posted to the wrong issue