Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
This was uncovered in #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work and hard-blocks that issue. It was split off to this issue because it's a significant new capability in Entity API that merits thorough reviews.
Proposed resolution
- Add
EntityWithDirtyFieldsInterface
(see "API changes" for details) - Update
ContentEntityBase
to implementEntityWithDirtyFieldsInterface
Remaining tasks
Patch that passes tests withdone, see #2456257-69: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work and later.EntityResource::patch()
updated to leverage this, to prove it works.- Thoroughly review the patch.
- Test coverage
- Review the patch MORE thoroughly.
User interface changes
None.
API changes
Add interface EntityWithDirtyFieldsInterface extends FieldableEntityInterface
with a single method: getDirtyFields()
. Why not add this to FieldableEntityInterface
? Because doing so breaks BC.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#60 | interdiff_59-60.txt | 2.37 KB | johnwebdev |
#60 | 2862574-60.patch | 9.04 KB | johnwebdev |
#59 | interdiff_57-59.txt | 4.52 KB | johnwebdev |
#59 | 2862574-59.patch | 6.93 KB | johnwebdev |
#57 | 2862574-57.patch | 5.84 KB | johnwebdev |
Comments
Comment #2
alexpottThis is so important. We have half implemented this for translated fields in #2297817: Do not attempt field storage write when field content did not change
Comment #3
Wim LeersComment #4
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedWord 'That' is mentioned twice in a sequence.
Fixed and attached new patch.
Comment #5
Wim Leers#4: That is utterly unhelpful. This patch does not need language nitpicking. It needs thorough reviews.
Worse, the change is *wrong*.
Please stop doing this. It's distracting. It's annoying.
Comment #6
dagmarJust asking here. According to this https://www.drupal.org/node/2112677 computed fields section says: "This tells the Field API that this field is computed so that it doesn't look for it in the database."
I'm not sure if the docs are accurate, but, would be the same use getFields(FALSE) here?
Comment #7
Wim LeersExcellent point: this definitely needs test coverage for how to deal with computed fields!
Comment #8
hchonovYou are defining a new entity property, which should be unique per entity object, but when dealing with translations and entity cloning, which is really common in Drupal it will break and leak. To avoid this, you have to break the reference in CEB::__clone if not cloning because of initializing a translation and in CEB::initializeTranslation make the property shared between the entity translation objects.
For common mistakes regarding this you could take a look at
...
You have to be aware of the current behavior of CEB::onChange, which will be fired even if you set the same value on a field e.g. $field->setValue($field->getValue()) and if you are ready to ignore this at the moment then probably it would be nice to document this in the code and open a follow up issue.
Comment #9
xjmRemoving credit for @Munavijayalakshmi. Thanks for your interest in contributing. Please make sure that your contributions are part of the current discussion on the issue. Read the issue carefully before posting new patches.
Comment #10
Wim LeersThis should become a deprecation instead.
Comment #11
hchonovWe just had a discussion about the new interface on IRC and personally at the DrupalDevDays Seville with tstockler and berdir and they both agree that we add new methods to content entities by adding them to ContentEntityInterface and in D9 they should be moved to the according interface - in this case to FieldableEntityInterface. We've done this for an example with the getLoadedRevisionId method.
This means we do not need the new interface and instead we have to add the new method to ContentEntityInterface.
Comment #12
Wim LeersI like that! A lot!
I'm wondering if we should document that on
ContentEntityInterface
and add@todo
s for this?Comment #13
hchonovI think that a
@todo
is fine.Comment #14
tstoecklerHere are some thoughts on the patch. I still want to make a list/table of "possible things you can do to entities" and the expected return value of
getDirtyFields()
in each case for the issue summary.Is this not "string[][]"? (is that allowed? Or should it be array[]?)
@hchonov pointed this out in person yesterday when we talked about this: Since this should be "synchronized" between all translations of an entity (as it already keys field names per translation), we should convert it to a reference in
ContentEntityBase::initializeTranslation()
- and then also break that reference inContentEntityBase::__clone()
. See the handling of the other properties in those methods, for inspiration.Ahh these hunks belong together, now I understand what is going on.
That's interesting. On the one hand this is the minimal fix to resolve this, on the other hand this could arguably be seen as a BC break. I.e. if I see the current code in
ContentEntityBase::postCreate()
and want to provide a default revision message for my entity I would overridepostCreate()
and unconditionally (!) call$this->setRevisionMessage('Just created a new entity!')
there. That would now then also be called for new translations, which would be unfortunate.The other (slightly more invasive) way would be to add a
postCreateTranslation()
method or similar, although that also seems non-ideal as then one could argue we would also need equivalent hooks for translation updating and deleting.As far as I can tell, this should clear the list of dirty fields in all languages, because we always save all translations of an entity.
So this seems to account for the case that there is something in
$this->dirtyFields
that is not a field name? How can that happen? Or am I missing something?@hchonov brought up the interesting case of:
The question is whether
'foo'
should be returned at that point. To fix this, we would somehow get the "original" item (not sure how, this could be tricky) and then compare with$items->equals($original_items)
. Since this would only result in "false negatives" (i.e. fields that are marked dirty that aren't actually dirty) and not "false positives" (i.e. fields that are not marked dirty even though they are) I personally think it would be fine to handle this case in a follow-up, but it would be nice to have confirmation that something like that would be accepted in a future patch. Not sure what your thoughts about this are.This is the default value, so not sure why the
setValue()
call is changed.Discussed this with @Berdir, @hchonov and a head-shaking @klausi and @berdir, @hchonov and I agreed that instead of this new interface this should be added to
ContentEntityInterface
;-)Comment #15
tstoecklerComment #16
tstoecklerSorry, just realized that I totally missed comment #8, so I duplicated some points that @hchonov brought up when we personally talked about it. Just in case anyone is (rightly) confused.
Comment #17
Wim LeersSee the related #2821077 — I left a comment at #2821077-11: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors where I'm wondering whether
ContentEntityBase::validate()
should actually be relying on the tracking we do here to only validate fields that have been modified?Comment #18
Wim LeersThis is blocking lots of things in API-first.
What are the next steps here?
Comment #19
Wim LeersBump. #18 still holds true.
Comment #20
jonathanshawReading the reviews in #8 and #14 it looks like the reviewers think the patch can be updated to take into account their concerns. (though there is slight uncertainty about #14.3).
@WimLeers Looking at it in ignorance from the outside, it looks like the ball is in your court. Can you say more about in what way the next step is not clear to you?
Comment #21
hchonovAbout #14.3
@tstoeckler, I would say it will be much better if we isolate this and introduce postCreateTranslation() instead of misusing the postCreate method which is intended to be called only after an entity is created. There is already a case in core where the concepts have been mixed-up causing some difficulties e.g. deleting an entity or only a translation will lead to calling the delete method on field items in both cases.
We already have hooks for creating and deleting translations - hook_entity_translation_insert and hook_entity_translation_delete :). For updating a translation the hook_entity_update is sufficient as there we get the entity in the translation in which the entity is being saved which over the FAPI is the entity in the language that has been edited.
Regarding entity creation we have:
Regarding entity translation creation we have:
In order to make the Entity API consistent we just have to introduce the postCreateTranslation() method on the entity.
Comment #22
hchonovActually I am still not really sure what this is about. The dirty fields have to be emptied on the new translation object? Why would they not be empty on creating a new translation for that new language code?
Comment #23
Wim LeersIS updated to clarify which issues are blocked on this.
Comment #24
Wim Leers#21:
This sounds reasonable.
@tstoeckler, do you want to proceed with that?
Comment #25
Wim LeersAlso, since this was split off from #2456257, and I just rerolled that patch. It failed in a new way. Quoting #2456257-89: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work:
(I'd forgotten for a moment that this issue must go in first, i.e. that we had split off the Entity Field API-related changes to this issue.)
So, reuploading this patch (and rebasing at the same time), to see if that new test also fails for this patch.
Comment #27
tstoecklerRe #24: I don't think I have time to work on this patch myself at the moment. If I can allocate some time I will try to tackle the first part of #14 first to basically put this patch in a bit more context as that would be helpful in assessing the benefit and risk of this patch.
Comment #28
hchonov@WimLeers, could you please explain #22?
Comment #29
Wim Leers#25 failed as expected, great! :)
#27: okay!
#28: I wrote this >2 months ago, so I don't have a full explanation available. It was introduced in #2456257-74: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work, based on feedback by @tstoeckler and my prior experience/attempts. The best I can muster:
$this->dirtyFields
tracking needs to be reset for the given language after creating a new entity object (which can be either loading an entity at all, or loading a translation of the entity)postCreate()
is not yet being called inContentEntityStorageBase::createTranslation()
, so I added a::postCreate()
call there$this->newRevision = TRUE
remains unconditional as it is in HEAD, then tests fail, because in HEADpostCreate()
was only called for loading an entity the first time, now it's being called also when creating an entity translation objectThis is why #21's proposal of adding
postCreateTranslation()
sounds sensible to me (without backing it up with facts, just from a superficial POV), because it allows us to not need to do things conditionally inpostCreate()
.Does that help?
Comment #30
catchThis is closely linked to the 'revision_translation_affected' functionality added by #2453153: Node revisions cannot be reverted per translation which I don't see mentioned here.
Comment #31
Wim LeersComment #32
Wim LeersWe just discussed this in an
call. The consensus is that this is major in and of itself, because it's an important gap in the Entity/Field API.However, it's also a highly risky change. Because there are so many edge cases where this needs to work correctly: revisions, translations, unserializing, and it must work correctly for all field types. So this issue will need a lot of time.
@tstoeckler has recommended that the issues that need this functionality use
\Drupal\Core\Field\FieldItemListInterface::equals()
for now. That's also used by "foundational" Entity API functionality, including by\Drupal\Core\Entity\ContentEntityStorageBase::hasFieldValueChanged()
to optimize SQL writes when saving entities, so if anything is broken in there, then it'd be critical. Therefore #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior should be solvable using that. And we'd just choose to postpone #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work until this lands, and accept the code smell and negative consequences for contrib modules.Comment #33
timmillwoodI don't like the term "dirtyFields", what's dirty about them? I had to ask what it meant. How about "changedFields" or "updatedFields"?
Comment #34
Wim Leers"dirty" is the term used in many many other libraries and frameworks.
Comment #35
timmillwoodOk, maybe I just need "get off the island" and learn new terminology.
Comment #36
Wim LeersWell, there's lots of questionable things shared across many cultures, this may very well be one of them :) I kinda agree with you, but yeah — do we want Drupalisms or follow the perhaps questionable common naming pattern?
Anyway, the name is something we can bikeshed when this is actually working :)
Comment #37
BerdirAlso, the problem with changed is that the fields might not have actually *changed*, maybe the same value was set again that was there before. So I think getChangedFields() would imply a functionality that doesn't actually exist.
(yes, onChange() is also not correct in that sense, but onDirty or onDirtyingField() is kinda... dirty ;))
Comment #38
plachThis is a "touching" issue ;)
Comment #39
tstoecklerWell that is not actually clear to me, to be honest. I had always thought that as part of tracking which fields are dirty we compare the to-be-set value with the current one and if they are equal, the field is not dirty? So basically my personal long-term plan was that once we have this API that at some point in the future that
ContentEntityBase::hasTranslationChanges()
could basically become super cheap, i.e.or something. And for that it would be important to track whether the field values actually changed. But maybe (apparently?) that's not what others are thinking? I would be interested to hear other ideas.
Comment #40
BerdirAt least that's not what the patch is doing as far as I see and the only way to do that through onChange() would be to change all the field implementations to only call onChange() if they actually detect a change, which would be a BC change in theory (not sure if it would actually break something) and require changes in many field types.
So yes, that would be an interesting thing to have but I'm not sure how we could actually make that work :)
Comment #41
Berdir$notify defaults to TRUE, why is this change necessary?
Interesting, we've had quite some discussions already about what should and shouldn't be called per-translation.
See #2577609: Add per-entity-translation pre/post save methods (and hooks?) so that preSave() and postSave() implementations don't need to iterate all translations.
There are bugs by not doing it, but I fear that adding this might also introduce new/different bugs.
I only found two postCreate() implementations in all my entities and they're both not translatable, so I have no specific example of what could change.
what we've done before is just add those methods to ContentEntityInterface with docs to move up in 9.x, but maybe only on issues that weren't committed yet because I can't find an example right now. (the argument is that fieldable interface is a generic interface that might have other implementations out there, while ContentEntityInterface is a 1:1 interface for ContentEntityBase)
Comment #42
Berdir#1926714: Track changes for entity property changes and make available to hooks is also related, maybe we can close that as a duplicate now. It clearly is very old :)
Comment #43
hchonovThere is an issue about the problems with on change -
#2863818: Fix notification issues in the entity/field on-change system.
Comment #44
catch#38 seems to be suggesting 'touched' fields as opposed to 'dirty' fields, seems like a good suggestion: (https://en.wikipedia.org/wiki/Touch_(Unix))
Comment #45
plach@catch:
Yep, sorry for being cryptic :)
Comment #46
plachBtw, I share @tstoeckler's perplexity about the usefulness of this patch as it is currently. If I understand it correctly a regular entity form submission would mark every field for which we have a widget as dirty/touched/younameit, regardless of it being actually modified. Is this going to be used only for REST? I guess it couldn't be used to build a proper conflict management solution supporting merging of non-conflicting changes. If so, we will need another issue and a similar API to detect actual field changes. However I suppose that once we have the latter, that can address also the use cases for which we are introducing the former.
That said, I'm wondering whether the only reason to keep the current approach is that it's way easier to implement and it will allow to unblock/fix the issues in the OP way more quickly.
Comment #48
Wim LeersToday I ran into https://www.w3.org/TR/2011/WD-html5-20110525/the-input-element.html#conc...
So this is the name used truly everywhere, even in the HTML spec!
(I hope this helps address #33.)
Comment #51
hchonovComment #52
Wim LeersThe issue summary was still saying this blocks #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior. This used to be true originally, but an alternative solution was found. The issue summary is now once again accurate.
Comment #54
geek-merlinGreat API addition. And: I share the point made in #37 / #39 / #46 that the current approach (back-and-forth changes that result in no change are still marked dirty) does not seem useful.
Why not simply do a sophisticated version of
?
Comment #57
johnwebdev CreditAttribution: johnwebdev commentedThis would still be a really useful API addition. Let's begin with a reroll.
(comments triage)
Comment #59
johnwebdev CreditAttribution: johnwebdev commentedThis patch:
I did a small comment triage in #57, that I intend to work more on later.
Comment #60
johnwebdev CreditAttribution: johnwebdev commentedSetting back to NW. This new patch adds more test coverage, ::testDirtyFields is expected to fail (due to suggested change in #37, #39, #46), so leaving at NW for now.
So while writing tests, I finally understood why ::postCreate was implemented here.
::addTranslation calls ContentEntityStorageBase::createTranslation, which calls Entity::getTranslation() that returns the new translation object. However, ::createTranslation then sets new values on that newly created object which gets stored in dirtyFields array. So the current solution works around that by, emptying the array again.
Comment #62
geek-merlin(Needed a while to find this again, so added googlefood)
Comment #63
geek-merlin