Node taxonomy array key change on node_save

Having severe issues after updating to 6.24 as well...

An of the issues; using computed field I was setting a value based on taxonomy values by checking $node->taxonomy['tags'] where tags was the name of the vocabulary.... this isn't possible anymore. But that seems the least of my problems, noticing multiple taxonomy issues... changing to critical as I expect this not to be intentional and it breaks lives sites when updating to 6.24....

Cheers

It's worth bearing in mind that the $node->taxonomy array has always had different formats at different points in a node's lifecycle.

IIRC node_load, node_save, and dealing with a save operation in hook_nodeapi() all present you with different arrays of data.

However, changing when you get which one is definitely a bug.

The taxonomy array for a node prior to 6.24 should return TID keys with objects, not VIDs with TID integers, so some other module may be setting that incorrectly.

The change in 6.24 is to reload these values correctly into the $node object during an update operation (http://drupal.org/node/767104), hence you now seeing the correct objects.

Not real happy with this being closed. Like joachim writes, the way how the $node->taxonomy array is presented should not be changed just like that and this is probably a bug!

Okay, perhaps I wasn't reproducing the problem correctly. I only looked at the output of devel from an individual node after saving, which does return a Term object prior to 6.24.

The functionality can't be reverted, as it removes a bug. The VID->TID result in 6.23 may well be stale, so even though the 6.24 data format is different, it is at least correct. The fix is probably to get it back into the original format without reverting.

So it removes one bug and creates new bugs on dozens of contrib modules? Great fix! :)

I've the same problem!!! Some fix?

@stimart; for now I downgraded to 6.x-2.3 again... don't know of any other quick fixes or patches.
Remarkable that a critical core bug like this doesn't get more attention!

@KeyboardCowboy: thanks for doing that investigation in #5 above. That's very useful information to have clearly laid out.

Though there's even more complexity than that: on Drupal 6.22, the contents of $node->taxonomy for the 'insert' and 'update' hook_nodeapi operations differ depending on whether you're editing a node in the UI, or saving a node programatically with node_save().

For a node save form, as KeyboardCowboy reported, you get an array of the numerical values. But it's somewhat more complicated even then, as vocabularies that are single-valued don't get a nested array:

array(
  // multi-valued vocab:
  vid1 => array(
    tid1 => tid1,
    tid2 => tid2,
  )
  // single-valued vocab:
  vid2 => tid3
)

For a programmatic node operation though, you get one big array of term objects from all vocabs, indexed by tid.

To illustrate this here's a snip of code from a custom module I run on one of my sites. This is in an implementation of hook_token_values() which looks at a node's taxonomy terms to provide tokens to node paths, hence gets called with a $node object for hook_nodeapi insert and update:

    // Detect whether we are coming here straight from a node form or a round
    // trip of node_load() and node_save().
    // The contents of $node->taxonomy are completely different depending on
    // this, hence we need to take this into account so we handle both saving
    // a node and updating a node in content admin.
    $first_term = reset($issue_node->taxonomy);
    if (is_object($first_term)) {
      // Node_load round trip.
      <snip>
    }
    else {
      // Node save form.
      // [You then still need to figure out whether it's a single or multi-valued vocabulary before you can do anything useful.
      <snip>
    }

Hence in 6.22 and earlier, there was an existing WTF because $node->taxonomy took two different forms for the same operations, depending on how those operations were being carried out.

Arguably, the situation in 6.24 fixes this bug.

Furthermore, any module which only deals with the array of tids format is buggy for doing so. Saving a node programmatically may seem like a non-standard thing to do, but any operation you do from the Content Admin page such as publishing / unpublishing is doing just that.

Therefore, any non-buggy module should already be handling *both cases* (as my code sadly had to), otherwise it's had a bug all along. Therefore, a module that doesn't have this bug will continue to work as normal, but the 'Node save form' part of the code is now redundant and will never be executed.

Hence, to come to a conclusion:

- modules that are affected by this change were buggy in the first place
- I'm inclined to call this 'works as designed'

A change notice might be a good idea though, if we're doing them for D6? Contrib module maintainers should certainly be made aware of this, though like I say, they should be aware of it already, in the form of either a bug report or crappy code that caters for multiple data formats :/

Hi joachim,

Thanx for your clear answer and yes, now that you put it this way I'm also inclined to change to a 'works as designed' yet a very clear and strong notification of change on http://drupal.org/node/1425094 pointing to this issue would be nice!

Cheers

Might be an idea to talk to people about this during Core Office Hours on IRC; it's likely the people who can edit those release notes will be around then.

@KeyboardCowboy; we share the same worries. Of course joachim is right calling those contrib modules 'buggy' yet they have been operating on something that was available, due to a core bug the contrib developer probably never knew even was a core bug but thought was a core feature. I'm also worried about all those less technically inclined that simply don't know why their site isn't operating as expected anymore since an update and don't find this issue to even report to the contrib maintainer what is going on.

The best solution IMHO is to somehow fix this core bug but still allow for backwards compatibility but am not sure if this is possible at all.

I filed a request to have the 6.24 release notes updated with a mention of this: #1450680: update core 6.24 release notes to mention API change.

> but I have found at least one instance that broke my site in the SimpleNews module.

Has this been filed on Simplenews?

yet they have been operating on something that was available, due to a core bug the contrib developer probably never knew even was a core bug but thought was a core feature

That doesn't make it NOT a bug. As @joachim said, any module which didnt handle both cases already had a bug, just not one that you might have encountered.

As @joachim has filed the change request in #1450680: update core 6.24 release notes to mention API change, I agree that this is 'works as designed', as there is no extant bug in core.