This is blocking #412518: Convert taxonomy_node_* related code to use field API + upgrade path and a result of #552716: Impossible to join or order by on field tables without using Views short version is that due to pluggable field storage, we can't reliably join on field tables (without Views in core at least), which means when {taxonomy_term_node} is replaced by field_* storage, all queries joining on that table need to be rewritten to take this into account.

Fortunately, forum modules keeps it's own copy of term_node anyway, so we can join on that instead. I seem to remember us removing the {forum} table in one release, then putting it back again - if there's something which is broken by doing this, then it's not tested, since all tests pass with this patch.

While rolling this, I also found out forum.module still implements hook_taxonomy(), which no longer exists, so there's definitely some untested mess in here as well.

A nice side effect of this is it opens up the way to do some of the work from #145353: Forums performance improvements - i.e. adding node and last_comment_timestamp to the {forum} table to remove temp tables and filesorts from all these queries.

Would be great to get a quick commit here to keep the term fields patch from being too monstrous, if not we'll have to merge this in most likely.

CommentFileSizeAuthor
#13 forum.patch1.62 KBmfb
#7 forum.patch861 bytesmfb
forum.patch6.82 KBcatch

Comments

bjaspan’s picture

subscribe

Anonymous’s picture

on it.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Needs review

Oh, testing bot...

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Looks reasonable - essentially just substituting the table for the join.

dries’s picture

Status: Reviewed & tested by the community » Fixed

Alright. Committed.

mfb’s picture

Status: Fixed » Needs review
StatusFileSize
new861 bytes

Found a typo in forum_get_forums() which causes incorrect topic and post counts.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Ouch. Forum test coverage is a bit ropey :(

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Let's fill out our test coverage then, please.

catch’s picture

Forum tests are more or less beyond repair, and my description at #251235: forum.test needs a cleanup is still pretty accurate 18 months later, so it might not be easy to add coverage without completely rewriting the tests.

mfb’s picture

Status: Needs work » Needs review
StatusFileSize
new1.62 KB

I added a couple assertions verifying the topic and post counts on the forum page. Both fail without this patch.

catch’s picture

Status: Needs review » Reviewed & tested by the community

OK maybe not a rewrite then, that test still makes my eyes bleed though.

dries’s picture

I just committed #251235: forum.test needs a cleanup so I'm asking the test-bot to retest this patch.

dries’s picture

Status: Reviewed & tested by the community » Fixed

Tests passed, both using the test bot and on my localhost. Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests

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