Incorrect behaviour when duplicating element

[putyourlightson]

Description

When clicking on Save as a new entry, Elements::EVENT_AFTER_SAVE_ELEMENT is called on the old element with isNew incorrectly set to true.

Steps to reproduce

1. Create and Save and continue editing an entry with the title Entry 1.
2. Change the title to Entry 2 and click Save as a new entry.
3. The Elements::EVENT_AFTER_SAVE_ELEMENT event is triggered first for Entry 1 with isNew set to true, then again for Entry 2 with isNew set to true.

    Event::on(
        Elements::class,
        Elements::EVENT_AFTER_SAVE_ELEMENT,
        function(ElementEvent $event) {
            $event->isNew;    // true
            $event->element->title;    // Entry 1
    });

Additional info

• Craft version: 3.2.10

[brandonkelly]

I just double checked and everything is working correctly. Here’s the expected order of operations:

1. The original entry is duplicated as-is, without applying any posted content changes yet.

   cms/src/controllers/EntriesController.php

   Line 334 in 620fa9e

   $entry = Craft::$app->getElements()->duplicateElement($entry);

   This will trigger the first EVENT_AFTER_SAVE_ELEMENT event, where $isNew is true but the title is still the original title.

2. The posted content is applied to the duplicated entry:

   cms/src/controllers/EntriesController.php

   Line 361 in 620fa9e

   $this->_populateEntryModel($entry);

3. The duplicated entry is saved again, now with posted content applied:

   cms/src/controllers/EntriesController.php

   Line 377 in 620fa9e

   if (!Craft::$app->getElements()->saveElement($entry)) {

   This will trigger a second EVENT_AFTER_SAVE_ELEMENT event. Now $isNew will be false, but it will have the now title.

If you’re seeing a second event where $isNew is true, I’m guessing that is triggered by an entry revision creation, if the section has versioning enabled.

[putyourlightson]

My mistake, the second time around $isNew is set to false. What I would like to do is notify a user whenever a new entry is posted along with its title. As it stands now, if an entry is duplicated then the notification is sent with the old title. Do you have any suggestions as to how I can ensure that the correct title is sent when the entry is duplicated?

[brandonkelly]

It’s best to send user notifications in a separate request via a background job added to the queue, so if you go that route, by the time the job is executed, the title will already be updated to the new title.

[putyourlightson]

Ok, thanks for the pointer.

I'm probably not seeing the bigger picture here, but the order of operations appears to be a bit convoluted and contains a permission check (point 2 below) which doesn't necessarily seem appropriate when a user has clicked on "Save as a new entry".

1. The original entry is duplicated as-is, without applying any posted content changes yet.
2. The user's permissions are enforced to ensure they are allowed to publish peer entries.

   cms/src/controllers/EntriesController.php

   Line 328 in 620fa9e

   $this->requirePermission('publishPeerEntries:' . $entry->getSection()->uid);

3. The posted content is applied to the duplicated entry.
4. The duplicated entry is saved again, now with posted content applied.

A more suitable (and simpler) order of operations might be:

1. If $duplicate is true then remove the entryId request parameter.
2. Carry on with saving as normal without duplicating the original element.

If this makes sense then I'd be happy to submit a pull request, otherwise I'll just go with the queue job :)

[brandonkelly]

Steps 1 and 2 on your first list are backwards – all permission enforcement is done before the entry is duplicated:

cms/src/controllers/EntriesController.php

Lines 328 to 334 in 620fa9e

	$this->requirePermission('publishPeerEntries:' . $entry->getSection()->uid);
	}
	// If we're duplicating the entry, swap $entry with the duplicate
	if ($duplicate) {
	try {
	$entry = Craft::$app->getElements()->duplicateElement($entry);

But I see your point – there’s no need to enforce the publishPeerEntries permission when duplicating an entry. That seems like an oversight given that we do skip enforcing the editPeerEntries permission for duplicates. So I’ve fixed that for the next release.

Your suggestion to simply drop the entryId is basically how it worked in Craft 2. The reason we changed it is, that approach doesn’t account for other sites’ content. So in Craft 2 when you duplicate an entry, all other sites’ content will be lost in the “duplication” process.

This is also the reason why we start by duplicating the entry as-is, before applying the posted content to it and re-saving it – if we were to go straight to applying the posted content and then re-saving, it would make it much harder to determine which field values should stay identical to how they were on the original entry for other sites. By fully duplicating the original entry as-is first, we know that the field translation settings will be applied to the incoming content exactly the same way it would have been for the original entry, had they clicked “Save” instead of “Save as a new entry”.

[putyourlightson]

I see, thanks for the thorough explanation Brandon, I had a feeling that I wasn't seeing the big picture here!!