Cleanup in hook_field_attach_*()
yched - November 3, 2009 - 22:47
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | API clean-up |
Description
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...
#1
- 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.
#2
The last submitted patch failed testing.
#3
Small copy/paste error, that one should be better.
#4
10712 passes is not enough, problem with slave #44. Retesting.
#5
The 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
$objectdata structure before storage write.There are some minor string issues:
+++ modules/field/field.api.php 4 Nov 2009 00:53:57 -0000@@ -1364,6 +1288,151 @@
+ * Possible use cases include : per-bundle storage, per-combo-field storage...
White space before the semicolon, should be: "include: per-bundle".
+++ modules/field/field.api.php 4 Nov 2009 00:53:57 -0000@@ -1364,6 +1288,151 @@
+ * - Loaded field values are added to $objects. Fields with no values should be
The comment does not wrap at column 80.
+++ modules/field/field.api.php 4 Nov 2009 00:53:57 -0000@@ -1364,6 +1288,151 @@
+ * This hook allows modules to store data before the Field Storage
+ * API, optionally preventing the field storage module from doing so.
The comment wraps before column 80.
+++ modules/field/field.api.php 4 Nov 2009 00:53:57 -0000@@ -1364,6 +1288,151 @@
+ * This hook allows modules to store data before the Field Storage
+ * API, optionally preventing the field storage module from doing so.
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?
#6
Fair enough, I'll update the patch asap.
Can this use hook_field_attach_presave, then ?
#7
Well, we need to distinguish between insert and update, I don't seem to find a way to do it in the presave hook.
#8
Fixed 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.
#9
@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:
<?php
public function prepareRevision($langcode) {
// Load into $stored_entity the field values as currently stored.
$stored_entity = clone($this->entity);
field_attach_load($this->entityType, array($this->getEntityId() => $stored_entity));
list(, , $bundle) = entity_extract_ids($this->entityType, $this->entity);
foreach (field_info_instances($this->entityType, $bundle) as $instance) {
$field_name = $instance['field_name'];
$field = field_info_field($field_name);
if ($field['translatable']) {
// For each translatable field copy into the wrapped entity the field
// values as currently stored, except the ones having language equal to
// the one currently being edited. This way every time a new revision is
// created for a certain language all of its translations get a new
// revision.
foreach ($stored_entity->{$field_name} as $stored_langcode => $items) {
if ($stored_langcode != $langcode) {
$this->entity->{$field_name}[$stored_langcode] = $items;
}
}
}
}
}
?>
If we execute the code above after storage write, tranlsation revisions are not saved.
#10
OK - 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.
#11
Sorry, I missed this one :(
+++ modules/field/field.api.php 5 Nov 2009 11:09:47 -0000@@ -1364,6 +1288,151 @@
+ // The logic for determining last_comment_count is fairly complex, so
+ // call _forum_update_forum_index() too.
The comment does not wrap at column 80.
This review is powered by Dreditor.
#12
No, this is correct.
// The logic for determining last_comment_count is fairly complex, so call// ...
would wrap at 81 ;-)
#13
string-issue-mania!
#14
We'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.
#15
Also 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.
#16
re 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 :-(
#17
Side 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.
#18
Hm. Ok. Since this is close enough to when I rolled UNSTABLE-10, hopefully no one will notice. :P~
Committed to HEAD.
#19
@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?#20
@yched:
Nevermind, I found another way. Still worried about the name of f_a_presave(), though.
#21
Unfortunately 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.
#22
Maybe another API cleanup issue, then?
#23
Crossposted with #19 - OK, glad you found a workaround.
I opened #627626: Node-specific code for the 'is_new' in filefield.
#24
re #22, possibly in theory, although I'd stand for presave. Anyway, *that* change would be too late for this cycle ;-)
#25
Automatically closed -- issue fixed for 2 weeks with no activity.