Removing terms

It looks like it should be a pass-by-reference. See http://api.drupal.org/api/function/taxonomy_term_load_multiple/7 for the calling function, and http://api.drupal.org/api/function/taxonomy_test_taxonomy_term_load/7 for a sample implementation (it looks to me as though that hook is not implemented anywhere in core).

The same problem exists in hook_taxonomy_vocabulary_load() in the same file. There are no implementations of this at all on api.drupal.org, but the way it is called suggests it should be passed by reference too -- see http://api.drupal.org/api/function/taxonomy_vocabulary_load_multiple/7

The attached patch fixes both of them (I think).

As I already said in the other issue, this is not necessary, atleast not for this example.

The thing is that the objects inside the array are always passed by reference because they are objects. This means that the example works.
Another example of that is http://api.drupal.org/api/function/field_sql_storage_field_storage_load/7 ($objects is an array of objects). Actually, I had to remove the & there, because it generated a warning with PHP 5.3. This would not happen here though, because it is not invoked with call_user_func_array().

What you can't do is adding vocabularies to the array or remove some of them. Not sure if this is intended or not. And you can still add a & if you need to do that for whatever reason.

Berder: You said "As I already said in the other issue, this is not necessary, atleast not for this example." -- Which other issue?

Anyway, that aside... My PHP in a Nutshell book says (p. 58): "As of PHP 5, objects are passed and assigned by reference by default... This was different before PHP 5 -- obects were treated like other types of variables and copied entirely when assigned." So what you are saying about objects is correct for Drupal 7 (which requires PHP 5) but not for Drupal 6 and before. I guess this issue is Drupal 7 only, though, since there doesn't appear to be any equivalent to hook_taxonomy_term_load in Drupal 6 (there is hook_taxonomy, but it does not appear to have a 'load' operation).

In this case, however, it is an array and not an object that is being passed into the hook, I think? It is coming from
$queried_terms = $query->execute()->fetchAllAssoc('tid');
in http://api.drupal.org/api/function/taxonomy_term_load_multiple/7 and it looks like fetchAllAssoc() is making an associative array.

Assuming I've got that right, the question becomes whether your statement is true for arrays (that they are passed by reference by default). I don't think they are.... I couldn't find doc saying this was the case as I found for Objects (via web search or in my Nutshell book), so I wrote a quick and dirty little test script. I tried it with numeric and associative arrays -- same result: they are not passed by reference by default in PHP 5.

Here's the script, for reference:

function a_noref( $a ) {
  $a['x'] = 'hi';
  $a['y'] = 'ho';
}

function a_ref( &$a ) {
  $a['x'] = 'hi';
  $a['y'] = 'ho';
}

$arr = array();
$arr['x'] = 'hello';
a_noref( $arr );

echo 'Noref: count ' . count( $arr ) . "<br />\n";
echo 'Noref: arr elem x: ' . $arr['x'] . "<br />\n";

$arr = array();
$arr['x'] = 'hello';
a_ref( $arr );

echo 'Ref: count ' . count( $arr ) . "<br />\n";
echo 'Ref: arr elem x: ' . $arr['x'] . "<br />\n";

Here is the output:

Noref: count 1
Noref: arr elem x: hello
Ref: count 2
Ref: arr elem x: hi

So it looks like arrays are not passed by reference. This was tested using PHP 5.

Hmmm... I guess you are also saying that the objects within the array are passed by reference, so that they can be altered directly...

Well, I don't know what the right answer is. If we should omit the &, then it should also be omitted from the sample module http://api.drupal.org/api/function/taxonomy_test_taxonomy_term_load/7 -- and you are probably right that that particular function does not alter the array itself, and that the object references in the array would/should be successfully modified (in PHP 5) without the & in the function declaration.

I sit corrected: Passing an array of objects in PHP5, you are correct to point out that the objects will be modified directly:

function a_obj( $a ) {
  foreach( $a as $obj ) {
    $obj->bar = 'hi';
  }
}

$arr = array();
$arr[0] = new Foo;
$arr[0]->bar = 'hello';

a_obj( $arr );
echo 'Obj: arr elem 0 bar: ' . $arr[0]->bar . "<br />\n";

Output:

Obj: arr elem 0 bar: hi

So I think we should instead patch the sample implementation to remove the & and maybe make a note in the hook doc that you don't need to pass by reference unless you wanted to add/remove terms from the array. Thoughts?

Sounds good. the "other issue" is #471746: hook_taxonomy_vocabulary_load missing return or reference?.

I'd also like to get catch's opinion on this...

The hook documentation shouldn't have the & in it at all I think. I'm not sure about letting people alter the array here - would rather leave it undocumented and let people do evil things with the references if they really, really want to.

OK. We should therefore remove the & from http://api.drupal.org/api/function/taxonomy_test_taxonomy_term_load/7

I also think the example in the hook doc is written badly -- it looks like it could be adding things to the array. And we can document the passing by reference thing so that this question doesn't come up.

I'll make a patch.

I don't think we should document the passing by reference thing here - there are a lot of hook in D7 which pass an array of objects without an explicit by ref and we'd need to do that one by one. Maybe add it to coding standards or upgrade guide?

hook documention should just act on the object rather than worrying about the array key though yeah.

I think this patch improves the doc for both of those taxonomy load hooks (the doc for the vocabulary hook was incorrect, as it didn't even mention it was receiving an array). And I've removed the & from the test module implementation of the term load hook.

Thoughts?

Setting back to Needs Review in hopes that the patch will get retested. The test bot was apparently malfunctioning (I had about 7 patches fail in that time period, all were doc, none broke the HEAD install I am pretty sure).

Hm. I'm not sure about the documentation hunks. There are many, many, many more functions we'll need to add exactly the same string about objects in PHP5. Would it make more sense to document this somewhere more central?

Should we put it in coding standards maybe? And/or should there be a PHP 5.2 (and probably PHP 5.3 support) section in the upgrade docs?

I didn't know about this behaviour until someone told me, while I was writing the patch which introduced this hook ;) but once you realise it then it's easy to remember.

The confusing thing about this particular hook is that it passes an array of objects. The array itself is NOT passed by ref in PHP 5, but the objects in the array are, and neither is in PHP 4 (i.e. Drupal 6 and eariler). So if you wanted to add items to the array (does that apply here? not sure), you'd need to make the hook implementation use a by-ref parameter, while if all you needed to do was to modify objects in the array, you'd be OK without using a by-ref parameter. In PHP 5 anyway.

I also think that this is a bit of an unusual case. Much of the other stuff in Drupal is either just single objects (like $node) being passed around, or arrays of arrays. And most of the Drupal functions return their values instead of passing by reference.

Anyway... all of that said, if you want that note taken out, I can take it out. And we can put something in the coding standards about this... how's this:

---

Heading: Function arguments passed by reference

For PHP functions that needs to return more than one value, a common practice is to make the other return values be function arguments passed by reference. For example:

   my_module_my_function( $input1, $input2, &$output1, &$output2) { 
      // Function body here, typically with a Boolean return value or no return value
      // Function modifies $output1 and $output2 in place. Doesn't modify $input1 or $input2.
   }

Because PHP 5 passes all objects by reference, you should omit the & from passed-by-reference arguments in functions (including hook implementations) in Drupal 7 and later versions, in the following cases:
* If the argument is a PHP object.
* If the argument is an array of objects, and your function will be modifying objects within the array (not adding or removing elements from the array itself).

Here is a patch that fixes up doc for the taxonomy term/vocabulary hooks, and removes the & in the one implementation, as discussed above. And it doesn't say anything about passing by reference.

I also added a new section to the module upgrade guide about PHP 5 and object passing. Review/comments requested...
http://drupal.org/update/modules/6/7#php_version

This patch fixes one more spot that was wrong in the file.

This is a straight bug report now, patch looks great.

Committed to CVS HEAD.

As noted above, if you want to remove specific terms, you can't just unset them in $terms in the hook, because it's not pass by ref.

But you can (1) change the term object in the hook, and (2) check for that change in a theme. E.g.,

Hook:

function access_changes_taxonomy_term_load($terms) {
  foreach ($terms as $index => $term_object) {
     //Some conditions being checked
       $term_object->name = '';
  }
}

Theme:

<?php foreach ($items as $delta => $item) : ?>
  <?php if ($item['#title'] != '') : ?>
    <li class="field-item <?php print $delta % 2 ? 'odd' : 'even'; ?>"<?php print $item_attributes[$delta]; ?>>
      <?php print render($item); ?>
    </li>
  <?php endif; ?>
<?php endforeach; ?>

I haven't bothered making odd/even work, since I don't use it. But it wouldn't be hard.