Converting taxonomy vocabulary to required can cause PHP errors in watchdog

/**
 * Implementation of hook_nodeapi('update_index').
 */
function taxonomy_node_update_index(&$node) {
  $output = array();
  foreach ($node->taxonomy as $term) {
    $output[] = $term->name;
  }

If I were coding it, I'd just make the code more robust, and shut it up.

  foreach ((array)$node->taxonomy as $term) {

I can confirm that substituting this code

foreach ((array)$node->taxonomy as $term) {

solved the following problem: Invalid argument supplied for foreach() in /home/mydir/mysite.org/modules/taxonomy/taxonomy.module on line 1226.

Same as in #4 here. We should really be checking if something is empty before using it as an array. The typecasting works fine in this case, though.

The same from #4 applies for me. Although i usually never change code in drupal core modules, i've made an exception for this one, because it seriously pissed me off that every time cron was run, there was a big errorlist of "Invalid argument supplied for foreach()" .

I hope this minor change is included in the next taxonomy update (if there's one on the way).

thanks dman.

+1... just got bit by this... there should be a check here, for non-array passed for whatever reason, to at least fail gracefully.

Adding (array) solves this cleanly if not beautifully.

It's true what you're saying. The data that doesn't pass an array as all the data is supposed to do, still won't pass an array on the next occasion.
Although i don't think that by adding the (array) to it, we will miss out on any errors or warnings because it was simply not catched.

A nice official fix for this problem would be better of course.

It comes down to programmers pragmatism.
*I* believe that an API that is an advertised interface with external unregulated code should be able to protect itself against incorrect input or at least identify where the problem is coming from so it can be fixed. You call PHP core functions with the wrong number or type of arguments, then it tells you what you did wrong. We can do the same.
The Drupal API (in D6) does not have any of those safeguards built in (assertions etc) anywhere and has tended to fall into the "don't call API functions unless the input is perfect and we'll hope that core won't die" sort of approach.

Building code where you can assume that nobody ever makes mistakes is nice, but I find it fragile.

*I* find it more interesting to built fault-tolerant code that helps bad coders find out what they are doing wrong (notices) but doesn't cause failure if there is a small problem.

In this case (I've seen it, or something very similar elsewhere) it's really hard to find out where in the chain this admittedly incorrect value was added to the data. It would that some error-checking overhead somewhere else in the chain to really see what was going wrong.
Here's another issue where taxonomy processing was unable to deal with slightly unexpected datatypes being added to an input array

So ... Where I get the chance, I add at least a data type check to places where I expect that someone, somewhere may accidentally give the wrong type of input to an API call. Or if that fails, just do something that makes it no longer a bad error if they do.

in this case where this function is small, called from only one place, and no harm can come from casting it like this, I don't think that there is anything wrong with the first approach - it's probably more efficient than anything else.

But for a more normal fix, this attached path will also do what we want, in the caller, in a tidy, correct way.

--- taxonomy.module     17 Sep 2008 12:55:37 -0000      1.414.2.5
+++ taxonomy.module     3 Jun 2010 18:28:41 -0000
@@ -1177,7 +1177,9 @@ function taxonomy_nodeapi($node, $op, $a
       return taxonomy_rss_item($node);
 
     case 'update index':
-      return taxonomy_node_update_index($node);
+      if (!empty($node->taxonomy)) {
+        return taxonomy_node_update_index($node);
+      }
   }
 }

Personally I'd still favor making the function more robust and fault-tolerant, but this alternative way ensures that the fault doesn't happen in the first place. Also good.

bot-test it, I guess.

Meh. roll the patch from the right directory this time.

I haven't had my coffee yet. Sorry bot, THIS directory I mean.

Subscribe.

I was unable to replicate the error in d7.
The whole term tagging process has been turned inside out. This issue would have been unlikely to persist.

Adding a mandatory vocabulary to existing content does NOT cause this problem in d7.

Due to lack of priority, a fix like this is unlikely to be bothered with for d6 it seems

I don't know if it would help anyone, but I was getting the error on every cron run after updating to Pressflow 6.19.92. I tried the simple fix in #2, and it hasn't come back yet.

It is important that a fix for this bug be committed for Drupal 6. Re-opening.

This problem was affecting me as well; I used the solution in #2 and it solved it.

I would also like to see this simple bug with a simple fix get committed in a future D6 update. Among other things, it can generate a page full of scary looking errors every time cron is run, not something that end users will think is a minor bug.

Subscribing what rootwork says in #26

I also found the errors to go away when I used #2. I thought it would be good to find out which nodes were bad so I could fix them. I added code to dump the node if taxonomy was Null. Sure enough I got three dumps corresponding to my 3 previous errors. Surprising to me is that these are not really nodes, at least not yet. No nid or vid or even anything in the body?

They look like this:

object(stdClass)#26 (5) {
  ["build_mode"]=>
  int(2)
  ["body"]=>
  NULL
  ["readmore"]=>
  bool(false)
  ["content"]=>
  array(4) {
    ["body"]=>
    array(5) {
      ["#weight"]=>
      int(0)
      ["#value"]=>
      string(0) ""
      ["#title"]=>
      NULL
      ["#description"]=>
      NULL
      ["#printed"]=>
      bool(true)
    }
    ["#title"]=>
    NULL
    ["#description"]=>
    NULL
    ["#printed"]=>
    bool(true)
  }
  ["has_body"]=>
  NULL
}

The build modes may be coming from Display Suite which is running on this site.

There isn't a working 6.x patch in this issue, so setting NW.