Revision 1.407, #20295: Allow forum topics to be custom node types removed the {forum} table and turned

function forum_load($node) {
  $forum = db_fetch_object(db_query('SELECT * FROM {forum} WHERE vid = %d', $node->vid));
  return $forum;
}

into 

function forum_load($node) {
  $forum = db_fetch_object(db_query('SELECT * FROM {term_node} WHERE vid = %d', $node->vid));
  return $forum;
}

This sets $node->tid to the first tid that comes along, which is not necessarily the tid from the forum vocabulary, leading to strangeness as in #410930: forum access is not working if another taxonomy is assigned to forum posts.

chx put the {forum} table back in with 1.418 and explained it well in #172643-4: Beta breaker: Forum: Leave shadow copy doesn't leave shadow copy., but he forgot to roll back forum_load().

The attached patch fixes the forum nodes. Weighing the simplicity of the patch against its usefulness, I'm marking this critical.

This issue is not present in D7.

CommentFileSizeAuthor
#8 forum_module-forum_nodeapi.D6.patch782 bytesjeremiah.snapp
forum_module-forum_load.D6.patch647 bytessalvis
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

OHGE’s picture

OHGE CreditAttribution: OHGE commented

fixed some troubles I had :)

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Version: 6.x-dev » 7.x-dev
Status: Needs review » Patch (to be ported)

*Sigh*

First D7, then D6.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Version: 7.x-dev » 6.x-dev
Status: Patch (to be ported) » Needs review

*Sigh* — as I wrote above, this is not an issue for D7.

forum_load() is gone from D7. $node->tid is set in forum_node_view() with...

      foreach ($node->taxonomy as $term_id => $term) {
        if (in_array($term_id, $forum_terms)) {
          $node->tid = $term_id;
        }
      }

... so it gets the correct tid.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

What can we do to get this obvious bug fixed?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

This indeed seems to be fixed in D7 already: http://api.drupal.org/api/function/forum_node_load/7

http://api.drupal.org/api/function/forum_nodeapi/6 already uses {forum}, too.

Only http://api.drupal.org/api/function/forum_load/6 seems to be still using the {term_node} table.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Status: Fixed » Closed (fixed)

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

jeremiah.snapp’s picture

jeremiah.snapp CreditAttribution: jeremiah.snapp commented
Status: Closed (fixed) » Needs review
FileSize
forum_module-forum_nodeapi.D6.patch782 bytes

The attached patch modifies the existing sql statement in the 'presave' case in forum_nodeapi to use {forum} instead of {term_node}.

I'm not sure but I wonder why the 'presave' case in forum_nodeapi can't use the more simple sql statement the forum_load function uses.
The current method of joining tables seems unnecessary to me but I might be missing something.
If others decide the more simple sql query should be used then we may want to consider porting this forward to D7's forum_node_presave function.

Either way it should be using the {forum} table instead of {term_node}.

This issue is not present in D7.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Priority: Critical » Major

Looks to be the same change as in the original patch, just a different query.

jeremiah.snapp’s picture

jeremiah.snapp CreditAttribution: jeremiah.snapp commented

This is for the 'presave' case in forum_nodeapi. The original patch was for forum_load.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

In forum_load() we already know which revision id to get. The code in 'presave' makes sure it gets the latest vid. We might be editing an older revision (which had a different tid), but when saving, we want to replace the latest revision (i.e. the tid that goes with the latest vid). So, no, it cannot be simplified.

The forum_nodeapi() code was newly introduced in the same revision 1.407 as the original bug of this thread and fixed in 1.418 to its present state, and I agree that it's the same bug.

+1 for the patch in #8.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Status: Needs review » Reviewed & tested by the community

Let's fix this. It can cause nasty errors when trying to move a forum topic from one forum into another. The "shadow copy" left behind may end up with a tid that is not a forum, if the forum topics have additional vocabularies!

It's difficult to construct a test for this that would reliably fail if it were broken. The reason is that the current code retrieves the tid from {term_node}. If we're lucky, the first (and only retrieved row) happens to actually give the forum_tid, but it might also give the tid from another vocabulary.

The order of the records in {term_node} is not defined and impossible to control in a DB-independent way, so we cannot create a test that reliably returns the bad tid.

jeremiah.snapp’s picture

jeremiah.snapp CreditAttribution: jeremiah.snapp commented

@salvis Thanks for supporting this. This is the first patch I've submitted to core. Is it likely to be difficult getting it approved. Anything I can do to help?

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

Having your patch tested by the testbot would help a lot. I'm not sure why the test bot is ignoring it.

Try re-uploading it and remove ".D6" from the filename.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Testbot doesn't review D6 patches. We didn't get a testing framework in core until D7, so that's by design.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

I thought so, too, but then I saw
#345080-24: term_node table is empty after upgrading from Drupal 5 to 6 and
#746440-4: ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list, for example.

Those are D6 patches and according to the test report they were tested against D6.

There are lots of others, but they all have one thing in common: they all FAILED. I haven't found any D6 patch that PASSED, only Ignored and FAILED ones. Strange...

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Looks like a simple change similar to the previous patch. Thanks, committed.

Status: Fixed » Closed (fixed)

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