[4.x]: getDirtyFields always returns all fields in a hook

[MoritzLost]

What happened?

Description

I'm trying to write a hook for Entry::EVENT_BEFORE_SAVE where I need to get a list of fields that have changed relative to the canonical element. This is surprisingly difficult:

Event::on(
    Entry::class,
    Entry::EVENT_BEFORE_SAVE,
    function (ModelEvent $e) {
        if (ElementHelper::isDraftOrRevision($e->sender)) {
            return;
        }
        Craft::dd($e->sender->getDirtyFields());
    },
);

This always returns an array with all field handles of the entry, not only those that have changed. Is this a bug or am I doing something wrong?

I've also tried some other methods:

• getModifiedFields() returns either an empty array or an array of all fields on the entry, depending on the state of the entry / draft being saved.
• getOutdatedFields returns an empty array.

How can I actually get a list of fields that have changed / are going to change during the pending save?

Steps to reproduce

1. Put the code above in a module.
2. On any published entry, modify some fields and save the entry.

Expected behavior

getModifiedFields() should only return the fields that have actually been modified, not all fields.

Also, the documentation (or the docblocks for the ElementInterface) for the three mentioned methods could be improved. They all sound very similar and to an outside observer it's difficult to understand in what scenarios they can and can't be used.

Actual behavior

getModifiedFields returns all field handles, regardless of which fields have actually changed.

Craft CMS version

4.4.5

PHP version

8.2

Operating system and version

No response

Database type and version

No response

Image driver and version

No response

Installed plugins and versions

No response

[brandonkelly]

How is the entry being saved in the first place?

[MoritzLost]

@brandonkelly Just normally from the Control Panel:

https://user-images.githubusercontent.com/10146880/228796007-e341d11f-1684-4c1f-8e38-8f8721b75ebe.mov

[brandonkelly]

Technically this is working as expected: dirty fields will only be accurate on POST requests from the control panel, and only for the element that the post data is getting applied to. In most cases, that element will be the provisional draft that automatically gets created the first time the entry gets autosaved, not the canonical element.

(The only exceptions are if autosaveDrafts is disabled—where no provisional draft would be created in the first place—or if you make a change and press Save / Command + S before the provisional draft is created. In those scenarios your current code should work as intended.)

To capture the fields that were modified on a draft, you’ll need to add this code as well:

use craft\events\DraftEvent;
use craft\services\Drafts;
use yii\base\Event;

Event::on(
    Drafts::class,
    Drafts::EVENT_BEFORE_APPLY_DRAFT,
    function(DraftEvent $event) {
        $modifiedFieldHandles = $event->draft->getModifiedFields();
        // ...
    }
);

All that said, I can see how it would be expected that getDirtyFields()/getDirtyAttributes() would return fields/attributes that were modified in the draft, when the draft is being applied to the entry. So I’ve just made that the case for Craft 4.5 (4c59bcbb5ea9ed3a6a42ec2be031906abc2475e5).

[MoritzLost]

@brandonkelly Thanks for the thorough explanation! And thanks for the adjustment, I think that makes sense.

Technically this is working as expected: dirty fields will only be accurate on POST requests from the control panel, and only for the element that the post data is getting applied to. In most cases, that element will be the provisional draft that automatically gets created the first time the entry gets autosaved, not the canonical element.

Just so I understand completely: If this is the case, why was getDirtyFields() returning all fields in my example above? Shouldn't it have returned an empty array then?

All that said, I can see how it would be expected that getDirtyFields()/getDirtyAttributes() would return fields/attributes that were modified in the draft, when the draft is being applied to the entry. So I’ve just made that the case for Craft 4.5 (https://github.com/craftcms/cms/commit/4c59bcbb5ea9ed3a6a42ec2be031906abc2475e5).

Thanks, that's looking good! But is modifying an interface 'allowed' in a minor version release? This is technically a breaking change, so shouldn't this have to wait for 5.0?

[brandonkelly]

Just so I understand completely: If this is the case, why was getDirtyFields() returning all fields in my example above? Shouldn't it have returned an empty array then?

If the specific dirty fields isn’t known, the element will just report all fields as dirty, to err on the side of caution.

But is modifying an interface 'allowed' in a minor version release? This is technically a breaking change, so shouldn't this have to wait for 5.0?

We are admittedly not really using interfaces correctly. We still expect that all elements extend craft\base\Element, and all fields extend craft\base\Field, so adding new interface methods is safe so long as we are able to provide a default implementation in those base classes.

(Eventually we will probably phase the interfaces out completely in favor of the base classes, since there’s no practical way to provide a custom element type/field type by implementing the interface directly.)

[MoritzLost]

@brandonkelly Thanks again, I understand now!

(Eventually we will probably phase the interfaces out completely in favor of the base classes, since there’s no practical way to provide a custom element type/field type by implementing the interface directly.)

Off-topic now, but I think the root problem is that the Element class has way too many responsibilities. This also leads to the Element class having a lot of properties and methods that only make sense for some element types, but still every element type gets them because some other element type requires them. It also makes the API reference very hard to read, because every element type gets tons of methods by default, and finding the actually useful methods that are specific to that element type becomes difficult. In the long, long term, I think it would be better to split up the ElementInterface in multiple smaller interfaces that can be individually implemented, and type-hint against those interfaces (or multiple of them using intersection types). But of course this would be a major rework, so it's probably not feasible.

But even if all element types need to extend the Element class, I still think it's useful to have the ElementInterface, so I can type-hint against it and also know which methods are safe to use.

[brandonkelly]

I think it would be better to split up the ElementInterface in multiple smaller interfaces that can be individually implemented

We do a bit of that – with BlockElementInterface, EagerLoadingFieldInterface, etc., but I agree we should probably be doing more, like ContentElementInterface, StatusElementInterface, StructureElementInterface, etc.

But even if all element types need to extend the Element class, I still think it's useful to have the ElementInterface, so I can type-hint against it and also know which methods are safe to use.

Well you’d be able to use craft\base\Element as a type declaration.

[MoritzLost]

We do a bit of that – with BlockElementInterface, EagerLoadingFieldInterface, etc., but I agree we should probably be doing more, like ContentElementInterface, StatusElementInterface, StructureElementInterface, etc.

@brandonkelly Absolutely agree! Structures are a good example, since most element types aren't structures, so having a separate interface just for that would make sense.

Well you’d be able to use craft\base\Element as a type declaration.

@brandonkelly Yeah, but the class may have public methods that are not part of the ElementInterface and therefore are allowed to change in minor releases. By type-hinting against the interface, I get errors in my editor when I try to use a method that isn't part of the interface, and decide whether to use a different method or type-hint against Element explicitly. But admittedly, it's probably not a common scenario.

[brandonkelly]

Craft 4.5.0 is out with that change to getDirtyFields() and getDirtyAttributes().

[masiorama]

Sorry to bother since the issue is closed, but I wonder if my scenario is included in what is reported above: I'm having the same difficulties to retrieve modified/outdated custom fields in a User element (which, as far as I know, has no drafts). Any suggestion? Thanks! @brandonkelly

[brandonkelly]

@masiorama Users don’t currently support dirty field tracking, but they will in Craft 5.

[masiorama]

@brandonkelly can you confirm dirty fields work on user entity? I'm about to make a huge upgrade to my platform just for this (didn't plan to upgrade to v5 for this project). Thanks.

[brandonkelly]

@masiorama In Craft 5, User::getDirtyFields() will only return the fields that actually changed, for user saves in the control panel, when called from EVENT_BEFORE_SAVE or EVENT_AFTER_SAVE.

[masiorama]

Thanks, I had the chance to edit my code after upgrading to craft5 and it does work as you described.