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...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
Issue tags: +API clean-up
FileSize
20.03 KB

- 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
20.03 KB

Small copy/paste error, that one should be better.

yched’s picture

10712 passes is not enough, problem with slave #44. Retesting.

plach’s picture

Status: Needs review » Needs work

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 $object data 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 replace module_invoke_all().

I'm on crack. Are you, too?

yched’s picture

Fair enough, I'll update the patch asap.

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

Can this use hook_field_attach_presave, then ?

plach’s picture

Can this use hook_field_attach_presave, then ?

Well, we need to distinguish between insert and update, I don't seem to find a way to do it in the presave hook.

yched’s picture

Status: Needs work » Needs review
FileSize
20.03 KB

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.

plach’s picture

@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.

yched’s picture

Title: Add missing hook_field_attach_[insert|update]() hooks » Cleanup in hook_field_attach_*()

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.

plach’s picture

Status: Needs review » Needs work

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.

yched’s picture

Status: Needs work » Needs review

No, this is correct.

      // The logic for determining last_comment_count is fairly complex, so call
      // ...

would wrap at 81 ;-)

plach’s picture

Status: Needs review » Reviewed & tested by the community

string-issue-mania!

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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.

amitaibu’s picture

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.

yched’s picture

Status: Needs work » Reviewed & tested by the community

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 :-(

yched’s picture

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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Hm. Ok. Since this is close enough to when I rolled UNSTABLE-10, hopefully no one will notice. :P~

Committed to HEAD.

plach’s picture

@yched:

Dangerous mix - this is also a wink at plach for #539110: TF #4: Translatable fields UI.

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 to field_attach_pre_save() as the other functions in here?

plach’s picture

@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?

Nevermind, I found another way. Still worried about the name of f_a_presave(), though.

yched’s picture

Is having an empty id from entity_extract_ids() enough to determine if an entity is being created or updated?

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.

plach’s picture

presave / pre_save: the rest of core uses presave: hook_node_presave(), hook_comment_presave(), f_a_presave() follows that pattern.

Maybe another API cleanup issue, then?

yched’s picture

Crossposted with #19 - OK, glad you found a workaround.

I opened #627626: Node-specific code in file field for the 'is_new' in filefield.

yched’s picture

re #22, possibly in theory, although I'd stand for presave. Anyway, *that* change would be too late for this cycle ;-)

Status: Fixed » Closed (fixed)
Issue tags: -API clean-up

Automatically closed -- issue fixed for 2 weeks with no activity.