forum.module causes errors after upgrade [#186546]

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.

Comment	File	Size	Author
#6	forum-vocabulary-load-186546-6.patch	2.33 KB	bjaspan
#6
#4	forum-vocabulary-load-186546-4.patch	2.38 KB	bjaspan
#4
#3	forum-vocabulary-load-186546-3.patch	1.07 KB	bjaspan
#3
	forum-vocabulary-load.patch	674 bytes	bjaspan

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Dries CreditAttribution: Dries commented 25 October 2007 at 17:50

I can't help but think this fix is a little odd. I'll investigate more.

Comment #2

bjaspan CreditAttribution: bjaspan commented 25 October 2007 at 18:51

Status:

Needs review

» Needs work

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.

Comment #3

bjaspan CreditAttribution: bjaspan commented 1 November 2007 at 22:52

Status:

Needs work

» Needs review

File	Size
forum-vocabulary-load-186546-3.patch	1.07 KB

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.

Comment #4

bjaspan CreditAttribution: bjaspan commented 12 December 2007 at 14:22

Title:	forum_nodeapi assumes that forum_nav_vocabulary exists	» forum.module causes errors after upgrade
Priority:	Normal	» Critical

File	Size
forum-vocabulary-load-186546-4.patch	2.38 KB

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. :-)

Comment #5

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 12 December 2007 at 14:46

Status:

Needs review

» Needs work

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.

Comment #6

bjaspan CreditAttribution: bjaspan commented 12 December 2007 at 17:00

Priority:	Critical	» Normal
Status:	Needs work	» Needs review

File	Size
forum-vocabulary-load-186546-6.patch	2.33 KB

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.

Comment #7

bjaspan CreditAttribution: bjaspan commented 20 December 2007 at 15:51

Priority:	Normal	» Critical
Status:	Needs review	» Reviewed & tested by the community

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.