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.FieldItem::insert() and FieldItem::update() are left-overs of the previous field.module API, but do not make much sense in the entity context as insert/update are not post-storage operation as they are usually for entities, but pre-save operations as well.
Let's fix that by making use of $entity->isNew() as in the other entity preSave() method callbacks also.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | remove-methods-2023473-5-interdiff.txt | 655 bytes | Berdir |
| #5 | remove-methods-2023473-5.patch | 8.72 KB | Berdir |
| #3 | remove-methods-2023473-3.patch | 8.69 KB | Berdir |
Comments
Comment #1
fagoComment #2
BerdirComment #3
BerdirHere we go. Looks like we don't have many implementations for those ;)
Comment #5
BerdirAnother try.
Comment #6
amateescu CreditAttribution: amateescu commentedLooks great! I'll let the testbot disagree :)
Comment #7
yched CreditAttribution: yched commentedWorks for me too :-)
Comment #10
BerdirTestbot disagrees. And shows the problem. We don't have an entity ID in preSave()...
Comment #11
BerdirI fear that means won't fix for this issue?
Comment #12
yched CreditAttribution: yched commentedHm, doesn't it mean we should rename preSave() to save(), and invoke it at the place where we currently invoke insert() / update() ?
Comment #13
amateescu CreditAttribution: amateescu commentedIs there something wrong with using postSave() that I'm missing? :)
Comment #14
Berdirthat we don't have postFix() but insert and update. But it's inconsistent anyway, we either follow the hook pattern, which is update/insert or the entity class method name, which is postSave(), with an argument for update/insert, but then we'd need to pass that down..
preSave() might be necessary too? Especially as you don't know if it's update or insert without an additional argument. $entity->isNew() no longer works.
Comment #15
fagoyeah, I think just one save() isn't enough - so imo a preSave() + a save() would make sense. It's not postSave() as this is considered to be part of saving - right? :-)
Thus, I'd go with
- preSave() as it's there
- save($update = TRUE) instead of insert/update
?
Comment #16
BerdirWell, we also call it postSave() on entities (we couldn't call it save() there)..
And naming it the same would make sense to me, because it's the same. We could pass the calls through Entity::$method() too, just like we already pass it through the list for the items.
Comment #17
fagoSo this means you see postSave() as part of the save operation, right?
As noted in #1729812: Separate storage operations from reactions it's right now used for both, partly it's for saving and partly for reacting upon the save. Imho, the naming suggests it's already after the save operations.
But yeah, we cannot call it just save() on the entity class. It's more like a "save event", so what about "onSave()" ? You are writing code, that says when it is saved, also save this and that... - aka on save, do that. Then, after saving you might want to clear some caches, etc. - aka post save, do that.
Thinking about it, it's actually the same for fields. The default save operation of fieldswon't happen in FieldItem::save() either (but in the entity storage controller), so onSave() could work there also.
Comment #18
BerdirSo, #2478459: FieldItemInterface methods are only invoked for SQL storage and are inconsistent with hooks is changing when those methods are called, and as a result, I think they would become useful to have. I think this issue will become a won't fix after the other one, but we could use feedback/review from @fago/@yched there :)
Comment #19
mkalkbrennerI was looking for a "postSave" for fields. @alexpott pointed me to 'insert' and 'delete'. But as I had a closer look on these methods we decided to open #2478459: FieldItemInterface methods are only invoked for SQL storage and are inconsistent with hooks.
In other words, there are use-cases for 'insert' and 'update' if they're called after save - like the corresponding hooks.
Comment #20
plachTentatively closing this in favor of #2478459-46: FieldItemInterface methods are only invoked for SQL storage and are inconsistent with hooks.