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.
Comment | File | Size | Author |
---|---|---|---|
#60 | 1003308-forum-enable.patch | 612 bytes | andypost |
#59 | 1003308-D8-fixup.patch | 338 bytes | bfroehle |
#53 | d8-1003308-Fix-inconsistent-use-of-taxonomy_forum.patch | 2.31 KB | bfroehle |
#52 | 1003308-Fix-inconsistent-use-of-taxonomy_forum.patch | 13.35 KB | bfroehle |
#51 | 1003308-Fix-inconsistent-use-of-taxonomy_forum-D8.patch | 2.31 KB | bfroehle |
Comments
Comment #1
webchickWhile 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!
Comment #2
webchickTagging.
Comment #3
Patkos Csaba CreditAttribution: Patkos Csaba commentedI 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.
Comment #4
GStegemann CreditAttribution: GStegemann commentedMaybe 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:
Second posts cannot be added anymore due to "missing" permissions.
Comment #5
GStegemann CreditAttribution: GStegemann commentedI 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.
Comment #6
federico CreditAttribution: federico commentedIs this related to #345387: Forum topic will not display ?
Comment #7
federico CreditAttribution: federico commentedI'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
Comment #8
bfroehle CreditAttribution: bfroehle commentedEditing 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.
Comment #9
federico CreditAttribution: federico commentedHi, I applied the patch and it's not working for me.
Comment #10
bfroehle CreditAttribution: bfroehle commentedfederico: 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.
Comment #11
webchickComment #12
federico CreditAttribution: federico commentedThanks 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
Comment #13
bfroehle CreditAttribution: bfroehle commentedI 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.
Comment #14
bfroehle CreditAttribution: bfroehle commentedOkay, 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:and then renaming the field to 'taxonomy_forums'?
Or could we hijack node_load to reference taxonomy_forums to {'taxonomy_' . $vocabulary->machine_name}?
Comment #15
bfroehle CreditAttribution: bfroehle commentedImplements #14... essentially always hard coding in 'taxonomy_forums.' Would need an upgrade path for those affected by this issue...
Comment #16
bfroehle CreditAttribution: bfroehle commentedResurrected #628244: No way to attach the automatically created taxo field where this bug came from, as far as I know.
Comment #17
bfroehle CreditAttribution: bfroehle commentedThis 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.
Comment #18
svenrissmann CreditAttribution: svenrissmann commentedHi,
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!
Comment #19
GStegemann CreditAttribution: GStegemann commentedI 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:
See attached screen shots.
Tested on a migrated 6.20 site with Advanced Forum enabled.
Comment #20
svenrissmann CreditAttribution: svenrissmann commentedI can confirm this!
But for my opinion this is definitly an AF issue now!
Comment #21
federico CreditAttribution: federico commentedThanks 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.
Comment #22
bfroehle CreditAttribution: bfroehle commentedfederico: 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.
Comment #23
tonnyl CreditAttribution: tonnyl commentedI 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.
Comment #24
catchPatch 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.
Comment #25
tonnyl CreditAttribution: tonnyl commentedIf someone can describe exactly what to do, then I can do some more tests on a upgrade from 6.2 to 7.
Comment #26
bfroehle CreditAttribution: bfroehle commentedtonnyl: 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.
Comment #27
tonnyl CreditAttribution: tonnyl commentedbfroehle: Thanks for this. I have now completed the update and everything seems to work - http://www.eve-o.dk/
Comment #28
federico CreditAttribution: federico commentedI've got an error in the patched site:
I've disabled cache, and this error arose:
I've removed lines 412 to 417 and the error is gone
Comment #29
bfroehle CreditAttribution: bfroehle commentedfederico: 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?
Comment #30
bfroehle CreditAttribution: bfroehle commentedFrom @catch in #25:
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
?Comment #31
catchI 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?
Comment #32
bfroehle CreditAttribution: bfroehle commentedThe existing D6 database dumps in simpletest's upgrade tests do not have any forum content.
Comment #33
bfroehle CreditAttribution: bfroehle commentedHere'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.
Comment #35
federico CreditAttribution: federico commentedReply to #29:
Yes, lines 412-417 come from patch in http://drupal.org/node/1008410#comment-3894212
Comment #36
matthaus CreditAttribution: matthaus commentedMost 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.
Comment #37
bfroehle CreditAttribution: bfroehle commentedYou 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.
Comment #38
bfroehle CreditAttribution: bfroehle commented#17: 1003308-the-god-patch-or-at-least-it-took-hours-to-code.patch queued for re-testing.
Comment #39
matthaus CreditAttribution: matthaus commentedThanks a bunch! I applied the patch and it fixed the issue.
Comment #40
GStegemann CreditAttribution: GStegemann commentedBut I still have the errors reported in #17 and what about the duplicated update functions mentioned in #28?
Anyway, I renamed this one
to "forum_update_7003()".
Possibly both update functions are needed for the forum module.
Comment #41
bfroehle CreditAttribution: bfroehle commented@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:
#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.
Comment #42
GStegemann CreditAttribution: GStegemann commentedOK, 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?
What needs to be checked?
Comment #43
bfroehle CreditAttribution: bfroehle commented@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:
.
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.
Comment #44
GStegemann CreditAttribution: GStegemann commentedThanks for your clarification.
Regarding the issue:
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.
Comment #45
bfroehle CreditAttribution: bfroehle commentedOn the recommendation of @catch:
I have done this, and have attached two patches to this issue:
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 totaxonomy_forums
.I've provided an upgrade routine,
forum_update_7002()
which will rename the field, at least for those using SQL field storage.Comment #46
bfroehle CreditAttribution: bfroehle commented@GStegemann: Pardon my ignorance, but how are you 'selecting to display all "Active Posts"' ?
Comment #47
GStegemann CreditAttribution: GStegemann commentedNo 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.
Comment #48
catchTest 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.
Comment #49
bfroehle CreditAttribution: bfroehle commented@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?
Comment #50
catchHmm, 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".
Comment #51
bfroehle CreditAttribution: bfroehle commentedTwo patches attached to this issue
Comment #52
bfroehle CreditAttribution: bfroehle commentedThis 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.
Comment #53
bfroehle CreditAttribution: bfroehle commentedThis is 1003308-Fix-inconsistent-use-of-taxonomy_forum-D8.patch from #51 but renamed so that test occurs.
Comment #54
catchLooks great.
Comment #55
tonnyl CreditAttribution: tonnyl commentedI did the patch from #17. Should I use the new patch now or shall I just wait for the next official 7.x release?
Comment #56
bfroehle CreditAttribution: bfroehle commentedAnother older issue related to this: #895774: Forum field should not rely on vocabulary machine name.
Comment #57
Dries CreditAttribution: Dries commentedCommitted the 8.x patch to 8.x Moving to 7.x for @webchick.
Comment #58
webchickReviewed this patch. Thanks for the awesome tests, bfroehle!
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?Comment #59
bfroehle CreditAttribution: bfroehle commentedAs 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.
Comment #60
andypostSame patch with a small cleanup for forum_enable() - eliminate unusable var_set()
Comment #61
bfroehle CreditAttribution: bfroehle commentedThe variable_set() is not so much unusable as it is duplicitous to another identical call a few lines prior.
Comment #62
andypostIf variable already set then no reson to set it up again. If not set before enable then inited and set within IF statement
Comment #63
bfroehle CreditAttribution: bfroehle commentedSo 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).
Comment #64
saparker CreditAttribution: saparker commented@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?
Comment #65
bfroehle CreditAttribution: bfroehle commentedsaparker: 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.
Comment #66
GStegemann CreditAttribution: GStegemann commented@saparker:
No, not yet. Still waiting for fixes.
Comment #67
Dries CreditAttribution: Dries commentedI committed the patch in #60 to 8.x. Back to webchick for 7.x.
Comment #68
webchickCommitted the first hunk of #60 to 7.x. I think this is good to go now. Thanks!
Comment #70
Liam_Mc CreditAttribution: Liam_Mc commentedHi,
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