How to reproduce:
1) take durpal 6.x with forum and with taxonomy keeping the forum structure in "vocabulary_2" for example
2) update to drupal 7

Expected:
- the forum should work after upgrade

What happens:
1) when forum.module starts, it correctly reads the "Forum" field from the "taxonomy_vocabulary" table and creates (if needed) the "field_body_taxonomy_vocabulary_2" and "field_revision_taxonomy_vocabulary_2" tables.
2) when browsing forums, this information is also OK, you can see all the defined forums
3) when adding a new forum topic, however, I observed in the code of forum.module that the string "taxonomy_forums" is hardcoded. This leads to a serious problems. Topics are added, but they are not showing up in the forum categories. The reason for this is, that no data is written to the above mentioned 2 tables. There is no error message or log to indicate this problem. The topics can be seen in all other places, in content manager and so on.
4) when you try to edit such a topic, 2 notices occur. The first one is referring to undefined variables in forum.modul for "taxonomy_forums", which obviously does not exists. (sorry I've lost the exact error message). The second error is: Undefined property: stdClass::$forum_tid in forum_form()

Work-around:
1) disable forum module
2) change the name of the "Forum" value in "taxonomy_vocabulary" table to "forums"
3) start the forum module
4) copy all the content of your old tables ("field_body_taxonomy_vocabulary_2" and "field_revision_taxonomy_vocabulary_2") to the new once.

However, this issue should be solved before final is released.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Priority: Critical » Major

While this is definitely a big problem if you run into it, using a different vocabulary for forums than the default represents a significant enough edge case that I'm going to downgrade this to 'major' so it doesn't block release.

Still more than happy to commit patches that fix it in advance of release though!

webchick’s picture

Issue tags: +D7 upgrade path

Tagging.

Patkos Csaba’s picture

I think my case is not so rare. My drupal 6 with advanced forum installed had the vocabularies set up and configured by modules. I was never asked for a name for the vocabulary, so many others may have gone the same path I did and they have other than "forums" taxonomy vocabulary for their forums.

GStegemann’s picture

Maybe there is an simpler solution: I just changed the "machine name" of the forums vocabulray from "vocabulary_x" to "forums". But I'm not sure what other site effects this may have?

Yes, it has site effects. First the following warning is thrown:

Warning: Invalid argument supplied for foreach() in forum_menu_local_tasks_alter() (line 179 of /var/www/html/cm7/modules/forum/forum.module).

Second posts cannot be added anymore due to "missing" permissions.

GStegemann’s picture

I think, this can happen quite often.

Anyway, can you tell me which type of statements are affected? I.e. where the hardcoded "taxonomy_forums" has to be replaced somehow. There are several occurences.

federico’s picture

federico’s picture

Priority: Major » Critical

I'm marking this as critical, since I think:

- it is common to have a vocabulary different than the default; I was also never asked for a name for the vocabulary and my forum vocabulary name is vocabulary_9 (maybe its because we are upgrading since versions prior to d5?).

- it makes the forum almost unusable,

- and if new forum nodes are not appearing in 'forum' table, nor in 'forum index' table, it will be hard to get the forum topics created before the workaround to show in the forum.

I'm trying to apply the workaround, but when I go to node/add/forum I'm asked for 2 taxonomy terms: forum and vocabulary_9

bfroehle’s picture

Priority: Critical » Major
FileSize
674 bytes

Editing the machine name of a taxonomy vocabulary seems like a bad idea. However, I think you are correct that taxonomy_forum should not be hard coded into the module.

Here's a random stab at a patch. Completely un-tested. Maybe it solves this issue... maybe not.

federico’s picture

Hi, I applied the patch and it's not working for me.

bfroehle’s picture

federico: Can you be a little more specific.. what are you trying to do? what errors did you observe?

I agree that the proposed patch in #8 won't fix the problem caused by renaming the machine name of the taxonomy vocabulary for the forum, but I think it might address the issue in the original post.

webchick’s picture

Status: Active » Needs review
federico’s picture

Thanks bfroehle,

- I reverted the database to the original settings (no renaming of the forum taxonomy vocabulary machine name)
- then I applied your patch to forum.module
- I posted a new forum topic (new node)
- Result: new forum topic appear on taxonomy/term/tid but doesn't show up on forum categories listings.

By the way, in the first database (no patch applied) I was able to make new forum topics show up in the forum using the dirty workaround (one more step than #1) :
1- disable forum
2- rename the machine name of the forum vocabulary from "vocabulary_X" to "forums"
3- Inside "taxonomy_vocabulary" table, change the value "vocabulary_X" to "forums"
4- copy "field_data_taxonomy_X" to "field_data_forums" and "field_revisions_taxonomy_vocabulary_X" to "field_revisions_taxonomy_forums"
5- Inside table "field_data_taxonomy_forum" change the value "taxonomy_vocabulary_X_tid" to "taxonomy_forums_tid".
6- drop "field_data_taxonomy_vocabulary_X" and "field_revision_taxonomy_vocabulary_X".
7- start forum.module
where: vocabulary_X is the machine name of the forum vocabulary, for example: vocabulary_9

bfroehle’s picture

I missed an instance of taxonomy_forum being hardcoded. An updated version of the patch in #8. Still working on reproducing this locally, so it's hard for me to evaluate it's impact on #12.

bfroehle’s picture

Status: Needs review » Needs work

Okay, the more I look at this, the more backwards I think it seems... #13 is definitely _wrong_.

I think what we should be doing is hard-coding in taxonomy_forums everywhere. Or storing a variable which has the forum field name so we don't have to recompute it *everywhere*. Just search for '->taxonomy_forums' in modules/forum/* and you'll see dozens of references. So it seems improbable that we should have the field named something else, like 'taxonomy_vocabulary_12'.

This code change would be relatively easy... just replace the three existing occurrences of 'taxonomy_' . $vocabulary->machine_name with 'taxonomy_forums'. The problem though comes in providing an upgrade path for those affected by this issue. This would require calculating the field name:

$vid = variable_get('forum_nav_vocabulary', 0);
$vocabulary = taxonomy_vocabulary_load($vid);
$field_name = 'taxonomy_' . $vocabulary->machine_name;

and then renaming the field to 'taxonomy_forums'?

Or could we hijack node_load to reference taxonomy_forums to {'taxonomy_' . $vocabulary->machine_name}?

bfroehle’s picture

Status: Needs work » Needs review
FileSize
1.72 KB

Implements #14... essentially always hard coding in 'taxonomy_forums.' Would need an upgrade path for those affected by this issue...

bfroehle’s picture

Title: Forum module has 'taxonomy_forums' hardcoded in it leading to faulty functioning » Forum module cannot decide if it wants 'taxonomy_forums' hardcoded in it leading to faulty functioning
Status: Needs review » Needs work

Resurrected #628244: No way to attach the automatically created taxo field where this bug came from, as far as I know.

bfroehle’s picture

Status: Needs work » Needs review
FileSize
4.35 KB

This patch hard-codes in taxonomy_forums everywhere, and provides an upgrade path (at least for those using 'field_sql_storage'). Not for the faint of heart. This will need some testing, but don't even consider doing it on a production site.

svenrissmann’s picture

Hi,

I´m not realy sure if this helps, but:

1.) This error occurs (for me) only if I upgrade an existing site from Drupal 6 to 7! On a new fresh & clean Drupal 7 everything works fine!
2.) Even if I put my posts manualy into the tables forum and forum_index everything is still the same.
3.) Renaming the Forums Voc name isn´t realy helpful, once I´ve done I get the same error as posted in #4. After renaming the Voc back to vocabulary_5 (in my case) the error stays!

Hope this brings us one step closer...

P.S. Everything is tested on 7.0 by me!

GStegemann’s picture

I have applied this patch and run update.php. And it does look much better. At least all forum nodes are styled now correctly.

But there are two remaining errors:

  • error message that even User 1 cannot add new posts in this forum,
  • when selecting to display all "Active Posts" a message is displayed that an invalid vocabulary is selected, which is in fact not.
  • See attached screen shots.

    Tested on a migrated 6.20 site with Advanced Forum enabled.

    svenrissmann’s picture

    I can confirm this!
    But for my opinion this is definitly an AF issue now!

    federico’s picture

    Thanks a lot bfroehle for your wisdom and help!

    I have applied this patch and run update.php

    After that:

    - new forums nodes do appear in forum listings. (expected behavior)

    - in "/forum" page, the new post does show up, with the correct date/time of the last forum topic added (expected behavior)

    - in "/forum/tid" page, new post show up with "n/a" date/time and not sorted correctly, however, this seems to be another issue: #1008410: Forum list header sorting broken

    - I don't get any error regarding "cannot add new posts in this forum".

    - I don't receive any error log.

    I don't have AF installed.

    Edit: I've applied the patch #18 on #1008410: Forum list header sorting broken, now, after sorting the forums by date, new forum post do show up next to the header (as expected), the only issue (in my case), is that the date/time information is "n/a", but it seems to be another unrelated and minor issue.

    bfroehle’s picture

    federico: glad to see that the patch worked for you.

    Also, watch out for #1022924: Updates are broken for deleted fields which is mucking around with the _update_7000_field_... functions used in this patch.

    tonnyl’s picture

    I have the same issue after an upgrade from 6.2 to 7, I have now rolled back. I have not tried the patch, but will wait for a version 7.1, that hopefully have it fixed.

    catch’s picture

    Patch looks sane to me.

    It would be good to add to the upgrade path tests here, but not sure how much we already have for forum module to build on.

    Apart from that this looks RTBC.

    tonnyl’s picture

    If someone can describe exactly what to do, then I can do some more tests on a upgrade from 6.2 to 7.

    bfroehle’s picture

    tonnyl: In the upgrade process from D6 to D7, after you have extracted the Drupal 7.0 files (but before you run update.php), apply the patch in comment #17. Then run update.php, and continue.

    Then check to make sure you can browse all the forums and the taxonomy pages without any errors, create new forum posts, etc.

    tonnyl’s picture

    bfroehle: Thanks for this. I have now completed the update and everything seems to work - http://www.eve-o.dk/

    federico’s picture

    Status: Needs review » Needs work

    I've got an error in the patched site:

    I've disabled cache, and this error arose:

    Fatal error: Cannot redeclare forum_update_7002() (previously declared in /var/www/modules/forum/forum.install:339) in /var/www/modules/forum/forum.install on line 417

    412 /**
    413  * Add new index to forum_index table.
    414  */
    415 function forum_update_7002() {
    416  db_add_primary_key('forum_index', array('nid'));
    417 }
    
    335 /**
    336  * Rename field to 'taxonomy_forums'.
    337  */
    338 function forum_update_7002() {
    339  $messages = array();
    341  $new_field_name = 'taxonomy_forums';
    

    I've removed lines 412 to 417 and the error is gone

    bfroehle’s picture

    Status: Needs work » Needs review

    federico: The other hunk of code you mention on lines 412-417 isn't official in the Drupal CVS repository. Did it come form another patch you were considering?

    bfroehle’s picture

    Issue tags: +Needs tests

    From @catch in #25:

    Patch looks sane to me.

    It would be good to add to the upgrade path tests here, but not sure how much we already have for forum module to build on.

    Apart from that this looks RTBC.

    Adding test routines would be challenging -- the changes to forum_enable() and forum_menu_local_tasks_alter() are enough to make sure things work on D6->D7 upgrades or new installs. The provided forum_update_7002() routine is only to rescue those with broken configurations from a previous botched upgrade. Perhaps we omit the update functionality, or just post it as an attachment here and suggest folks run it with drush scr fix-forums.php?

    catch’s picture

    I was thinking we ought to be able to follow the steps to reproduce from the first post as part of the 6-7 forum upgrade tests - not sure if the existing databases we have are enough to reproduce the bug with. Or am I missing something?

    bfroehle’s picture

    The existing D6 database dumps in simpletest's upgrade tests do not have any forum content.

    bfroehle’s picture

    Here's a start to a framework for testing forum upgrades. It's failing for all the wrong reasons so far. Hopefully somebody else can carry the torch for a bit on this one.

    Status: Needs review » Needs work

    The last submitted patch, 1003308-33-start-forum-upgrade-test-framework.patch, failed testing.

    federico’s picture

    Reply to #29:

    Yes, lines 412-417 come from patch in http://drupal.org/node/1008410#comment-3894212

    matthaus’s picture

    Most of the comments indicate that this patch gets applied to the 7.0 upgrade before applying. Is there any way to apply this patch to an already upgraded Drupal instance? (I'm pretty sure I'm running into this issue on a recently upgraded site.) Thanks much.

    bfroehle’s picture

    You can apply the patch in #17 and it should fix any problems you were having.

    Later patches were for test routines to try to verify that patch.

    bfroehle’s picture

    Status: Needs work » Needs review
    matthaus’s picture

    Thanks a bunch! I applied the patch and it fixed the issue.

    GStegemann’s picture

    But I still have the errors reported in #17 and what about the duplicated update functions mentioned in #28?

    Anyway, I renamed this one

    335 /**
    336  * Rename field to 'taxonomy_forums'.
    337  */
    338 function forum_update_7002() {
    ...
    

    to "forum_update_7003()".

    Possibly both update functions are needed for the forum module.

    bfroehle’s picture

    @GStegemann: There is no forum_update_7002() yet. See http://api.drupal.org/api/drupal/modules--forum--forum.install/7

    The patch in #17 applies to 7.x cleanly and passes validation http://qa.drupal.org/pifr/test/127244.

    And from @catch in #25:

    Patch looks sane to me.

    It would be good to add to the upgrade path tests here, but not sure how much we already have for forum module to build on.

    Apart from that this looks RTBC.

    #33 was the beginning of tests for this, but writing tests (for this issue) is a tiresome task that I do not have time to pursue at the moment.

    GStegemann’s picture

    OK, lets say forum_update_7002() was an interims update in your patch and is not going to be an offical update of the forum module, and will be later replaced by an upgrade path test.

    However, but what can be the reason of the following problems and how can they fixed?

  • error message that even User 1 cannot add new posts in this forum,
  • when selecting to display all "Active Posts" a message is displayed that an invalid vocabulary is selected, which is in fact not.

  • What needs to be checked?

    bfroehle’s picture

    @GStegemann:

    You are right that in the end there will be only one forum_update_7002(). There must be another issue floating around that needs an upgrade path for the forum module. Whichever gets committed first will get forum_update_7002(), the other will be renamed to forum_update_7003().

    Now as to your issues:

    error message that even User 1 cannot add new posts in this forum,
    when selecting to display all "Active Posts" a message is displayed that an invalid vocabulary is selected, which is in fact not.

    .

    The first error seems unrelated to this issue. The second seems related. Can you be a lot more specific about how you can reproduce this? In particular, have you applied the patch in #17 and run the upgrade path? If you've been testing lots of patches, you might need to manually edit {system} to change the forum database schema number to 7001 so that you can run the upgrade path again.

    GStegemann’s picture

    Thanks for your clarification.

    Regarding the issue:

    when selecting to display all "Active Posts" a message is displayed that an invalid vocabulary is selected, which is in fact not.

    This error occured the first time after applying patch #17. Before I have seen other errors, as reported in #345387: Forum topic will not display and #1040114: New posts are not added to tables forum and forum_index. Yes, I ran the upgrade path several times, i.e. resetting the schema version before running update.php. I have no idea how to be more specific since I don't all the internas of Drupal. Maybe its a site affect of the migration from Drupal 6.20 to D7.

    bfroehle’s picture

    On the recommendation of @catch:

    I was thinking we ought to be able to follow the steps to reproduce from the first post as part of the 6-7 forum upgrade tests - not sure if the existing databases we have are enough to reproduce the bug with. Or am I missing something?

    I have done this, and have attached two patches to this issue:

    1. Test routines: Based upon a database dump of D6, I created a rudimentary upgrade test routine for forum. Currently it has one forum topic "Apples" in a forum "Fruit". We test upgrading, editing the existing forum topic "Apples", and creating a new forum topic "Bananas". We successfully capture both errors in the original post, namely the inability to add forum topics and the undefined stdClass::$taxonomy_forums / stdClass::$forum_tid notices.
    2. Test routines and fix: The fix is patch #17, namely to hard code the field name of "taxonomy_forums" always and provide an upgrade routine.

      Now there is one slight problem with hard coding "taxonomy_forums", namely the field upgrade mechanism will create a field named 'taxonomy_' . $vocabulary->machine_name which will need to be renamed to taxonomy_forums.

      I've provided an upgrade routine, forum_update_7002() which will rename the field, at least for those using SQL field storage.

    bfroehle’s picture

    @GStegemann: Pardon my ignorance, but how are you 'selecting to display all "Active Posts"' ?

    GStegemann’s picture

    No problem. I'm also using the module Advanced Forum. There are several additional buttons, and one is called "Active Posts". Please have a look at comment #19. There I have attached screen shots showing the two issues.

    catch’s picture

    Issue tags: +Needs backport to D7

    Test looks great. I just realised one more thing that needs to happen here though (sorry bfroehle). Since the upgrade is being added after 7.0, we need to put it in a 6.x-7.x extra defgroup (should be one of these in core already for 5.x-6.x extra). That also means there should be a D8 version of the patch (with forum_update_7002() in that defgroup, no test hunks). Everything looks great otherwise.

    bfroehle’s picture

    @catch: What, specifically, should be the doxygen group name? updates-7.x-extra ? updates-6.x-to-7.x-extra ?

    Also, in regards to the 8.x version, why is it necessary to include forum_update_7002()? Why not just set forum_update_last_removed() and put the upgrade routine in head2head?

    catch’s picture

    Hmm, I forgot we managed to get #760014: Stop supporting direct updates from Drupal < 6.17 committed. In that case forum_update_last_removed() is a much better idea than forward-porting the update.

    defgroup should be "updates-7.x-extra".

    bfroehle’s picture

    Two patches attached to this issue

    1. 1003308-Fix-inconsistent-use-of-taxonomy_forum-D8.patch: For use in Drupal 8. Includes only the fixes to the forum code and an implementation of hook_update_last_removed() [to ensure that forum_update_7002() was run in D7 before upgrading]. The upgrade test routines and forum_update_7002() are not included.
    2. 1003308-Fix-inconsistent-use-of-taxonomy_forum-D7.patch: For use in Drupal 7. Includes upgrade test routines, forum_update_7002() in an @addtogroup updates-7.x-extra block, and the actual fixes to the forum code.
    bfroehle’s picture

    This is 1003308-Fix-inconsistent-use-of-taxonomy_forum-D7.patch from #51, but renamed so that testing will occur.

    Good descriptions of the problem are in the original post and in #45.

    bfroehle’s picture

    Version: 7.x-dev » 8.x-dev
    FileSize
    2.31 KB

    This is 1003308-Fix-inconsistent-use-of-taxonomy_forum-D8.patch from #51 but renamed so that test occurs.

    catch’s picture

    Status: Needs review » Reviewed & tested by the community

    Looks great.

    tonnyl’s picture

    I did the patch from #17. Should I use the new patch now or shall I just wait for the next official 7.x release?

    bfroehle’s picture

    Dries’s picture

    Version: 8.x-dev » 7.x-dev

    Committed the 8.x patch to 8.x Moving to 7.x for @webchick.

    webchick’s picture

    Version: 7.x-dev » 8.x-dev
    Priority: Major » Normal
    Status: Reviewed & tested by the community » Needs work
    Issue tags: -Needs backport to D7

    Reviewed this patch. Thanks for the awesome tests, bfroehle!

    -      $vid = variable_get('forum_nav_vocabulary', 0);
    -      $vocabulary = taxonomy_vocabulary_load($vid);
    

    At first I thought this hunk was hard-coding the forum taxonomy, but that $vid = variable_get('forum_nav_vocabulary', 0); pattern persists throughout the code base; the only place we're hard-coding is the actual machine name, and I think that's fair enough.

    I added some periods to comments in forum_update_7003() (which is what I renamed 7002() to, since 7002() already exists) and committed to 7.x. Thanks SO much!

    I believe this now needs to be kicked back to 8.x to adjust +function forum_update_last_removed() { to 7003 rather than 7002?

    bfroehle’s picture

    Status: Needs work » Needs review
    FileSize
    338 bytes

    As webchick mentioned, the update function went into D7 as forum_update_7003(), therefore we need to bump the version number of hook_update_last_removed() in D8.

    Now there has been some talk about letting this schema / version synchronization fall apart only to be recreated at a later date. I'm not sure this is a good idea, so I've attached a patch for Drupal 8 here.

    andypost’s picture

    FileSize
    612 bytes

    Same patch with a small cleanup for forum_enable() - eliminate unusable var_set()

    bfroehle’s picture

    The variable_set() is not so much unusable as it is duplicitous to another identical call a few lines prior.

    andypost’s picture

    If variable already set then no reson to set it up again. If not set before enable then inited and set within IF statement

    bfroehle’s picture

    Status: Needs review » Reviewed & tested by the community

    So this fix-up to the 8.x branch is RTBC. (And should get kicked back to 7.x as patch (to be ported) after being committed).

    saparker’s picture

    @GStegemann:

    I have applied the patch in #17 successfully, but also have your remaining problems (comments #19, #42. #44):
    1. error message that even User 1 cannot add new posts in this forum,
    2. when selecting to display all "Active Posts" a message is displayed that an invalid vocabulary is selected, which is in fact not.

    I also have a possibly related issue:
    3. The icon next to each forum topic is a padlock, indicating that the topic is closed to comments. However, the topics are available for commenting.

    I too am running Advanced Forum with D7. I'm also using Taxonomy Access Control Lite as an access control module.

    Did you manage to find a fix for these matters?

    bfroehle’s picture

    saparker: I suspect you'll be better served by creating a new support request / bug report in the Advanced Forum issue queue since the errors you describe are not in features provided by the core forum.module.

    GStegemann’s picture

    @saparker:

    Did you manage to find a fix for these matters?

    No, not yet. Still waiting for fixes.

    Dries’s picture

    Version: 8.x-dev » 7.x-dev

    I committed the patch in #60 to 8.x. Back to webchick for 7.x.

    webchick’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed the first hunk of #60 to 7.x. I think this is good to go now. Thanks!

    Status: Fixed » Closed (fixed)

    Automatically closed -- issue fixed for 2 weeks with no activity.

    Liam_Mc’s picture

    Hi,

    I'm having a similar problem to the one described at the start of this thread after upgrading from 6.22 to 7.8 and then 7.9. I don't have Advanced Forum installed and I'm a bit confused as to whether I need to apply one of the patches above. I think my problem is related to the the forum taxonomy terms that seem to have been bundled into a 'Taxonomy upgrade extras' vocabulary after the upgrade. There isn't much activity in the forum, but I would prefer not to have to wipe it clean and start again, if possible.
    The problems can be seen here: http://www.sustainablebrampton.org/forum

    Any help would be much appreciated.

    Many thanks,

    Liam