Posted by bjaspan on October 25, 2007 at 3:12pm
3 followers
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | forum.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
If the variable forum_nav_vocabulary exists, forum_nodeapi() calls taxonomy_vocabulary_load(NULL) which triggers an error on E_ALL. This bug appears to have been introduced by #20295. Patch attached.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| forum-vocabulary-load.patch | 674 bytes | Ignored: Check issue status. | None | None |
Comments
#1
I can't help but think this fix is a little odd. I'll investigate more.
#2
I confess I did not fully grok forum_nodeapi(). I reasoned as follows:
1. If !in_array($node->type, $vocabulary->nodes), the function returns.
2. If the variable forum_nav_vocabulary is undefined, $vid will be empty, so $vocabulary will be empty, so $node->type will never be in $vocabulary->nodes. So we can just return if $vid is empty.
I now realize there is another problem: forum_nav_vocabulary may exist but refer to a deleted vocabulary, so $vocabulary will be empty. I'll make a new patch soon.
#3
In the original description I said, "If the variable forum_nav_vocabulary exists, ..." That should have been, "If the variable forum_nav_vocabulary does not exist, ...".
Anyway, new patch attached addressing #2 attached.
#4
My patch in #3 handles the cases in which forum_nav_vocabulary does not exist or refers to a non-existent vocabulary. However, what I just realized is that the reason the vocabulary and variable did not exist in my test environment is that it is created in forum_enable() in D6 but nothing creates it during a D5->D6 upgrade. So forums won't work after an upgrade.
To reproduce: Install D5. Enable Forum. Upgrade to D6. Do anything that invokes hook_nodeapi.
I think the attached patch fixes the problem but I have (still) not looked closely at the details of forum.module so I could be completely mistaken. That' what reviews are for. :-)
#5
The forum vocabulary is created in _forum_get_vid() in Drupal 5, when this function is first called and there is no forum vocabulary set up yet. So in fact, it might not exist for an extremely small number of existing Drupal 5 sites. On those sites, where this function was never called, this vocabulary will indeed not exist.
drupal-53 gabor$ grep -r "forum_get_vid" *modules/forum/forum.module: if ($op == 'delete' && $term['vid'] == _forum_get_vid()) {
modules/forum/forum.module: if ($form['vid']['#value'] == _forum_get_vid()) {
modules/forum/forum.module: $tree = taxonomy_get_tree(_forum_get_vid());
modules/forum/forum.module: $forum_terms = taxonomy_node_get_terms_by_vocabulary(_forum_get_vid(), $node->nid);
modules/forum/forum.module: $node->taxonomy[arg(3)]->vid = _forum_get_vid();
modules/forum/forum.module: '#value' => _forum_get_vid());
modules/forum/forum.module: $form['vid'] = array('#type' => 'hidden', '#value' => _forum_get_vid());
modules/forum/forum.module: $tree = taxonomy_get_tree(_forum_get_vid());
modules/forum/forum.module: $children = taxonomy_get_tree(_forum_get_vid(), $tid);
modules/forum/forum.module: $tree = taxonomy_get_tree(_forum_get_vid());
modules/forum/forum.module: if ($term->vid == _forum_get_vid()) {
modules/forum/forum.module:function _forum_get_vid() {
Basically if any forum page was ever visited after the module was enabled, the vocabulary is there. This is unfortunately not 100% certain, and Drupal 6 has this vocabulary creation moved to the enable hook, so we might as well make sure it exists.
Looking at the patch with this in mind:
- "Create the forum vocabulary which did not exist prior to D6." is not true.
- Why would we need to check for it anymore in forum_nodeapi(), as with the update function, it is ensured that with an update the vocabulary is there if it was not there before, and new installs get the vocabulary with enabling the module? The only problem I can think of here is that it might have been removed independently on the taxonomy admin interface, but only the second check would be required in this case too.
#6
Your analysis makes it sound like this is not really critical though it should still be fixed.
I fixed the comment in forum.install and removed the test to verify if $vid is set.
#7
After deciding this was not critical on 12/12, I'm thinking now that it is. Sure, we think only a small number of sites will encounter this problem: only those that enabled the forum module but never used any forum pages. However, for those sites, the current code leaves them *no way* to recover without hacking. Even if they manually create the forum vocabulary, the variable identifying its vid will not exist.
My patch in #6 still applies and solves the problem: on upgrade, the vocabulary is created and the variable is set if it is not already. Also, forum_nodeapi() will no longer report errors if the vocabulary is manually deleted.
#8
Agreed, committed, thanks.
#9
Automatically closed -- issue fixed for two weeks with no activity.