Forum vocabulary is mandatory for node types (and prints notices)

I tried to do this with a 'form_alter'...

Make the vocabulary not required by default and 'form_alter' it to be required when node-type is 'forum'.

But form_alter will first visit forum.module and then taxonomy.module. So the forum has nothing to alter yet.... Any ideas?

Took the approach as written above. Attached a patch that use form_alter to set the vocabulaire required when node-type is 'forum' and by default optional for other node-types.

But, code needs work. For this to work, forum module must be called after taxonomy module so I set the forum.module weight manual to 1 in the system-table. I have no clue how to do this in forum.install yet, same for upgrading.

In my opinion, taxonomy is such a 'low-level' module that it should have a weight of -1, so all 'normal' modules will come after it. Don't know if that is possible.

Fixes the notices by setting default values instead of unsetting.

A new patch that includes the necessary updates:

- Set weight to 1 in the system-table.
- Set 'required' for the forum-vocabulaire to 0

I see two other options:

- Set the weight of the taxonomy module negative as suggested in #2. I'm not the first one having problems with altering taxonomy and it would be silly to require contrib modules to set a weight or name it after the 'T' in the alphabet.

- Alter the taxonomy module not to overwrite existing values if set earlier by other modules via form_alter. But this seems ugly to me....

Setting the weight of the forum.module to 1 breaks it when doing a menu_rebuild(). I'm afraid it will for every module that provides a node-type and has a name after the 'T'.

The form.module did not function after a rebuild when the weight is set to 1 because it defines it's own 'node/add/forum'; e.g. overwriting the one created by node.module.

A new patch with the menu entry deleted from forum.module. It's not necessary and poll.module doesn't have it either for example.

Mistake from me.... menu entry is necessary for acces check of 'create forum topics'. Code needs work....

Pffff, I should stop playing with it and leave it up to the experts to review :-)

Access-check is done by node_access who's calling forum_access; so it's not necessary to define the menu entry in forum.module and my patch is working and ready for review again.

Attached an updated patch that deals with forum_nodeapi to save and update the {forum} table properly with nodes than CAN be in the form but NO forum is selected.

Reroll because the patch did not apply anymore.

There are a few feature requests around asking not to have the forum vocabulary restricted to one term by default - so that announcements could be placed in multiple forums for example. This is something I'd support and would simplify this bug as well I think.

I don't see that feature request related to this bug; but it's very easy to do. Just set 'multiple' => 1 instead ' multiple' => 0,' and you have more choices. Or don't hard-code it and make the choice available in the admin screen. You still have to deal with the 'required' forum-vocabulary for non-forum-nodes.

Anyway, the biggest problem with this bug (and my approach) is that I have to set the weight of the forum.module higher than the weight of the taxonomy.module so forum can form_alter the taxonomy forms.... This is nasty and it would be very nice if this could be done different but I have no clue.

But patch does work fine :-)

http://drupal.org/node/171634 was duplicate (actually this is, but leaving it active since there's more to this than just the notices).

Updated the patch, now alters taxonomy.module's 'form_alter' to respect 'old' values set earlier by the forum.module. So not necessary to set a weight; thanks for the suggestion Chx!

Still an update necessary in forum.install to set the requirement of the vocabulary to 0 (included in this patch).

Moved an inval around 'variable_get from the forum.module to forum.install

O o, not good.... the first option in the Forum-vocabulary is still .... even when required....

CNW, update will follow...

They wil be required for the Forum, off course.

It did before in my previous patches but now I moved something around and that function was lost. On my testbed it's back already but I just have to deal with some unpleasant notices before I can submit a new patch.

Attached the patch that will set the Forum vocabulary for Forum nodes as required and removes the first 'none' selection as well (that was what the last patch forgot to do).

Pfff, typo in some php-doc.... This is it for today :-)

I want to add that although setting weight seems to be very unpopular I would personaly like it much more if such a 'low-level' module as Taxonomy is simply below other 'normal' moduls; e.g. has a weight of -1.

The solution I presented now works but to me it feels like a dirty hack to Taxonomy just to get the forum-issue done. Bleh! :-(

I gladly redo the patch to set Taxonomy to 'weight -1'; but first need some others' opinion about it.

you can slap a #process on $form['taxonomy'] in forum now that you honor $form['taxonomy'] in taxonomy.

Gábor, Chx

#process is the way to go. This is all new to me but I got it already working on my local website. Please leave it up to me to provide a nice patch before the 2nd of november 24:00 :-)

Yeh, I assign this issue to myself, it's mine, my precious :-D.

The final patch (???), way before the 2nd of November:-)

general
- added a lot of php-comments

taxonomy.module
- alters taxonomy.module to respect previous values.

forum.module
- form_alter to set it's vocabulary to be required.
- use #process to remove 'none' from the selection.

forum.install
- set it's vocabulary not to be required by default.

Doesn't look 'hackish' to me anymore.

After a good talk with Chx on IRC I set it back to CNW. I'm getting there, but not yet :-)

Updated patch as by Chx' review on IRC yesterday (thanks again Chx!).

- corrected a '+=' mis-usage.
- corrected respect of 'weight' of a vocabulary.
- removed 2 useless 'intval's in forum.install
- '#required' => 0 deleted; was unnecessary (and should have been FALSE otherwise).
- _forum_require_vocabulair corrected to _forum_require_vocabulary.

- moved update to system.module
- code-style corrections
- extended php-comments a bit
- added #vaulue ='' to avoid any possible tampering

I can't find any more flaws :)

- "We have already an array" and "an fatal error" do not seem to be valid English to me.

"We have already an array" seems correct to me. "an fatal error" should be "a fatal error". Please note that my native language is not English so if this is still incorrect I would appreciate a suggestion from somebody with better language-skills than me :-)

- How is it possible to remove the taxonomy form element weight?

The weight is loaded from the database and is set in function 'taxonomy_form_alter'. Hardcoding it to '15' makes the weight-field completely useless as all the fields will be set to 15. This is actually unrelated to the forum-issue but a fault in taxonomy.module in general.

- How is it possible to remove the link to the forum node creation form from forum_menu()?

It's not necessary. Node.module takes care of that. It defined 'node/add' and takes care of the 3th argument. Please note dat blog and book module don't declare it either for example.

- PHPDoc on _forum_require_vocabulary() is one empty line too far from the function itself.

Oops....

Attached a new patch that corrected the English and removed one empty line.

I would say "We already have an array" or possibly "We have an array already".

"We already have an array" sounds wonderful to me. Thanks John!

Try this one.

Thank again John, but 'CNW'... found a notice when viewing a node that CAN be in the forum but is NOT.

Update follows....

Fixed notice by looking for vocabulary instead of node-type. It was not just a notice; the breadcrumb was also wrong.

New patch attached.

Still tons of notices from Taxonomy.module when doing a preview, but not from Forum.module anymore when viewing, creating or editing a node connected to the forum.. See http://drupal.org/node/163728

Patch applies ok here, but I'm in a big fight with CVS; hope it's ok now.

After a fresh checkout I saw that system_update_6035 is already in use.... Will see if I found more new issues and post a patch later.

Well I reroll meanwhile. I do not like CNW criticals :)

A 'break;' was missing in forum_nodeapi('update') with as result an duplicate entry into the {forum} table after editing a node.

Updated the patch again and I will update http://drupal.org/node/163728 now to get rid of the last notice when previewing a forum-related-node.

I only glanced quickly at the most recent patch for this issue but it looks like it is related to http://drupal.org/node/186546: "If the variable forum_nav_vocabulary does not exist, forum_nodeapi() calls taxonomy_vocabulary_load(NULL) which triggers an error on E_ALL. This bug appears to have been introduced by #20295."

If that issue is made irrelevant by the patch on this one, feel free to mark that one duplicate.

This issue does not deal with the forum_nav_vocabulary existence so http://drupal.org/node/186546 is not a duplicate. This issue only deals with the forum-vocabulary handling for non-forum nodes that can be added to a forum.

Patch from #43 applies with offsets to system.module and taxonomy.module

Issues:
* Required forum vocab, is not in fact required. It is possible to submit a forum topic without a forum chosen. The Asterik shows up correctly on forum node types, and it doesn't show up on non-forum node types that are assigned to the forum vocab, but it is not required and the none selected option is available, and works.
* The edit vocabulary form for the forum vocab has no options for multiple select. I just scanned the follow ups here, so this may have been outside the scope of this patch.

Setting back to code needs work

Thanks for the review mikey_p

The commit from http://drupal.org/node/180109 for taxonomy.module broke this patch regarding the taxonomy selection :-( Will update the patch later today.

'multiple select' is not part of this patch.

A new patch:

- deals with commit http://drupal.org/node/180109: function '_forum_require_vocabulary' is changed now to reflect that.
- function 'forum_form_alter' is simplified and working correctly now for 'real forum nodes'.

Thanks for chx and eaton for your help on IRC; I really appreciate it; it's learning and very motivating!

Patch #21 works as advertised. I got an offset of one line in system.install (no fuzz), but this seems to be a relatively trivial problem?

However, it definitely needs to be tested by someone else, because for some reason my test site "swallowed" the system update 6036. I was not alerted to a new database schema, and when visiting update.php I had to manually select 6036 from the list of available updates. I'm pretty sure that the offset has nothing to do with this, and that it's in fact a problem that occurred only on my site, but it would be essential to verify this in another test.

To do that, the tester needs to have an unpatched site installed, patch it, and visit /admin as well as /admin/logs/status and make sure that they are alerted to the changed DB schema.

Just to agree with leaving shadows only for forum topics, this could be more fine-grained in D7 maybe but would require some thought.

Firstly, the search patch has usurped system_update_6036, which means a) we have to go up to 6037, b) all the test sites that have applied this patch have to be re-installed clean to work without problems (as a matter of fact, I suspect that a similar issue caused the trouble in my last post). I'll take care of a) in this patch.

Secondly, I think I can fix the forum_nodeapi problem; but I'd rather not touch the shadow problem.

forum_nodeapi/update simply assumes that if tid is set, it is already in the database. Fortunately, it already falls through to insert when a new revision is saved, so this condition just needs to be expanded to also fall through when there is no term yet in the database.

I'm not sure if db_result(db_query(SELECT COUNT...)) is the best thing here, nor whether putting it directly into the if condition is code-style kosher. I haven't seen any obvious rules against it though.

Steps:

- Installed a new site with patch
- Created forum terms
- Attached forum vocabulary to Page type
- Created page node without term
- Updated page node and entered term
- Checked {forum} db table as well as forum page to see that page node has now been added to the forum.

Patch is here.

(I'm leaving this as CNW due to the Shadow problem.)

Oh wait, I seem to have misunderstood how involved the Shadow issue would be. If it only involves clearing a notice-level message on a value that is never set for non-forum nodes, nothing simpler than wrapping the condition in empty(). At least I /think/ that's the right method.

Here's another patch that should address both of Gábor's issues.

Updated the patch to deal with all issues from #55

- check for $node->tid now (an 'isset')
- corrected intention and some more small code-style issues
- made the 'if-if' an 'if-&&'

Didn't saw that I could combine another 'if-if' in forum_nodeapi case 'presave' as well... Was a bit too much in a hurry with my path I guess...

New patch attached.

Found another one; but please note that these "if-if"'s are not created in this patch but do already exist.

Off course it's good to improve code while solving a issue; but holding-up a patch because of current code is not so nice...

One more 'if-if'....

Those were all the remaining issues, then... RTBC?

Gábor, thanks for the update. I re-rolled the patch because it had a huge offset after the taxonomy-preview-commit.

Could the forum module alter the form array differently to ensure it overwrites only the values it wants to change?

Is this "function realizing it has run already" a design pattern that can be perpetuated elsewhere? I think this would be a concept whose "hackishness" depends on how widely it's used - if it ends up being a special case, we should consider it a bit more critically.

Gave it some time to think about the available three options:

1 - Set the weight of the taxonomy lower.

I like it, as I wrote already in #2 and #6. The cleanest option for me. It's not only 'forum' who has problems with taxonomy. A search on Drupal.org revealed more. Taxonomy.module is a 'low-level' module; same like node.module for example.

2 - invoke taxonomy_form_alter() from forum_form_alter()

Smells like an awful hack to me. Modules are not ment to call each others hook function directly like this and it doesn't solve the 'general' problem as wel, it solves just for the forum. And also (although I did not check) I'm shore taxonomy.module will be called -again- by his -natural-hook-function- so it must be aware of it's own previous existence... oef.....

3 - As Arancaytar suggest in #65, make the forum also 'value-aware' just like Taxonomy is in my last patch. Then it should work in random order. Still smells not too nice for me.

I like nr. 1 by far the most with nr. 3 as an acceptable 2nd choice.

So the array to change might or might not be in the form already.

As an example (I'm not sure about the exact form changes in this case):

If we want to change $form['taxonomy']['#required'] to true, then if $form['taxonomy'] exists already, this code would overwrite it:

$form['taxonomy'] = array('#required' => TRUE);

This should have the intended effect:

$form['taxonomy']['#required'] = TRUE;

(It might not even cause a notice - if it does, we'd have to use isset/empty checks)

Is there something deeper about the problem that would prevent this approach?

So which is it to be: Form value preservation or weight change?

And if a weight change, should we make forum.module heavier or taxonomy.module lighter?

This is really a policy issue; once it's decided the patch will be trivial to fix.

I did some thinking about weights (only theoretical) but the best way is to set the forum.module heavier. You can't lower the taxonomy without lowering node.module too. Taxonomy.module alters a form created by node.module and there might be even more that could break if taxonomy is lowered.

I think any other option than weights will be a lot of work. My last patch was already complicated and a bit dirty and it would be even worse if we have to make it 'order-in-depended'. It looks that the 'form_alter' construction is not ment to alter an altered form by design and fixing that is out of this league in my opinion. As to quote Gábor in #66:

This is not a new problem, chx done some proof of concept on per-hook call ordering: http://cvs.drupal.org/viewvc.py/drupal/contributions/sandbox/chx/weights... But this is not for Drupal 6, so we need to find a solution, which fits this problem at hand for now.

I have some time next Sunday to work on this issue again if we can decide in what direction we want to go.

Gábor, weights or not; the patch does not apply anymore. Last commits are breaking the patch.

But, for now, setting the weight is the best we can do. An 'order independent solution' is much better but out of scope now. I think it's no good to 'invent' it for forum.module only.

Working on the patch right now. As you wrote, this is critical and I do my best to solve it.

Attached a new patch. Basically the same as in #63, except this time the weight of the forum.module is set to 1 and that makes forum_form_alter() a bit simpler.

Setting the weight is done in function forum_install() and an update is provided as well.

Proper status.

lovelycyprus: it doesn't matter whether you're a developer or not, but please don't hijack this issue with your support request. Drupal 6 is still in Beta, and unsupported until there's a full release.

Sorry, missed the needs work in tidy up, setting back, again.

how to get (none) option under (forum) box on the 'creating content' page. there is compulsory sign against (Forums: *)
i am not a developer so pls advise accordingly
i activated forum module and now any content i create goes to a forum category also as there is a compulsory Astrix in forum box? how to make it optional?
tnx

lovelycyprus, you are in the wrong place, please go to http://drupal.org/node/add/project_issue/drupal/support

Sorry for lack of updating the patch; I'm truly occupied with my job lately. Attached a new patch according the review of Gábor in #78.

- why not set the forum weight to taxonomy weight + 1 to be a bit more bullet proof?

Agree it is a bit safer for upgrading but to be honest, I don't see any reason someone would lift the weight of the taxonomy.module as it will bring a lot of problems anyway. I did not implement this; also because lack of time.

- to me, the node/add/forum path seems to be there to properly check the permissions... I checked how things go after this patch, and it seems access args are array('create', 'forum') with this patch, which means forum_access() is consulted, so the proper behavior is achieved

So removing it from forum.module was alright.

- why have an if ($tree) inside an if(), which seemed to be already checked the tree.

My mistake; a leftover from if-if cleanup.

- was hook_nodeapi(load) modified somewhere to return an object? http://api.drupal.org/api/function/hook_nodeapi/6 still says we should return an array

Interesting :-) I looked at the docs and 'view' and 'prepare' are not suppose to return anything! It was in the original forum.mudule for some reason but I removed it in this patch.

- in "forum.module to 1 so its loaded after" "its" should be "it is"

Corrected.

I'm sorry to say, but tomorrow I leave the country for about 3 weeks spending some time on a vessel without internet so I'm unable to work on this issue for a while. Hope somebody can take over or else it would be too late to fix this for D6 and that would be very sad....

- Oops, commit #194075 changed that and I had old code. Corrected.
- Ok, update does forum = taxonomy + 1 now. Tested with different weights.

Tested this on a fresh install, all works cleanly as far as I can see.

Will there be a patch for 5x?

gracearoha:

This patch deals with an issue introduced by being able to use multiple node types with the forum module - which hasn't been backported to 5.x