Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
all field_attach_*() functions have correpsonding hook_field_attach_*() hooks, but insert and update were left behind, for lack of use cases, IIRC.
#539110: TF #4: Translatable fields UI has a use case, and currently mis-uses storage 'pre' hooks field_attach_pre[update|insert]*() - which we should really rename to make it clear that they're for storage stuff...
Comment | File | Size | Author |
---|---|---|---|
#8 | field_hook_f_a_insert-622534-8.patch | 20.03 KB | yched |
#3 | field_hook_f_a_insert.patch | 20.03 KB | yched |
#1 | field_hook_f_a_insert.patch | 20.03 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commented- adds hook_field_attach_insert(), hook_field_attach_update()
- renames hook_field_attach_pre_[insert|update|query]() to hook_field_storage_pre_[insert|update|query]().
- changes the hook invocations within field.attach.inc to use module_invoke_all() for all hooks that don't need receiveng arguments by reference.
Comment #3
yched CreditAttribution: yched commentedSmall copy/paste error, that one should be better.
Comment #4
yched CreditAttribution: yched commented10712 passes is not enough, problem with slave #44. Retesting.
Comment #5
plachThe patch looks good, I also tested it with the TF4 patch and it works flawlessly. However TF4 will keep using the pre_[insert|update] hooks otherwise it won't be able to correctly handle field revisions which must be injected in the
$object
data structure before storage write.There are some minor string issues:
White space before the semicolon, should be: "include: per-bundle".
The comment does not wrap at column 80.
The comment wraps before column 80.
The comment wraps before column 80.
Edit: I'd suggest to change the issue title to reflect the changes performed on the
field_attach_X()
functions to replacemodule_invoke_all()
.I'm on crack. Are you, too?
Comment #6
yched CreditAttribution: yched commentedFair enough, I'll update the patch asap.
Can this use hook_field_attach_presave, then ?
Comment #7
plachWell, we need to distinguish between insert and update, I don't seem to find a way to do it in the presave hook.
Comment #8
yched CreditAttribution: yched commentedFixed the strings bugs noted by plach (were already present in the code that this patch moves around :-p)
re #7: well, brielfy looking at the patch in #539110: TF #4: Translatable fields UI, I couldn't find why the code needs to run before field storage is written - but I might very well have missed it.
Comment #9
plach@yched:
Were you able to read #539110-57: TF #4: Translatable fields UI? There were some issues about the Field API.
Here is an excerpt of the code attached there:
If we execute the code above after storage write, tranlsation revisions are not saved.
Comment #10
yched CreditAttribution: yched commentedOK - I'll try to chime in #539110: TF #4: Translatable fields UI.
This doesn't change the validity of this cleanup, though. We wave hook_field_attach_load() but no hook_field_attach_[update|insert].
Patch summary is in #1.
Comment #11
plachSorry, I missed this one :(
The comment does not wrap at column 80.
This review is powered by Dreditor.
Comment #12
yched CreditAttribution: yched commentedNo, this is correct.
would wrap at 81 ;-)
Comment #13
plachstring-issue-mania!
Comment #14
webchickWe're past code freeze now, so unless there's a really compelling OMG PEOPLE ARE DYING reason, I'd like to not change names of hooks at this stage.
Comment #15
amitaibuAlso OG7 will gain from it. A group is now determined by checking the state of og_group fieid in any fieldable entity. Currently field_attach_pre_* are (miss)used for this.
Comment #16
yched CreditAttribution: yched commentedre webchick #15: well, hook_field_attach_pre_[insert|update]() really aim at storage manipulation.
Using them for other stuff like altering actual field values is nasty because depending on execution order, the changes might or might not be caught by storage modules that uses those hooks to store the data. When your own hook_field_attach_pre_() runs, storage might have already started. Dangerous mix - this is also a wink at plach for #539110: TF #4: Translatable fields UI.
Having authors of non-storage-related modules (99%) stare at both hook_f_a_pre_update() and hook_f_a_update() as they stand right now is not good, because they think they have a choice between two fairly similar hooks.
If you want to alter field values before they are saved, either hook_f_a_presave should be good enough, or we need new, non-storage related hook_f_a_pre_update(), hook_f_a_pre_insert() hooks that run before any storage operations begin - they can't be the current ones.
Thus we need to rename IMO :-(
Comment #17
yched CreditAttribution: yched commentedSide note: we've been wanting to do this for a long time, our bad.
Been delayed for other FiC patches which might have conflicted. Barry suggested we did that change while reshuffling #443422: 'per field' storage engine, but the patch was big enough and I preferred not to kill that kitten there.
Comment #18
webchickHm. Ok. Since this is close enough to when I rolled UNSTABLE-10, hopefully no one will notice. :P~
Committed to HEAD.
Comment #19
plach@yched:
Ok, I'll try to use hook_f_a_presave(). Is having an empty id from entity_extract_ids() enough to determine if an entity is being created or updated?
Edit: Btw, shouldn't
field_attach_presave()
(and the corresponding hook) be renamed tofield_attach_pre_save()
as the other functions in here?Comment #20
plach@yched:
Nevermind, I found another way. Still worried about the name of f_a_presave(), though.
Comment #21
yched CreditAttribution: yched commentedUnfortunately no. node_save() allows you to specify the $node->nid of the node you're *creating*. It uses the $node->is_new flag to determine if it's a create or an update. And that behavior is not consistent across other entities. user_save() works the same, but not comment_save() or taxonomy_term_save() (no $obj->{id_key] means 'create').
That lack of consistency might be a problem, BTW - I just found out that file field has some code relying on $object->is_new, which is not a standard pattern across entities.
presave / pre_save: the rest of core uses presave: hook_node_presave(), hook_comment_presave(), f_a_presave() follows that pattern.
Comment #22
plachMaybe another API cleanup issue, then?
Comment #23
yched CreditAttribution: yched commentedCrossposted with #19 - OK, glad you found a workaround.
I opened #627626: Node-specific code in file field for the 'is_new' in filefield.
Comment #24
yched CreditAttribution: yched commentedre #22, possibly in theory, although I'd stand for presave. Anyway, *that* change would be too late for this cycle ;-)