I noticed this while writing a test that calls the drupalCreateNode function which can be found in modules/simpletest/drupal_web_test_case.php. That function creates a $node object, allowing $node->taxonomy to be filled with a value (and using NULL as default), and then calls node_save on it -- and it's not working properly. In earlier versions of Drupal, this filled with an array of tids would create the node with the correct terms saved. Now with taxonomy in fields, $node->taxonomy just seems to be ignored.

This seems like an undesirable change, as there's likely a lot of contrib modules and import code that uses $node->taxonomy. So either this needs to be fixed so $node->taxonomy worked as before, or at the very least, the change should probably be documented somewhere (and drupalCreateNode should be modified so that it doesn't imply that a taxonomy value is allowed).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

This is by design. $node->taxonomy was a simple array of term IDs, and with Taxonomy term reference fields, it's possible for the same term to appear the node in different fields multiple times.

I'll look at the test to see what needs to be fixed.

The change is documented in the change log, though admittedly it might not be clear that bringing taxonomy terms to nodes through Field API necessarily means the old way of the taxonomy property on a node is gone.

mcarbone’s picture

Title: $node->taxonomy is no longer node_save'd properly » Update documentation to account for taxonomy changes (no more $node->taxonomy)
Component: taxonomy.module » documentation
Category: bug » task
Status: Active » Needs review
FileSize
1.46 KB

I think these two simple changes would clear up some confusion. Although we may also want to update the module conversion or some other d.o docs.

jhodgdon’s picture

Status: Needs review » Needs work

Agreed, there should be an entry in the module update page, if there isn't already one. Did someone check?

Regarding this patch, I don't think the example for hook_node_access_records_alter should be using ->taxonomy_example -- that is too confusing. How about just choosing some other custom field name?

The other part of the patch, removing ->taxonomy from simpletest, makes perfect sense to me.

mcarbone’s picture

OK, I simplified that example a bit, removing the taxonomy component. I think its instructive value is still there.

Before I posted this issue, I looked around for any documentation on this and couldn't find any, but I'm not necessarily the most reliable documentation finder.

mcarbone’s picture

FileSize
2.82 KB

It would help if I attached the latest patch.

jhodgdon’s picture

Status: Needs work » Needs review

Setting to Needs Review for the test bot.

Assuming the test bot has no trouble (which I would expect), it's RTBC. I like the new example a lot -- it's clear and illuminates what is going on. Nice work!

agentrickard’s picture

Not crazy about the new node access example. What does 'control grants' mean in a real use-case? It's example code that has no real-world equivalent.

Can we instead change the old $node->taxonomy check to $node->is_preview, and use the old example use-case?

It might be better to include multiple examples for hook_node_access_records_alter(). But that would be a separate issue from this.

jhodgdon’s picture

Sounds like maybe a good idea, but where is this old $node->is_preview example?

If you want to do something with multiple examples, the best venue is the Examples for Developers project: http://drupal.org/project/examples

agentrickard’s picture

The is_preview example is what the old taxonomy code was doing, except we were using taxonomy tags instead of a 'this is a preview' checkbox.

mcarbone’s picture

FileSize
2.93 KB

OK, I went ahead and tweaked it to use "is_preview". The only change between this patch and the previous version is that the taxonomy tag language has been modified to a simple field. No other language or descriptions were lost. I don't think this is the clearest example, but node access is fairly difficult to understand in the first place. I think it's more important to get the obsolete taxonomy stuff out of here, and if necessary, a new thread can be started to improve node access documentation.

mcarbone’s picture

FileSize
2.93 KB

Minor language change.

jhodgdon’s picture

I'm happy with the patch in #11, but will let agentrickard set it to RTBC.

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

I like this. It was very hard to try to write an example that made sense. And if you understand node access, this one actually makes some sense. It says, in effect, "Hey! This node is in preview-only mode, so don't let any other module assert permissions to it."

The use of taxonomy in the original was so that we could avoid having to explain the whole "Node access modules must be able to rebuild the node access table by storing their data" issue.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow. Nice clean-up, folks.

Committed to HEAD.

Status: Fixed » Closed (fixed)

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