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.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | forum.patch | 1.62 KB | mfb |
| #7 | forum.patch | 861 bytes | mfb |
| forum.patch | 6.82 KB | catch |
Comments
Comment #1
bjaspan commentedsubscribe
Comment #2
Anonymous (not verified) commentedon it.
Comment #4
webchickOh, testing bot...
Comment #5
pwolanin commentedLooks reasonable - essentially just substituting the table for the join.
Comment #6
dries commentedAlright. Committed.
Comment #7
mfbFound a typo in forum_get_forums() which causes incorrect topic and post counts.
Comment #8
catchOuch. Forum test coverage is a bit ropey :(
Comment #10
catchComment #11
webchickLet's fill out our test coverage then, please.
Comment #12
catchForum 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.
Comment #13
mfbI added a couple assertions verifying the topic and post counts on the forum page. Both fail without this patch.
Comment #14
catchOK maybe not a rewrite then, that test still makes my eyes bleed though.
Comment #15
dries commentedI just committed #251235: forum.test needs a cleanup so I'm asking the test-bot to retest this patch.
Comment #16
dries commentedTests passed, both using the test bot and on my localhost. Committed to CVS HEAD. Thanks!