Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I found a few queries that use the outdated "taxonomy_term_node" table. I also changed these queries to be dynamic and tagged them appropriately, in case people need to alter them.
See patch in comment #1.
Comment | File | Size | Author |
---|---|---|---|
#20 | 610022_taxonomy_token.patch | 7.42 KB | mcarbone |
#18 | 610022_taxonomy_token.patch | 7.85 KB | mcarbone |
#16 | 610022_taxonomy_token.patch | 7.94 KB | mcarbone |
#8 | taxonomy-610022-8.patch | 2.2 KB | te-brian |
#2 | taxonomy-610022-2.patch | 2.2 KB | David_Rothstein |
Comments
Comment #1
te-brian CreditAttribution: te-brian commentedHere is a patch that addresses the issues outlined in the issue description. Most notable is the change to dynamic queries and the updated table names ('taxonomy_term_node' to 'taxonomy_index').
I tested the three tokens and they seem to work.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedLooks pretty good... in the attached patch, I cleaned up the queries a bit (removed some aliases and variable assignments that didn't seem to be necessary, and used a simpler $query->condition rather than $query->where). I also replaced COUNT(1) with COUNT(*) -- COUNT(1) was there before this patch, but we really always use COUNT(*) everywhere else in Drupal as far as I know, so I figured I'd fix it while we were touching this.
I tested these and they seemed to work. However, my testing also revealed another bug in this part of the code; we were double-counting nodes that had more than one term, so I added a DISTINCT which looks to me like the correct fix.
Also, it seems to me that a database query against a table that no longer exists is a critical bug.... fatal errors aren't nice :)
Comment #3
te-brian CreditAttribution: te-brian commentedI can confirm that applying David's version of the patch fixes the use case where I originally found this bug. Much cleaner too :)
Comment #4
xslim CreditAttribution: xslim commentedI hope they will commit this patch !
Comment #5
Dries CreditAttribution: Dries commentedIt looks like we need tests for this?
Comment #6
xslim CreditAttribution: xslim commentedit works ok for me
Comment #7
pwolanin CreditAttribution: pwolanin commentedCOUNT (1) or COUNT(tid) should perform better than COUNT(*) on innodb - why would we change to COUNT(*)?
Comment #8
te-brian CreditAttribution: te-brian commentedChanged to COUNT (1). We still need more tests for tokens in general. Not sure if that is in this patch or another though.
[Edit] Most likely, the previous patch should be used. COUNT(1) has been debunked. See #11.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedNote (see above) that the standard throughout core is to use COUNT(*) rather than COUNT(1).
COUNT(1) might be better, but it isn't used anywhere else.
Comment #10
te-brian CreditAttribution: te-brian commentedTo be honest, I don't have extensive knowledge of the performance differences between the two, but if Count (1) really is better, should it not be used wherever possible?
Comment #11
mcarbone CreditAttribution: mcarbone commentedFrom O'Reilly's High Performance MySQL (2nd edition):
Using the column name can have unintended consequences, as it ignores rows with a NULL value in that column. They don't say this, but I think COUNT(1) is probably equivalent to COUNT(*) in terms of performance, but * seems to be the clearest use.
So I think we can leave this as "needs work," since as Dries indicated, it needs tests, but I don't think the COUNT function needs to be changed.
Comment #12
mcarbone CreditAttribution: mcarbone commentedAlso, I just did a grep and it seems like the Drupal standard is to use COUNT(*). (115 lines in core code that use it, as opposed to only 3 that use COUNT(1), which happen to be in the taxonomy_tokens include file.) So not to be a nuisance, te-brian, but I think you should switch it back.
Comment #13
mcarbone CreditAttribution: mcarbone commentedAlternatively, you could do:
$count = $query->countQuery()->execute()->fetchField();
And get rid of the addExpression entirely. (countQuery() adds the COUNT(*) for you.)
Comment #14
te-brian CreditAttribution: te-brian commented@mcarbone
Thanks for the references. I guess we can tally up the vote as 2-1 in favor of COUNT(*) :)
No need to re-roll, just disregard #8.
Comment #15
mcarbone CreditAttribution: mcarbone commentedI'm working on some tests for this.
Comment #16
mcarbone CreditAttribution: mcarbone commentedAdded tests for all term and vocabulary tokens. Fixed a bug in the [term:url] token along the way.
Comment #17
Dries CreditAttribution: Dries commentedShouldn't we use countQuery() instead of adding an expression ourselves?
Otherwise looks great to me. Good job.
Comment #18
mcarbone CreditAttribution: mcarbone commentedAgreed. Re-rolled using countQuery(). (But one of them still has to explicitly add the count expression since it also uses DISTINCT.)
Comment #19
sun.core CreditAttribution: sun.core commentedLooks almost ready!
Trailing white-space here.
...and that's why we don't write inline comments on the same line. Most of those comments can be removed, because they don't really explain anything that isn't totally obvious from the code.
Exceeds 80 chars.
Powered by Dreditor.
Comment #20
mcarbone CreditAttribution: mcarbone commentedRe-rolled with fixes incorporated from #19. I wasn't entirely sure what line you were referring to with the trailing white-space comment, but hopefully I fixed it.
Comment #21
Dries CreditAttribution: Dries commentedThis looks ready now. Committed to CVS HEAD. Thanks.