Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.There's a lot of functions in taxonomy module which return an array of term objects - but which are loaded direct from the database instead of via taxonomy_term_load(). Here's a run on taxonomy_get_term_by_name() to start clearing that up now that the multiple load patch is in.
I had to make a couple of changes to taxonomy_term_load_multiple(), because get_term_by_name is case insensitive. So $conditions['name'] does a strcasecmp() in PHP, and a LIKE() in SQL (this is case insensitive in MySQL and ought to be mapped to ILIKE in postgresql by dbtng - need to check that). Changed from LOWER() to LIKE() since LOWER()'s not indexed or query cacheable at all, afaik LIKE is a bit better on that front. I can't think of a situation where you wouldn't want case insensitive matching, so seems OK to treat it in this way.
Comes with tests.
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | taxonomy.patch | 7.16 KB | catch |
| #20 | multiple_load_conversion.patch | 6.97 KB | catch |
| #15 | multiple_load_conversion.patch | 7.04 KB | catch |
| #11 | multiple_load_conversion.patch | 6.57 KB | catch |
| #9 | multiple_load_conversion.patch | 6.86 KB | catch |
Comments
Comment #1
catchComment #2
catchRe-rolled to use drupal_strtolower() instead of strcasecmp() so things are multi-byte safe.
Comment #4
catchAdded taxonomy_term_select() too. These are the two places where we can convert fairly easily, some of the rest of taxonomy.module will need refactoring before it can be done there.
Comment #5
catchRe-rolled for taxonomy table name changes.
Comment #7
catchretesting.
Comment #9
catchbetter re-roll this time.
Comment #11
catchRe-titling, since this is a general cleanup of stuff we can simplify now multiple load is in, and re-rolled now the test has moved.
Comment #12
moshe weitzman commentedwhy "// When name is passed as a condition use LIKE.". seems like new behavior that slows down all name queries a little. just wondering.
Comment #13
catchThe name queries currently use LOWER('name') in taxonomy_get_term_by_name() which isn't indexable at all - chx and Crell have promised that they'll fix the mappings in the postgres driver so we can have case insensitive LIKE behaviour and avoid the LOWER() - this may have already been done but I'm not sure either way. So the current version of the patch is partially dependent on that.
It's true that taxonomy_term_load_multiple() allows for a case sensitive name query, but we've not had that in previous versions of Drupal, so it wouldn't be an API change as such - or at least only within D7 itself. I think it's more important to have proper loading (and a shared static to prevent duplicate queries) than it is to support a case sensitive search.
Comment #15
catchre-rolled.
Comment #17
catchCurrently failing due to #333054: Make all varchar columns case sensitive (regression) (was make page cache case sensitive)
Comment #18
catchThat patch has been reverted, so back to needs review.
Comment #20
catchre-rolled after dbtng conversion.
Comment #21
catchTrying a different title to see if I can get more eyes on the patch ;)
Comment #23
catchre-rolled.
Comment #24
andypostLooks great by why
taxonomy_get_term_data()was removed?So only taxonomy_term_load_multiple() will be used to get term object?
Comment #25
catchYeah when taxonomy_term_load() went in (with load hook, before multiple), there were several places in taxonomy module where potentially dozens of terms were getting loaded only to get their titles and similar. _multiple() now covers those situations more efficiently, and _get_term_data() doesn't fire hooks - so you never get a proper term object if you use that. Additionally, if you have _load() and get_term_data() on the same page it causes duplicate database queries. The function is basically the same as taxonomy_get_term() in D6 but it took a couple of patches to get to a point where we could kill it.
Comment #26
andypostSo suppose RTBC
I'm checking taxonomy_get_term_data() only used once and just eats memory with static
Comment #27
berdirWould it make sense to rename taxonomy_get_term_by_name?
We have http://api.drupal.org/api/function/user_load_by_name/7, so taxonomy_term_load_by_name() would be similiar...
Patch looks good, the broken notNull assertion is a good catch :)
Comment #28
stella commentedPatch looks good. Though I wonder if you need to explain the following comment further:
I'd be happy to set to RTBC assuming all tests pass.
Comment #29
andypostComment #30
dries commentedThis looks good to me, will commit later today. Some of these tests could probably become UnitTests instead of WebTests but we can leave that to a follow-up patch as it probably requires some additional refactoring of the tests.
Comment #31
dries commentedA few days late, but committed this to CVS HEAD. Thanks!