function hook_field_attach_load($entity_type, &$entities, $age, $options)

$entities shouldn't be passed by reference there (because module_invoke_all doesn't pass the argument by reference?). Instead, the hook implementation should just return the $entities array after making changes to it.

This documentation error might also be found in other hooks in field.api.php... I would check, but it's getting close to 1am here and I'm afraid of getting sucked in and finding myself still awake as the dawn breaks...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Component: documentation » field system

Hmmm...

I'm looking at
http://api.drupal.org/api/drupal/modules--field--field.attach.inc/functi...
and returning an array from hook_field_attach_load() won't do anything, because field_attach_load doesn't do anything with the return value from module_invoke_all().

Moving to the field system component for consideration of what should be done here...

tstoeckler’s picture

Well, $entities is in fact passed by-reference, so I don't know what should be done here. Maybe the function header in hook_field_attach_load() could be a bit more explicit about that.

jhodgdon’s picture

It is passed by reference? I thought module_invoke_all couldn't pass things by reference?

Anonymous’s picture

Anonymous’s picture

I have an implementation in my module. It adds something to the $items variable in each entity. I then return the entities.

function microdata_field_attach_load($entity_type, $entities, $age, $options) {
  ...
  foreach ($entities as &$entity) {
    ...
    foreach ($entity->{$field_name}[$entity->language] as &$item) {
      $item['#microdata_attributes'] = microdata_mapping_to_attributes($field_mapping);
    }
  }

  return $entities;
}

This is working and I have access to the #microdata_attributes in the field formatter.

jhodgdon’s picture

Status: Active » Postponed (maintainer needs more info)

Um. I don't get it. field_attach_load() is definitely just throwing away the return value of its module_invoke_all. ???

Anonymous’s picture

Ok, so this is weird.

I don't actually need to return, but I can't use pass by reference in the function signature. It just gets magically passed by reference. So tstoeckler was right.

How would one document that?

jhodgdon’s picture

Status: Postponed (maintainer needs more info) » Needs work

When all else fails, read the code. And the doc. :)

So. module_invoke_all() calls PHP's call_user_func_array().

http://us3.php.net/call_user_func_array says:
"Referenced variables in param_arr are passed to the function by reference, regardless of whether the function expects the respective parameter to be passed by reference. This form of call-time pass by reference does not emit a deprecation notice, but it is nonetheless deprecated, and will most likely be removed in the next version of PHP. Furthermore, this does not apply to internal functions, for which the function signature is honored. Passing by value when the function expects a parameter by reference results in a warning and having call_user_func() return FALSE (does not apply if the passed value has a reference count = 1)."

Which is a bit confusing, but that does explain it:
- You can't put & in the function signature.
- Nonetheless, in the current PHP, everything is passed by reference.
- This functionality may be removed in a future version of PHP, thereby breaking stuff.

So we should probably document this:
- in module_invoke_all(), put in a note that all params are passed by reference to the hook functions
- same for module_invoke(), node_invoke(), user_module_invoke(), etc. [after verifying that they also use call_user_func_array(), which most of them do I think?]
- remove the & in the function signature for hook_field_attach_load()... but I would say not for other hooks in general, without checking them, because not all hooks are invoked with one of the *invoke* functions... so let's just stick with removing the & in hook_field_attach_load for now?
- Add a note to the Hooks topic (http://api.drupal.org/api/drupal/includes--module.inc/group/hooks/7) to say that most hooks are invoked via module_invoke_* and therefore have their args passed by reference.

How's that for a plan?

jhodgdon’s picture

Status: Needs work » Active

wrong status, sorry!

pillarsdotnet’s picture

See test program and output posted at #1192178-8: The user_module_invoke() function lacks documentation of parameters.

Also, objects are always passed by reference; arrays and scalars are not.

Anonymous’s picture

I'm pretty sure that $entities (which is passed by reference) is an array of objects.

pillarsdotnet’s picture

So the objects in the array are passed by reference. The array itself is not. Try adding a new element to the $entities array and you'll discover that the calling function won't see it.

jhodgdon’s picture

Ah, well maybe that's because module_invoke_all() doesn't take its ... args by reference.

OK. So the array is by value, but yes objects are always references in PHP, so if you modify an object you've modified it globally.

So *within* module_invoke_all, probably you can add array elements and the next module's implementation would get them, but then they're not returned to the caller of module_invoke_all.

And we don't need to document that the objects in the array can be modified, because that is just PHP's object behavior.

So what we do still need to do is remove the & in the hook function signature, because that will not work.

pillarsdotnet’s picture

FileSize
165 bytes
482 bytes

So *within* module_invoke_all, probably you can add array elements and the next module's implementation would get them

Sorry, but it doesn't work that way, at least not in the current PHP 5.5.0-dev (cli) (built: Jun 20 2011 19:39:55).

pillarsdotnet’s picture

FileSize
234 bytes
468 bytes

Not even if you define/test the $param_arr parameter directly.

And yes, in D8 (which requires PHP 5.3), no function that is ever called through module_invoke() should contain an argument passed by reference. Doing so is a sure recipe for failure.

Also see #20 and #36 of #353494: Remove node_invoke(), comment_invoke(), etc

jhodgdon’s picture

OK. Whatever. Anyway, module_invoke_* doesn't take its args by reference, so the whole question of what it does internally is rather academic, and not relevant to this issue. What's relevant to this issue is that we can't have hooks that are invoked through module_invoke_* have & in their function signature, because it causes a PHP warning or error or at least a notice. And the hook shouldn't count on anything it does affecting the arguments directly. So we need to remove one & from the hook here, if someone would please make a patch.

pillarsdotnet’s picture

Title: hook(s) in field.api.php pass by reference when they shouldn't » Change &$entities to $entities in hook_field_attach_load() and hook_field_storage_load() docs.
Component: field system » documentation
Status: Active » Needs review
FileSize
1.42 KB

Apparently this is a documentation-only bug. I searched through all of Drupal core for implementations of field hooks called by module_invoke() or module_invoke_all(). Then I searched the function signatures for ampersand & characters. I found only two matches, both in the field.api.php documentation file:

  • function hook_field_attach_load($entity_type, &$entities, $age, $options) {
  • function hook_field_storage_load($entity_type, &$entities, $age, $fields, $options) {

Patch attached.

pillarsdotnet’s picture

I suppose this should be broken into two separate issues/patches, one for each function signature.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Please don't split the issue, so that we can have two one-line patches. The original report mentioned there could be other spots. Splitting it would be extremely silly IMO.

8.x/7.x please...

catch’s picture

Issue tags: +Needs backport to D7

Just to clarify on why this works, in PHP5, all modules are passed by resource (not quite the same as by reference). So if you pass an array of objects, each object can be acted on directly, but the array itself cannot (i.e. unset() or adding items won't work).

The docs patch is right by itself, but this issue is missing the backport tag.

pillarsdotnet’s picture

in PHP5, all modules are passed by resource

I assume that you meant "all objects" rather than "all modules", correct?

catch’s picture

Yes sorry, all objects.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x and 7.x. Thanks!

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