Forum vocabulary does not handle standard vocabulary features correctly

It seems like there is still an issue on that. For some people the 'tid' field
of table 'forum' is still set to 0 for every forum item.

Look at http://drupal.org/node/54493 for a starting discussion.

Since forums are special kind of nodes and they use taxonomy system, it
is possible to configure them with multi- or even freetagging option. But doing
so breaks the node-term-relation and therefore the node/topic is not listed
in forums.

The reason for that lies in function 'forum_submit' in forum.module as
Jonas Kvarnstrom pointed out. $term is an array is this case and
$node->tid = $term; doesnt work. A very basic workaround is to
simply use the first $term and have the new node assigned to it this way.

foreach ($node->taxonomy as $term) {
 if (is_array($term)) {
  $term = current($term);
 }
 if (db_result(db_query('SELECT COUNT(*) FROM {term_data} WHERE tid = %d AND vid = %d', $term, $vocabulary))) {
  $node->tid = $term;
 }
}

For a better solution its necessary to add 'nid, vid, tid' to table 'forum' foreach
$term->tid. Its easy to do so, but also the update and delete function must be
adapted accordingly.

I think it is implied that the forum taxonomy is non-tagging and is a simple hierarchy, see _forum_get_vid(). Can we make sure that somebody doesn't mess this up?

The most simple solution was to hide hierarchy/relation/... options
whenever the node type 'forum' is selected. And set options back
to 'simple' hierarchy, no relation, no free-tagging, no multi-select
and required afterwards. (Patch for taxonomy.module is attached)

Do you think it is sufficient to do so? Maybe it would be better to
extend forum.module to support multi-select&Co!? Hide&Override
seems somehow inconsistent with taxonomy in general!

I think the proper solution would be to use hook_form_alter inside forum.module to remove these settings.

Non-critical. Downgrading.

-K

Non-critical? It breaks the forum!
However you can use hook_form_alter to remove the settings, whenever node type
'forum' is selected, but doing so does not set the options back to 'simple' hierarchy.
I tried with hidden form fields. But since the hook happens when the page is viewed,
it works only when the user visits the page for a second time.
If a user selects forum node type and presses 'submit' all the options incl. hierarchy
are stored. If the user visits the settings again hook_form_alter hides the options and
carries the hidden data fields instead. If the user presses 'submit' this time correct
values are saved.
As far as I know you can not alter a submit function of a different module, right?
But I think you must patch taxonomy_form_vocabulary_submit (in this case)
as well.

"Non-critical? It breaks the forum!"

So does deleting forum terms from the category menu. This is an extreme case where a user is doing stuff that he shouldn't be (at this point in time). The issue relates to users either creating the Forum category themselves via the categories admin menu, or editing the forum vocabulary directly and enabling as_of_yet unsupported features. Both of these are something of a no-no.

I think a simpler fix might be to just remove "forum" from the categories pages. I think the only ramifications of this will be that you can't customise the title or the weight of the forum vocabulary. IIRC this has been discussed a few times, but nothing has come of it. If you can, please submit a patch for this as well.

Cheers,
-K

Thanks. That makes things clearer. The point is: even if the user shouldnt edit forum vocab directly, he CAN do it with ease. I agree that it is a special case and should be avoided for unexperienced users. In my case I didnt play with forum vocab myself, but one of the users using taxonomy_theme did and reported that my module was not working. After hours on the code and the forums I (we) figured out, that it is not my code, but one of the core modules working not correctly ... this is really annoying, especially if this issue is already known ...

I think a simpler fix might be to just remove "forum" from the categories pages. I think the only ramifications of this will be that you can't customise the title or the weight of the forum vocabulary.

Having read this all. I think at the moment it might be better to use code like the one in comment #3. Doing so doesnt touch the vocab/terms directly and users that are using multi-select on forums for example, may continue to do so as long as there is no improved forum.module.
Read this from another post:
If I post a topic to 2 different terms in vocabulary forums, for example ForumA and News, the topic is actually added only two one of them and the other shows a 'This topic has been moved' message that links to the first one. (with the patch from #3).

If you can, please submit a patch for this as well.

What is 'THIS'? You mean remove 'forum' from categories?
I will try to code whatever patch you want to fix this quickly. So please tell me what you prefer. (I dont read IRC discussions at the moment. ;) Only developer mailinglist.)

Regards, Thilo

In my case, I'm new to Drupal and tried to get the hang of taxonomy and how to use vocabularies. I originally defined a number of terms to be used for categorizing news items, then activated the event module and also used those terms for event categories. Finally I activated the forum module and noticed that forums were also represented as terms in a vocabulary.

The idea that occurred to me at that point, which seemed quite natural at the time, was to try to use the same vocabulary for forums as for news items and so on -- the forums we use are quite coarse-grained and would fit well with the categories used for news and events. Then I wouldn't have to have basically two parallel vocabulary structures with very similar-sounding categories, which would be especially important because you may want to promote some forum posts to the front page so that terms from the two parallel vocabularies would appear in the same context (at least that's what I think, though as I said I'm a beginner when it comes to Drupal and maybe I've missed something). So I set the forum vocabulary to be applicable to news posts and so on, not just to forum posts.

Then, a couple of days later when I continued setting up my site, I thought I might like to post a news item with multiple categories, so I changed the vocabulary to be able to mark multiple terms.

And finally, another couple of days later, I went back to testing the forum -- and suddenly none of my posts would show up anymore.

This isn't really an argument for one solution or the other, and I'm not claiming that this bug is critical; I'm sure not many users would go through this sequence of events to destroy their own forum structures. I just thought I should explain how I got to this point in a way where each step seemed quite logical to me.

@profix: Yes please - a patch that hides the forum "taxonomy" from the category page will be excellent. IMO it's the simplest fix to this issue with the RC coming up. All the other patches in this thread promote these unsupported "features" and therefore are probably not the best course of action. Multi-select should only be an option for "sticky" threads.. all best left until 4.8 :)

@Jonas: Yes I agree. This is all not very intuitive atm.

Cheers
-K

I made a small patch for taxonomy.module to
- remove node type 'forum' from 'edit vocabulary' page
- hide forum vocabulary in categories

It seems to work correctly for me. Tested with existing site and on clean install.

Maybe I should set this to 'patch (code needs review)' !?
We can set it back to active, if further discussion on the unsupported "features" is needed for 4.8 ;)

I haven't tested this, but looking through it:

• remove node type 'forum' from 'edit vocabulary' page This should remain IMO. Adding other vocabularies to the forum forms works fine AFAIK and is something of a required feature.
• Functions that begin with a _ are generally considered private methods. It is not *proper* to call them from external modules (with a few notable exceptions) even with the module_exist check. Perhaps this can just be done with a variable_get on forum_nav_vocabulary?
• Comments need to be of the form // This is a comment. rather than //This is a comment.
• Quoting nids is not accepted practice here. As i found out recently, the CVS annotate function is used for all this.
• Please punctuate comments (add periods etc.).
• The first comment seems out of place/inaccurate. It makes more sense to have it above the if() IMO.

Thanks :)
-K

Hi Zen!

Thanks for your comments. But as you will see I disagree in some aspects ...

* remove node type 'forum' from 'edit vocabulary' page This should remain IMO. Adding other vocabularies to the forum forms works fine AFAIK and is something of a required feature.

NO, it does NOT work correctly! At the moment it is only valid to have one forum vocabulary. Try to create a second vocabulary with type forum in admin/taxonomy and look in /admin/forum and you will only see the first one. forum_nav_vocabulary stores only the vid of the original vocabulary (created by forum.module).
Additionally with forum node type showing up on vocabulary edit page you can create mixed-type vocabularies ... and these may have multi- or free-tagging enabled! Thats not what we intended, right!?

* Functions that begin with a _ are generally considered private methods. It is not *proper* to call them from external modules (with a few notable exceptions) even with the module_exist check. Perhaps this can just be done with a variable_get on forum_nav_vocabulary?

Yes, you are right. I also thought about that, but decided it would be better to use _forum_get_vid, because it adds additional logic to get_variable and always return a valid value for forum vocabulary. We could also use sth. like

$vocab = get_variable();
if (empty($vocab)) $vocab = 0;

* Comments need to be of the form // This is a comment. rather than //This is a comment.
* Quoting nids is not accepted practice here. As i found out recently, the CVS annotate function is used for all this.
* Please punctuate comments (add periods etc.).

I will remove all comments, it was just to explain what was added. I think they should not remain in code when it is being committed.

Similar patch as above, but without any comments and using get_variable()
instead of _forum_get_vid(). Patch still works the same ...
I think it looks much cleaner now ;)

Sorry, but patches that mention forum inside taxonomy.module won't get committed. Please use form API to rewrite the relevant taxonomy form and put that code into forum.module.

Function taxonomy_overview_vocabularies() does not use Form API to generate the table, but uses theme('table', ...) directly. That's why you can't hook_form_alter into that function at the moment. I will look at this more closely and try to make it work though. Thanks.

The new patch modifies function 'taxonomy_overview_vocabularies' in taxonomy.module using FormAPI to generate the vocabularies table and adds function 'theme_overview_vocabularies' to actually render the table. It also introduces hook_form_alter in forum.module to hide node type 'forum' on 'edit vocabulary' page and forum vocabulary on 'categories'.

I hope it complies with Drupal standards now!? Anyway it keeps taxonomy and forum stuff seperated.
Please let me know what else should be done ...

The patch looks great. It needs a review by somebody with some more clue about form API than myself and some testing.

Someone told me that as I have some idea of this form stuff I should review. First of all, loose #type 'markup'. That's the default so it's code bloat. Second put taxonomy into the theme function name. Third, use element_children() in the theme function for iterating over children.

And finally, why this super-duper deep form structure? Why ['vocabularies']['table'] is necessary?

One more note -- if you say "but markup is used at other places" and "element_children is not used everywhere" -- I know. See my devel list post is http://lists.drupal.org/pipermail/development/2006-January/013021.html for more.

I must admit I never used element_children() before and I always write #type 'markup', even though I know its default. Maybe I should change code in my modules as well! You live and learn! ;) Thanks. Hopefully this patch comes up to your expectations a little closer.

You could still lose the ['table'] from the form definition... and if you'd rename the function then you would not need #theme as the theme_form_id is always tried.

@chx: Have you got my mail? With the questions answered I could provide an even cleaner version. But for the moment I dont know exactly how to make it ...

After looking through some modules I think the last patch corresponds to FormAPI 'standard'.
Can anybody take a second look on this please.

It had minor problems, like using theme_table instead of theme('table',...) and you forgot the last form_render which does not bite you but could bite someone who would alter this form later.

Actually the patch doesnt work correctly. It worked perfectly with $form['table'][$vocabulary->vid], but it doesnt with $form[$vocabulary->vid]. What I do is to look for the $vid in hook_form_alter, which is identical to the array index of forum vocabulary. But when using $form directly (and no deeper level like ['table']) the form is restructured and the array is reindexed starting for 0. What means the array index is no longer equal $vocabulary->vid as it should be.

The problem lies in form.inc (line 82).
$form = array_merge(_element_info('form'), $form);
_element_info is called on form and adds additional data to the form array. Deeper levels however are not touched.
Is this a bug in FormAPI? Why is the form array renumbered?

Attached is a patch with $form['table'] (that DOES work) cleaned up according to chx patch.

Then form.inc needs patching, right :) ?

ok, I had a look at this patch. I am inclined to commit the first and last chunks of the most recnt patch (ie the changes to forms.inc and taxonomy.module) theses are in fact fixes to forms API and more conversion to use it.

The forum.module change doesn't do what I had in mind, though. I had only wanted it to remove the "multiple parents" setting from the forum vocabulary settings page. There are people who want to use one vocab throughout their whole site and I don't want to disable this feature.

The problem with only hiding 'multiple parents' is that users can create additional vocabs containing node type forum and thats not supported by forum.module (see _forum_get_vid). => form_alter, unset($form['nodes']['#options']['forum']); to hide node type 'forum'. But if you go to edit page of forum vocab with 'forum' unset and you hit submit forum will no be stored in enabled node types since it is not there. The only way to prevent this is to hook in _submit, but you cant do that from forum.module, right? => so better hide forum vocab completely and you are (almost) happy for the moment. I generally think forum vocab should be for forums only ... It is easy to only hide 'multiple parent' & Co

if ($form['vid']['#value'] == _forum_get_vid()) {
 unset($form['tags']); //disable free-tagging
 unset($form['multiple']); //disable multiple-parents
}

but it does not solve the problem!

I just read your IRC discussion on this topic. There's not much to add. For me it is critical if users can edit forum vocab directly (because of all reasons discussed in this thread, in worst case it breaks the forum). But preventing them from doing so leads to whole bunch of new problems. The only simple solution I see is to handle forum stuff through the forum.module only.

I have for now committed the forms API fix and the changes to taxonomy.module.

These are an improvement in any case.

For forum.module. it would make sense to

1) disable free tagging and multiple parents on the designated forum vocab form
and
2) remove the forum node choice from all other vocabs.

In both cases some informative texts for users should be added to minimize confusion.

Understood. New patch ...
- removes 'multile-parents' and 'free-tagging' from forum vocab
- sets 'required' to type 'hidden' (so people cant uncheck it)
- remove node type 'forum' from all but forum vocab
It would be great if we could gray out node type 'forum' to prevent people
from unchchecking in forum vocab, but its not possible atm I think.

thanks for keeping up. :)

i tested the patch:

on my forum vocab page I can still set it to "multiple".

the code style needs work

}
else {

not

} else {

there is no text explaining to the user why the settings are missing

There is no phpdoc code comment. (implementation of hook_form_alter) and I think the function should be move to where the other hook implementations are.

And again ... a new version of the patch above ...
Please try and tell me if it is what you had in mind!?

I looked at that again and I think its not a bad solution. It solves/hides most problems discussed here, but gives users the freedom to use forum vocabulary for other purposes, e.g. other node types. Can anyone test and comment on that? Dont look at the options only but also on the text!

Is that a problem, really? I've used my forum vocabulary for other node types from time to time, specifically when I DID want other node types implicitly connected to different forum sections. At the time I used it for 'related articles' links in the sidebar, etc.

It seems like removing options incompatible with forums solves the issue -- restricting the vocabulary to JUST forums might be going a bit far.

Ignore my previous post; I'd done that some time ago on a relatively low traffic site. Apparently there are problems with it that didn't affect me at the time.

I like the patch. The code looks solid. Testing it nothing is broken...

I think it is kosher to let other nodes user the forum vocabulary. It would be nice to remove the forum topic from the node selection list as you mentioned, but if admins disable it dis regarding the stern warning... that seems like their own problem.

So after some discussion with killes this was produced. We decided we we best to just hide the forum content type across the board... And remove the X/HTML from the form items.

Well hmm... I guess we should make sure the form is set if we're going to remove the option..

And since none of that worked.. I think I added a whole functional line to the original patch...
-> btw you can overload checkboxes....

Fixed two hiddens to be values and added a t().

applied