See: http://api.drupal.org/api/drupal/modules--field--field.api.php/function/...

This function must pass a referenced parameter so that changes can propagate to the caller.

When I create "my" module and add "my" hook, I have something like this.

<?php
function my_field_read_field(&$field) {
  // do nothing at this time
}
?>

I get a lot of messages like:
Warning: Parameter 1 to my_field_read_field() expected to be a reference, value given in module_invoke_all() (line 817 of includes/module.inc).

On line #561 of field.crud.inc, the following is specified:

<?php
module_invoke_all('field_read_field', $field);
?>

I would think drupal_alter() needs to be used here so that the $field parameter may be altered:

<?php
drupal_alter('field_read_field', $field);
?>

However, using alter here would break API, so there needs to be a way to ensure that the $field parameter gets passed as a reference or else the hook_field_read_field() does nothing.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thekevinday’s picture

Version: 7.0-rc4 » 7.0
FileSize
776 bytes

To prevent breaking existing api, I made this patch to accept a return value and merge that value into the field array.

bfroehle’s picture

Status: Active » Needs review
FileSize
712 bytes

thekevinday is correct that hook_field_read_field has $field passed by reference.

This patch replaces

    module_invoke_all('field_read_field', $field);

with

    foreach(module_implements('field_read_field') as $module) {
      $function = $module . '_field_read_field';
      $function($field);
    }
thekevinday’s picture

Status: Needs review » Reviewed & tested by the community

#2 patch makes more sense.

The patch applies and functions as expected.

yched’s picture

Version: 7.0 » 7.x-dev
Category: bug » feature
Status: Reviewed & tested by the community » Needs review

Not just yet, please.

At the very least, field_read_instances() needs the same change if we go that way.

More importantly, altering fields and instances definitions is something that was intentionally left out for the API - as such, this qualifies as a feature request. The consequences are not clear, and the cycle between load / alter / update (including the altered data ?) is not trivial.

bfroehle’s picture

Issue Summary

The API for hook_field_read_field() is

/**
 * Act on field records being read from the database.
 *
 * This hook is invoked from field_read_fields() on each field being read.
 *
 * @param $field
 *   The field record just read from the database.
 */
function hook_field_read_field(&$field) {
  // @todo Needs function body.
}

which describes $field as being passed by reference. However in field_read_fields() it is called via module_invoke_all('field_read_field', $field); which is limited to passing by value. The proposed patch in #2 manually invokes all modules implementing field_read_field so that $field may be passed by reference.

(Alternatively, the API could be updated to describe $field as being passed by value. I'm not a Field API guru, so I'll leave it to others to make the decision here.)

bryancasler’s picture

subscribe

bfroehle’s picture

Title: hook_field_read_field reference parameter problem » hook_field_read_field() reference parameter problem
Category: feature » bug
FileSize
430 bytes

More importantly, altering fields and instances definitions is something that was intentionally left out for the API - as such, this qualifies as a feature request. The consequences are not clear, and the cycle between load / alter / update (including the altered data ?) is not trivial.

In the near term, I suggest changing the API definition to indicate that the $field variable is not passed by reference.

After this is committed, the issue could be reopened to discuss the merits of allowing the field to be altered.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Agreed with #7.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Committed #7 to CVS HEAD /D7 so marking this back as 'needs work'.

bfroehle’s picture

Title: hook_field_read_field() reference parameter problem » hook_field_read_field() should pass $field by reference (so it can be altered)
Category: bug » feature
Status: Needs work » Active

Setting back to 'feature request' and 'active' so it's in the right queue.

It might be interesting and useful to hear from thekevinday about the specific use case here.

thekevinday’s picture

For starters I am wondering where do you store the data you actually load during this function call.
As it is now, with the recent changes, the function neither accepts return statements nor accepts modifying the parameters.
Where exactly is the data to be placed once its loading? /dev/null?
I must be missing something.

The use case:

I need to replace the existing cardinality functionality by using module+hooks instead of hacking drupal core.
I am altering the field cardinality "add another item" button to change its text based on 3 possible choices:
1) default drupal behavior: "Add another item"
2) button field label: "Add another %field_label"
3) custom/manual text: "Hello World"

Using the field api, I am able to do just this, with one exception: this function.
My additional data needs to be loaded when the field is loaded.
"Act on field records being read from the database." is exactly what I need to do.
That is, I need to act on the loaded data and append the additional information to the fields array.

My "hook_form_alter() for field_ui_field_edit_form" function should then be able to access this data from $form['#field'] (such as: $form['field']['add_another']['button_mode'])

Having the data loaded in this way should be more scalable as this database call happens only when hook_field_read_field() is called, any/all caching should be handled by core, and module integration should be easier.

fgm’s picture

Title: hook_field_read_field() should pass $field by reference (so it can be altered) » hook_field_read_field() should pass $field by reference (so it can be altered) or even be removed
Version: 7.x-dev » 8.x-dev

I noticed that this hook has currently no implementation in core at all.

Suspecting it might be needed by alternative field_storage implementations, I checked mongodb_field_storage, and it doesn't use this hook either.

Maybe the best would be to actually remove it ? Switching to 8.x because it could possibly break (contrib) implementations depending on it and does not appear to cause problems with core since it has no implementation.

Anonymous’s picture

I just asked on Twitter what the use case for this is because it isn't clear to me.

We should either remove this or figure out an example for api.drupal.org.

Anonymous’s picture

Title: hook_field_read_field() should pass $field by reference (so it can be altered) or even be removed » hook_field_read_field() and hook_field_read_instance() should pass by reference (so it can be altered) or even be removed

and whatever is decided should probably also be applied to hook_field_read_instance()

swentel’s picture

Status: Active » Closed (duplicate)

Actually you can now, as hook_entity_load operations are run anyway and act on objects.

This hook is also being removed in #2013939: Remove hook_field_[CRUD]_() in favor of hook_ENTITY_TYPE_[CRUD]() calls

No backport to D7 though as it's an API change.