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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

te-brian’s picture

FileSize
2.37 KB

Here 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.

David_Rothstein’s picture

Priority: Normal » Critical
FileSize
2.2 KB

Looks 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 :)

te-brian’s picture

I can confirm that applying David's version of the patch fixes the use case where I originally found this bug. Much cleaner too :)

xslim’s picture

I hope they will commit this patch !

Dries’s picture

Issue tags: +Needs tests

It looks like we need tests for this?

xslim’s picture

it works ok for me

pwolanin’s picture

Status: Needs review » Needs work

COUNT (1) or COUNT(tid) should perform better than COUNT(*) on innodb - why would we change to COUNT(*)?

te-brian’s picture

FileSize
2.2 KB

Changed 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.

David_Rothstein’s picture

Note (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.

te-brian’s picture

To 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?

mcarbone’s picture

From O'Reilly's High Performance MySQL (2nd edition):

The most obvious example is COUNT(*), which is a special form of COUNT() that does not expand the * wildcard into the full list of columns in the table, as you might expect; instead, it ignores columns altogether and counts rows.

One of the most common mistakes we see is specifying column names inside the parentheses when you want to count rows. When you want to know the number of rows in the result, you should always use COUNT(*). This communicates your intention clearly and avoids poor performance.

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.

mcarbone’s picture

Also, 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.

mcarbone’s picture

Alternatively, you could do:

$count = $query->countQuery()->execute()->fetchField();

And get rid of the addExpression entirely. (countQuery() adds the COUNT(*) for you.)

te-brian’s picture

@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.

mcarbone’s picture

Assigned: Unassigned » mcarbone

I'm working on some tests for this.

mcarbone’s picture

Assigned: mcarbone » Unassigned
Status: Needs work » Needs review
FileSize
7.94 KB

Added tests for all term and vocabulary tokens. Fixed a bug in the [term:url] token along the way.

Dries’s picture

Shouldn't we use countQuery() instead of adding an expression ourselves?

Otherwise looks great to me. Good job.

mcarbone’s picture

FileSize
7.85 KB

Agreed. Re-rolled using countQuery(). (But one of them still has to explicitly add the count expression since it also uses DISTINCT.)

sun.core’s picture

Status: Needs review » Needs work

Looks almost ready!

+++ modules/taxonomy/taxonomy.test	12 Jan 2010 01:25:45 -0000
@@ -962,3 +963,112 @@ class TaxonomyNodeFilterTestCase extends
+  

Trailing white-space here.

+++ modules/taxonomy/taxonomy.test	12 Jan 2010 01:25:45 -0000
@@ -962,3 +963,112 @@ class TaxonomyNodeFilterTestCase extends
+    $source = '[vocabulary:vid]';         // ID of vocabulary.
+    $source .= '[vocabulary:name]';        // Name of vocabulary.
+    $source .= '[vocabulary:description]'; // Description of vocabulary.

...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.

+++ modules/taxonomy/taxonomy.test	12 Jan 2010 01:25:45 -0000
@@ -962,3 +963,112 @@ class TaxonomyNodeFilterTestCase extends
+    // Check that the results of token_generate are sanitized properly. This does NOT
+    // test the cleanliness of every token -- just that the $sanitize flag is being
+    // passed properly through the call stack and being handled correctly by a 'known'
+    // token, [term:name].

Exceeds 80 chars.

Powered by Dreditor.

mcarbone’s picture

Assigned: Unassigned » mcarbone
Status: Needs work » Needs review
FileSize
7.42 KB

Re-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.

Dries’s picture

Status: Needs review » Fixed

This looks ready now. Committed to CVS HEAD. Thanks.

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

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