Cms: [3.2] `isNew` is never set in `EVENT_BEFORE_SAVE_ELEMENT`

[khalwat]

My plugin listens for Elements::EVENT_BEFORE_SAVE_ELEMENT and has different behavior depending on the passed in isNew property of the ElementEvent.

Unfortunately, in Craft 3.2.3 it appears that isNew never evaluates to true:

    public function saveElement(ElementInterface $element, bool $runValidation = true, bool $propagate = true): bool
    {
        /** @var Element $element */
        $isNewElement = !$element->id;

I haven't traced through the code fully yet, but I'm suspecting that the reason it's never set is due to the changes in the draft mechanism, the element always has an id as soon as the auto-save of a draft kicks in (which happens almost immediately).

This results in unexpected behavior in my plugin, since it tries to do different things depending on whether the entry is new or not.

Here's an example dump of print_r($element->getAttributes(), true) on an element as passed into my EVENT_BEFORE_SAVE_ELEMENT handler:

(
    [sectionId] => 4
    [typeId] => 4
    [authorId] => 1
    [postDate] => DateTime Object
        (
            [date] => 2019-07-16 19:03:00.000000
            [timezone_type] => 3
            [timezone] => America/Los_Angeles
        )

    [expiryDate] => 
    [newParentId] => 
    [deletedWithEntryType] => 
    [id] => 498
    [tempId] => 
    [draftId] => 
    [revisionId] => 
    [uid] => 58331c65-04ff-4ada-a22e-9d21f6b3c0f4
    [fieldLayoutId] => 8
    [contentId] => 956
    [enabled] => 1
    [archived] => 0
    [siteId] => 1
    [enabledForSite] => 1
    [title] => This is a test
    [slug] => this-is-a-test
    [uri] => blog/__temp_1t4eTG6O3BUPeBZU8bde9zRMMuAjv6BcKYe3
    [dateCreated] => DateTime Object
        (
            [date] => 2019-07-16 19:03:19.000000
            [timezone_type] => 3
            [timezone] => America/Los_Angeles
        )

    [dateUpdated] => DateTime Object
        (
            [date] => 2019-07-16 19:05:25.000000
            [timezone_type] => 3
            [timezone] => America/Los_Angeles
        )

    [dateDeleted] => 
    [trashed] => 
    [propagateAll] => 
    [resaving] => 
    [duplicateOf] => 
    [previewing] => 
    [hardDelete] => 
    [hasDescendants] => 
    [ref] => blog/this-is-a-test
    [status] => live
    [structureId] => 
    [totalDescendants] => 0
    [url] => http://craft3.test/blog/__temp_1t4eTG6O3BUPeBZU8bde9zRMMuAjv6BcKYe3
    [rating] => 
    [someRichText] => craft\redactor\FieldData Object
        (
            [_pages:craft\redactor\FieldData:private] => 
            [_rawContent:craft\redactor\FieldData:private] => <p>This is only a test</p>
            [content:Twig\Markup:private] => <p>This is only a test</p>
            [charset:Twig\Markup:private] => UTF-8
        )
)

Still inside of the EVENT_BEFORE_SAVE_ELEMENT since isNew is false I then attempt to load the saved element from the db to compare it to what we're about to save via:

                        $oldElement = Craft::$app->getElements()->getElementById($element->id);

Oddly, even though it's never been saved before other than as an auto-saved draft, print_r($oldElement->getAttributes(), true) doesn't show that it's a draft or a revision or anything of the kind:

(
    [sectionId] => 4
    [typeId] => 4
    [authorId] => 1
    [postDate] => DateTime Object
        (
            [date] => 2019-07-16 19:03:00.000000
            [timezone_type] => 3
            [timezone] => America/Los_Angeles
        )

    [expiryDate] => 
    [newParentId] => 
    [deletedWithEntryType] => 
    [id] => 498
    [tempId] => 
    [draftId] => 
    [revisionId] => 
    [uid] => 58331c65-04ff-4ada-a22e-9d21f6b3c0f4
    [fieldLayoutId] => 8
    [contentId] => 956
    [enabled] => 1
    [archived] => 0
    [siteId] => 1
    [enabledForSite] => 1
    [title] => This is a test
    [slug] => this-is-a-test
    [uri] => blog/__temp_1t4eTG6O3BUPeBZU8bde9zRMMuAjv6BcKYe3
    [dateCreated] => DateTime Object
        (
            [date] => 2019-07-16 19:03:19.000000
            [timezone_type] => 3
            [timezone] => America/Los_Angeles
        )

    [dateUpdated] => DateTime Object
        (
            [date] => 2019-07-16 19:05:25.000000
            [timezone_type] => 3
            [timezone] => America/Los_Angeles
        )

    [dateDeleted] => 
    [trashed] => 
    [propagateAll] => 
    [resaving] => 
    [duplicateOf] => 
    [previewing] => 
    [hardDelete] => 
    [hasDescendants] => 
    [ref] => blog/this-is-a-test
    [status] => live
    [structureId] => 
    [totalDescendants] => 0
    [url] => http://craft3.test/blog/__temp_1t4eTG6O3BUPeBZU8bde9zRMMuAjv6BcKYe3
    [rating] => 
    [someRichText] => craft\redactor\FieldData Object
        (
            [_pages:craft\redactor\FieldData:private] => 
            [_rawContent:craft\redactor\FieldData:private] => <p>This is only a test</p>
            [content:Twig\Markup:private] => <p>This is only a test</p>
            [charset:Twig\Markup:private] => UTF-8
        )
)

As it stands, I can't see how we can distinguish an element that isNew here.

[khalwat]

Relevant code referenced -> https://github.com/nystudio107/craft-retour/blob/develop/src/Retour.php#L295

[brandonkelly]

isNew does get set when the entry it initially created, however at that point it is created as an unsaved draft, so $checkElementSlug is going to be false.

So maybe replace this line:

$oldElement = Craft::$app->getElements()->getElementById($element->id);

with:

$oldElement = $element::find()
    ->id($element->id)
    ->siteId($element->siteId)
    ->anyStatus()
    ->one();

At the point when an unsaved draft is first getting saved into a real entry, $oldElement will end up coming back as null, since element queries won’t return drafts by default (whereas Elements::getElementById() will return a draft if that’s what the ID ends up referring to). So then you could delete this code:

if (Retour::$craft32 && ElementHelper::isDraftOrRevision($oldElement)) {
    $checkElementSlug = false;
}

[khalwat]

Here's what we ended up with, if it helps anyone searching on this: https://github.com/nystudio107/craft-retour/commit/9de8d09b3de61fa548f3400815186e91281867d8

The reason we didn't go with the above from Brandon is that by the time the element is getting saved, the DB has already been updated to drop its draftId

https://github.com/craftcms/cms/blob/develop/src/services/Drafts.php#L243-L258

So instead we're checking the element's URI for __temp_ is an indicator that it should be ignored.

We also decided that fetching $oldElement is a little pointless since $element will still have its old URI.

[putyourlightson]

It seems like removing the draftId from the table here:
https://github.com/craftcms/cms/blob/develop/src/services/Drafts.php#L243-L247

is unnecessary since it then sets $draft->draftId to null and then copies the value to the element record before saving:
https://github.com/craftcms/cms/blob/develop/src/services/Elements.php#L560

but I could be very wrong :)

[khalwat]

In Craft 3.2, a simplification is that you have 3 states for Elements:

1) A new entry
2) An auto-saved Draft that has never been an actual live entry
3) An existing entry

Imo most people expect isNew to be set in both states 1 & 2 on Craft 3.2, but it'll only be set on state 1, which is super brief (because it auto-saves the draft right away when you make a new entry)

And I think there's a good bit of code, both plugins and modules, that things won't work quite as expected because of this. Granted, there may have been logic issues already with pre-Craft 3.2, but the immediate auto-save to draft after making a new entry likely brings it to the fore.

[brandonkelly]

@putyourlightson If we don’t set draftId to null first, then the following query to delete the draft row would cascade-delete the whole element.

[putyourlightson]

Ah yes, thanks for the clarification @brandonkelly.

[khalwat]

Related -> https://craftcms.stackexchange.com/questions/31025/event-isnew-not-working-as-expected-after-craft-3-2-update

[brandonkelly]

We just released Craft 3.2.5, which changes the behavior of saving a new entry from the unpublished draft state. Now Craft assigns the entry a new ID at that point, so isNew will be true as expected.