Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
The introduction of hasTranslationChanges() and using it when validating entities introduced some problems with field types that don't return TRUE when they should.
Before that, it was only used in the storage to optimize saving, so it was only a minor performance issue when the field values had to updated wen it actually wasn't necessary.
Now, it can prevent users from saving translations, without any workaround, so I'm setting this to major.
One example in core is the image field, from #2826021-48: FieldItemList::equals is sufficient from the storage perspective but not for code checking for changes:
Unfortunately it's not quite that simple. The widget used for images can end up submitting with no change in the referenced image, but clearing the height & width. So currently that's then considered a change, even though the preSave would only go and set the height & width properties what they already are in storage. ie. something like the following are compared:
array( 'file reference' => 'fid123', 'height' => NULL, 'width' => NULL, 'alt' => 'no change', )vs
array( 'file reference' => 'fid123', 'height' => '456', 'width' => '789', 'alt' => 'no change', )Those are not considered equal, even though the height & width in the first array will get set to '456' and '789' by preSave. I hope that makes sense?
Additional examples can be found in contrib, from #2826021-41: FieldItemList::equals is sufficient from the storage perspective but not for code checking for changes:
We've been testing this in our distribution as part of enabling and using content moderation. We've been running into some problems with more complex field types, problems identified so far:
GeolocationItem: It has a bunch of extra properties that it recalculates in preSave(), but that's only called after validation. I had some success in fixing that by doing that logic in setValue(), or maybe an onChange so that the computed + stored properties are always in sync if you change lat/long. However, it also has a map property that isn't set at all and then the properties don't match. Worked around that by initializing that in setValue() to an empty array, but that's kind of an API change?
WebformEntityReferenceItem: Same deal with a map property.
What seemed to help with the data properties was to put an array_filter() into the callback in FieldItemList::equals(), but not sure if that makes sense? Does seem fine to ignore a property being NULL/''/[] with one that's not set at all, which == fails on?
Proposed resolution
There are multiple approaches on how to deal with it, for example not doing value recalucation in preSave() but doing that in setValue(), to make sure it is always consistent, or overriding the equals() method to ensure it happens there.
Fix the field types in core, test them, create a change record to help contrib do the same.
Remaining tasks
User interface changes
None.
API changes
Likely none.
Data model changes
Also none.
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | 2988309-18.patch | 2.53 KB | krzysztof domański |
Comments
Comment #2
berdirComment #3
wim leersAFAICT this also affects API-First DX since #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.
Comment #4
berdirAnother scenario that I found now. for a boolean field, NULL and FALSE isn't the same.
It shouldn't be NULL, but it can be if you for example add a new boolean field to an existing entity then it is hidden on adding a translation to an existing entity but still extracted/processed and set to FALSE, Then it's different and saving fails. Not sure how to deal with that as there is now a field item and before there wasn't, so it fails very early in equals().
This could happen to any field widget that will initialize to a value despite being hidden.
Comment #5
wim leers#4: Great find!
Comment #6
berdirCreated an issue/patch for the geolocation field: #2993648: Implement equals() to allow saving translations and optimize saving. That's a complicated one and implementing equals() there kind of makes sense although a different fix would also be possible as I posted there.
The other use case I mentioned is the webform field which also has a property key that is NULL on the stored value and not set on the new values set from the (hidden) widget.
I think it might actually make sense to deal with that in core, by filtering out keys that have a NULL value. Those shouldn't make a difference in whether the item has changed or not. Here's a first patch for that.
Comment #7
tstoecklerThis is somewhat of a different issue, but the issue title makes this sound rather ambitious, so linking #2925972: FieldItemList::equals() doesn't correctly compare textfield values with leading zeros here. ;-)
Comment #8
hchonovAbout the image item problem:
as the width and the height are being set in the
preSaveand being retrieved from the current image I wonder if theequalsmethod should compare those properties at all? If the image has changed then theequalsmethod would detect the change anyway.Filtered properties are being filtered out of the equals' comparison. Why aren't the width and height properties flagged as computed?
Comment #9
wim leers+1
Comment #10
berdirWell, because computed properties aren't stored, but those values are.
See the geolocation issue above, that's very similar, and what I proposed there is to calculate them on setValue()/onChange().
Comment #11
hchonovAre you sure? I couldn't find a place where they are filtered out. I think, that if we provide a schema for computed properties then they will be saved.
Comment #12
hchonovSee
\Drupal\Core\Entity\Sql\SqlContentEntityStorage::saveToDedicatedTables()for dedicated tables and\Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapToStorageRecordfor shared tables.There we iterate through the columns of the storage definition and for each column retrieve the value of the field item without any restriction if it is computed or not.
Comment #13
berdirOk, wasn't aware, but there are more things to consider.
For example \Drupal\Core\Field\FieldItemBase::__construct() initializes computed properties *without* the value, because it doesn't expect there to be a value. so we would need to recalculate it which invalidates the whole reason the property exists, so we can avoid IO on displaying the image when we nee to know the size.
As I wrote, we can make it computed conceptually, just not mark the properties as computed and the compute logic remains in ImageItem, just like it is now.
Comment #14
hchonovOne more use case where the problem with NULL values occurs:
Given a field with multiple properties, where only one of the properties is set on the field. Make two instances of that field object and run one of them through the validation. Now compare both fields using the
\Drupal\Core\Field\FieldItemList::equals()method. Surprisingly the comparison now fails.The reason for this is that in the equals method we retrieve the field values through the
getValuemethod, which iterates over the$propertiesproperty of the field and collects the value for each property. So if there are no field properties yet initalized the getValue method will return nothing and respectfully it will return the values only for the properties that have been initialzed.So we have to either filter out NULL values like proposed by @Berdir or ensure that
::getValue()always returns all properties including the ones that have not been initialized yet. I like the approach taken by @Berdir, but I also think that::getValue()should be always returning the same result no matter if the field has been validated (and therefore the properties intialized) or not.Comment #17
hchonovThinking over it again I would be in favor of ::getValue() returning all properties. However this will be a much bigger possibly disruptive change.
Therefore here we can filter NULL values, but create a follow-up to discuss changing ::getValue() to always return values for all the properties regardless if they have been initialized or not.
We only need a test, which ensures that the current approach fixes the problem when comparing non-fully initialized field with a fully initialized field. When the test is there I'll RTBC.
Comment #18
krzysztof domańskiI added a tests.
Comment #20
hchonov@Krzysztof Domański, thank you for the test, it looks good. This should be ready to go now.
Comment #22
krzysztof domański#21 Rare random test failure. Unrelated so back to RTBC.
#3041318: [random test failure] Various fails when a random string is used in an xpath selector
Comment #24
krzysztof domańskiUnrelated test failure.
Comment #28
catchCommitted/pushed to 9.1.x and cherry-picked back to 8.9.x, thanks!