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).
Comment | File | Size | Author |
---|---|---|---|
#11 | 676134_node_taxonomy.patch | 2.93 KB | mcarbone |
#10 | 676134_node_taxonomy.patch | 2.93 KB | mcarbone |
#5 | 676134_node_taxonomy.patch | 2.82 KB | mcarbone |
#2 | 676134_node_taxonomy.patch | 1.46 KB | mcarbone |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #2
mcarbone CreditAttribution: mcarbone commentedI 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.
Comment #3
jhodgdonAgreed, 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.
Comment #4
mcarbone CreditAttribution: mcarbone commentedOK, 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.
Comment #5
mcarbone CreditAttribution: mcarbone commentedIt would help if I attached the latest patch.
Comment #6
jhodgdonSetting 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!
Comment #7
agentrickardNot 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.
Comment #8
jhodgdonSounds 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
Comment #9
agentrickardThe 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.
Comment #10
mcarbone CreditAttribution: mcarbone commentedOK, 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.
Comment #11
mcarbone CreditAttribution: mcarbone commentedMinor language change.
Comment #12
jhodgdonI'm happy with the patch in #11, but will let agentrickard set it to RTBC.
Comment #13
agentrickardI 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.
Comment #14
webchickWow. Nice clean-up, folks.
Committed to HEAD.