Optional vid argument for taxonomy_get_term_by_name()

This is right, and should be an easy enough fix. Moving to Drupal 7 since bugs get fixed there first, then backported.

I use a modified version of taxonomy_get_term_by_name() to select by vocabulary ids to limit my number of returns- this maybe helpful for you. I also think it might be a nice thing for other modules to have access to.

Note for sub D7, the table name has to change from taxonomy_term_data to term_data

<?php
function taxonomy_get_term_by_name($name, $vocabularies = null) {
  // multiple vocabularies to select from?
  if (is_array($vocabularies)) {
    $vocabs = ' AND t.vid IN (' . implode(', ', $vocabularies) . ') ';
  }
  // single vocabulary to select from?
  if (is_int($vocabularies)) {
    $vocabs = " AND t.vid = $vocabularies";
  }
  $db_result = db_query(db_rewrite_sql("SELECT t.tid, t.* FROM {taxonomy_term_data} t WHERE LOWER(t.name) = LOWER('%s')" . $vocabs, 't', 'tid'), trim($name));
  $result = array();
  while ($term = db_fetch_object($db_result)) {
    $result[] = $term;
  }
  return $result;
}

?>

tagging for the novice queue.

here's a slightly modified version of arthurf's fix from #3. I modified it slightly so that $vocabularies ($vid in the patch) is passed to the db driver as (an) argument(s).

err... noticed something's wrong with the patch, re-rolling.

that's better

This ought to be handled when #343788: Taxonomy module doesn't use it's own APIs gets in.

You'd be able to do

taxonomy_term_load_multiple(array(), array('name' => 'foo', 'vid' => $vid));

It'd then be easy to add the extra argument to taxonomy_get_term_by_name().

The only thing missing at the moment is the case insensitive condition on name.

Should this be marked duplicate?

Yours does multiple vids, which isn't and won't be covered by the other patch, so probably worth keeping open for that. Also the other patch is more general, and isn't committed yet, whereas this is very specific. Could use some reviews over there though :)

I did a coding standards review (http://drupal.org/coding-standards): looks OK there,

and I downloaded the D7 head from cvs and when I applied the patch I got:
ctheys% patch -p0 < ../336697-tax-name-vid.patch
patching file modules/taxonomy/taxonomy.module
Hunk #1 succeeded at 888 (offset 1 line).
Hunk #2 succeeded at 1209 (offset 1 line).
patching file modules/taxonomy/taxonomy.test

Which is probably OK.

It installed OK then... and my eyes kept closing, so I'm going to stop there and sleep. :)

OK. I'm not sure what I should try in order to "exercise" the patch.

I thought I would try something, so I made two vocabularies, and put the same terms in each. Then I made new content and selected the term for each vocabulary, and I tried putting only one term on content, and some more variations and combinations. And it seems to be working as I expect (listing the right content under the right term in the right vocab).

I'd appreciate leads on what exactly to try testing for this one.

I've wanted this feature since Drupal 4.7. Let's hope it can get into 7.

Making the title more specific.

Patch looks nice. Does what it needs to do and we have a test for it.

Sorry, too late for Drupal 7 now. :\ Unless it fixes a critical bug of some kind, no more API changes.

in this form this function should be removed, as it is not used at all in core (simply because it totally useless)..
Searching for a term in all vocabularies? I do not know any use case for that..

Some contributed module developer might already be depending on this. You never know.

then it must be a bug in that module

Reroll for Drupal 8, and improve tests to add multiple terms with the same name, then check the $vid filter is working.

> 171 passes, 0 fails, 0 exceptions, and 51 debug messages

Patch looks good to me, and I agree that being able to restrict on vid makes sense.

Some minor style issues with the current patch:

1. + * @param vid
   ...
   + *   Vocabulary ID of the vocabulary to limit the search by, defaults to all.
   

   vid should be marked as "(Optional)" in the comment (see http://drupal.org/node/1354#general)

2. +    ¶
   

   Trailing whitespace.

3. +    $this->assertEqual(count($terms), 1, t('One term loaded when restricted by vocabulary'));
   

   Assertions should end with a period (2 occurrences).

4. +++ b/modules/taxonomy/taxonomy.test
   @@ -755,6 +755,24 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
   +    $new_term->description = $this->randomName();
   +    $new_term->format = $term->format;
   

   These two properties are optional, I'd consider dropping them since they play no role in this test.

5. I also think we should add one more test case: try to taxonomy_get_term_by_name('foo', vocabulary xyz) where 'foo' is a term in vocabulary abc but not in xyz, and assert that no terms are returned.

Powered by Dreditor.

1, 2, 3 & 4, not 5.

File rename for testbot?

Invalid unix-style line endings, trying again.

#26: taxonomy-get-name-vid-336697.patch queued for re-testing.

Ok, fail again xD. Tomorrow a good patch & 5.

1, 2, 3 & 4

And 5

- consistent capitalisation of Vocabulary ID in the first two PHP doc comments
- wrap PHP doc and comments at 80 characters
- typo "diferent"
- typo/english in last comment:
that not exists ==> that doesn't exist
but existst in ==> but exists in

New patch. Thanks!

Sorry. One more 80 characters comment fix

Arguably "Load single terms when restricted to one vocabulary." should say "single term"

And "exists in other vocabulary" should either say "exists in another vocabulary" or "exists in the other vocabulary".

Thanks!

Yay.

Looks good, thanks for all your work on this oriol_e9g!

One more issue with the current patch:

+    $this->assertFalse($terms);

Should use a descriptive message about the assertion rather than the default "Value @value is FALSE.".

Some comments added and punctuation in affirmative sentences.

MMmmmm...

Better wording and duplicated assert removed (patch line 118).

Please restrict this patch to just that required to implement+test "Optional vid argument for taxonomy_get_term_by_name()". This makes it easier to review, and more obvious what we're doing.

Raise another issues to do all the cleanups in the tests.

Ok. I will create a new issue to cleanup the tests.

Tests cleanups split to: http://drupal.org/node/1195254

Looks good to me. 56 messages on a three line change? Hahaha :)

Sorry folks, this should be machine name, not $vid, for portability between servers/sites.

Rather than join on the taxonomy_vocabulary() table (which is unlikely to be well indexed although it'd be worth an EXPLAIN), I'd use taxonomy_vocabulary_get_names() or similar, look up the correct vid from there, then do the same query - but the argument should be machine name.

Go again: optional filter by vocabulary machine name.

Ops! spaces mistake.

Looks good, it would be nice for this to fail a bit harder:

+    // Filtering by a non-existing vocabulary
+    else {
+      return NULL;
+    }

I would consider + if (isset($vocabularies[$vocabulary])) { - remove this and just throw the notice if the vocabulary doesn't exist - then it'll be logged to watchdog etc.

For some strange reason the link above doesn't work: http://drupal.org/files/issues/taxonomy-get-name-336697-62.patch

Yeah, looks good to me.

This is only an API addition, so could potentially be backported.

#62 includes a comment cleanup not relevant to the patch, and the wording is a little unclear. I'll reroll.

Updated issue summary.

Attached is like #62 but without the extraneous comment change.

Clearer.

Added an issue summary. Still RTBC.

Updated issue summary.

@xjm with your patch we lose the tests, #62 is better because include tests.

Ah, that is a mistake.

#67 plus the test from #62. Please review.

Updated summary to review patch.

Looks good

If we need a patch for D7 backporting I can roll one.

#71: term-by-name-336697-71.patch queued for re-testing.

#71: term-by-name-336697-71.patch queued for re-testing.

+++ b/modules/taxonomy/taxonomy.module
@@ -1028,12 +1028,19 @@ function taxonomy_get_tree($vid, $parent = 0, $max_depth = NULL, $load_entities
+ * @param vocabulary
+ *   (optional) Vocabulary machine name to limit the search.
...
+function taxonomy_get_term_by_name($name, $vocabulary = NULL) {
...
+    $vocabularies = taxonomy_vocabulary_get_names();

It's a bit odd to pass a vocabulary name here. The function name suggests to pass a name of a term, but it's very unusual to continue that for additional, optional arguments.

IMO, the second argument should be $vid.

Second, minor, while being there, let's also add the missing "$" on the @param lines in the phpDoc.

+++ b/modules/taxonomy/taxonomy.module
@@ -1028,12 +1028,19 @@ function taxonomy_get_tree($vid, $parent = 0, $max_depth = NULL, $load_entities
+  if ($vocabulary) {

Since the value can be NULL, we need to test with isset().

20 days to next Drupal core point release.

#77: I believe the use of machine name was recommended by catch in #57:

Sorry folks, this should be machine name, not $vid, for portability between servers/sites.

Rather than join on the taxonomy_vocabulary() table (which is unlikely to be well indexed although it'd be worth an EXPLAIN), I'd use taxonomy_vocabulary_get_names() or similar, look up the correct vid from there, then do the same query - but the argument should be machine name.

I'll reroll with the other fixes for now.

I added the $ before name as well because it drove me crazy for one to be wrong and not the other. Sorry kittens.

I think that we are walking in circles xD

The firsts patches works with $vid argument #54, then introduce the vocabulary machine name for portability between servers/sites in #60, If it's preferred we can return to #54 versions.

Another option is accept both arguments:

taxonomy_get_term_by_name($name, array('machine_name' => $machine_name, 'vid' => $vid) )...

I'd still go for machine name. Vocabularies as entities wasn't a great idea in retrospect and this discrepancy is part of that. But machine_name is more consistent with node type etc.

ok, good point - I almost forgot that we have machine names for vocabs now.

However, any reason for attempting to load vocabularies? Why don't we simply pass the 'machine_name' => $vocabulary in $conditions?

That's because machine_name isn't in the taxonomy_term base table.

So is #79 RTBC then?

Yep.

This new function is unlikely to be called frequently in any particular page load. Hence, I believe we should do some minimal checking before assignment.

As currently written, there is an implicit assumption that whatever value that was passed as $vocabulary will exist as a key within $vocabularies. An isset($vocabularies[$vocabulary]) check will avoid throwing a PHP error and also prevent an invalid condition from being set. An else statement could added to the new isset condition as well to handle $vid values. I imagine the resulting code would look something like:

  if (isset($vocabulary)) {
    $vocabularies = taxonomy_vocabulary_get_names();
    if (isset($vocabularies[$vocabulary]) {
      $conditions['vid'] = $vocabularies[$vocabulary]->vid;
    }
    elseif (is_numeric($vocabulary)) {
      $vid = (int) $vocabulary;
      foreach ($vocabularies as $voc) {
        if ($voc->vid == $vid) {
          $conditions['vid'] = $voc->vid;
        }
      }
    }
  }

From the documentation, it appears that one can only limit the term to that it one vocabulary. I have a use case for getting a term from vocA, vocB or vocC. Does it make sense to expand the definition of $vocabulary to allow for array() of vocabularies to look in?

  if (isset($vocabulary)) {
    $vocabularies = taxonomy_vocabulary_get_names();
    if (isset($vocabularies[$vocabulary]) {
      $conditions['vid'] = $vocabularies[$vocabulary]->vid;
    }

This is a good point, but we could simplify this to:

    $vocabularies = taxonomy_vocabulary_get_names();
    if (isset($vocabularies[$vocabulary]) {
      $conditions['vid'] = $vocabularies[$vocabulary]->vid;
    }

since taxonomy_vocabulary_get_names() is an API function and always returns an array. A null vocabulary name will not be set and so the check is skipped.

I'm not sure if the magic int-or-string bit is a good idea. Callers can do this lookup if needed.

From the documentation, it appears that one can only limit the term to that it one vocabulary. I have a use case for getting a term from vocA, vocB or vocC. Does it make sense to expand the definition of $vocabulary to allow for array() of vocabularies to look in?

There's another issue related to this somewhere.

Thanks for your thoughts @xjm.

I suggest using two distinct isset() checks so that there is no data base hit in the event that $vocabulary is not even set. I think this is more performant since the added time form an additional isset() call is minor relative to even having to access the data base.

In checking taxonomy_vocabulary_get_names() just now, I see that there is no static storage of the $vocabularies array. Hence,
looping through vocA or vocB or vocC condition currently will result in three calls to get the array values from the data base. That argues that either taxonomy_vocabulary_get_names() be modified for static storage, or this function should handle the multiple vocabularies constraint.

Indeed, we also need specify the expected behavior if you pass a $vocabulary name but that vocabulary does not exist.

In that case, it would be utterly wrong to silently skip/ignore the $vocabulary limitation. Instead, I think I'd expect an empty result set (and obviously it would be superfluous to even attempt to load terms for a non-existing vocabulary).

In that case, it would be utterly wrong to silently skip/ignore the $vocabulary limitation. Instead, I think I'd expect an empty result set (and obviously it would be superfluous to even attempt to load terms for a non-existing vocabulary).

Ah, yes. If someone passes a term name and it is not found in the vocabulary, it returns an empty array--so obviously we need to do the same thing for non-existent vocabularies as well.

Regarding #89, sounds like maybe we should add static caching to taxonomy_vocabulary_get_names(). That could be opened as a separate novice issue.

@sun What are your thoughts about the elseif clause suggested in #87?

I think it makes sense to overload the $vocabulary variable as 'mixed', but am open to following other patterns implemented in core.

As per @xjm's suggestion in #91, I created the related issue #1274674: Add static caching to taxonomy_vocabulary_get_names().

Another issue came up from some local testing. This thought concerns how does one deal with capitalization and/or whitespace issues in the passed $vocabulary variable.

I am presuming that the vocabulary machine_name array is expected to be lower case. Hence, if one wants to constrain the search to say the 'tags' vocabulary, one must define the constraint to be exactly 'tags'. Based upon @sun's comment in #90 above, one should expect the following vocabulary names to have no match: 'TAGS', 'Tags', 'Tags ', ' tags', 'tAgs ', etc. Is this assumption really reasonable?

Shouldn't we both trim whitespaces and convert $vocabulary to lower case before doing any comparisons?

Let me finally relay a question by a work associate. She observed that in D7 a $vocabulary became an entity.

Hence, why then is it not possible to call this function in the form

taxonomy_get_term_by_name($name, array(entity $vocabulary, string $voc, int $vid));

Posted by sun on September 9, 2011 at 7:22pm
Indeed, we also need specify the expected behavior if you pass a $vocabulary name but that vocabulary does not exist.

In that case, it would be utterly wrong to silently skip/ignore the $vocabulary limitation. Instead, I think I'd expect an empty result set (and obviously it would be superfluous to even attempt to load terms for a non-existing vocabulary).

In #60 we return a NULL value if vocabulary doesn't exists but this was removed for a comment in #62.

function taxonomy_get_term_by_name($name, $vocabulary = NULL) {
  $conditions = array('name' => trim($name));
  if (isset($vocabulary)) {
    $vocabularies = taxonomy_vocabulary_get_names();
    if (isset($vocabularies[$vocabulary])) {
      $conditions['vid'] = $vocabularies[$vocabulary]->vid;
    }
    // Filtering by a non-existing vocabulary
    else {
      return NULL;
    }
  }
  return taxonomy_term_load_multiple(array(), $conditions);
}

I can work with this but I need some guidance... where are we going? It would be helpful a version of #79 patch with #90 solved?

I will try with this.

Same approach as #79 plus:

1. When filtering by a non-existing vocabulary return and empty array() and avoid data hit for loading terms. This is consistent because if you filter by a non-existing term you receive and empty array, not an error, and you can expect the same for a non-existing vocabulary filtering.

2. I added a new test to ensure that we return an empty array when filter by a non-existing vocabulary.

#98 looks good to me.

#98: term-by-name-336697-98.patch queued for re-testing.

Rerolled with a couple minor comment cleanups.

Sorry, one other doxygen fix.

Updated tasks.

Updated summary.

And, killing fewer kittens, since that first line of the first hunk would undoubtedly collide with the docs cleanup sprint.

Updated issue summary.

Looks good to me, except there's a todo on taxonomy_term_load_multiple() to remove $conditions, which would break this change.

Call me pedantic, but the doc comment that says:

+ * @param $vocabulary
+ *   (optional) Vocabulary machine name to limit the search. Defaults to
+ *   NULL.

The "Defaults to NULL" is irrelevant - the comment should describe what the default *behaviour* is, not the value that happens to represent that. In fact, the rest of the comment "(optional) Vocabulary machine name to limit the search." describes that sufficiently by the use of the word "optional"

The "Defaults to NULL" is irrelevant - the comment should describe what the default *behaviour* is, not the value that happens to represent that. In fact, the rest of the comment "(optional) Vocabulary machine name to limit the search." describes that sufficiently by the use of the word "optional"

Explicitly listing the default value for optional parameters is part of our doxygen standards. On http://drupal.org/node/1354, use your browser to search for the text "Defaults".

Looks good to me, except there's a todo on taxonomy_term_load_multiple() to remove $conditions, which would break this change.

Hmm, is there an issue for this? Can someone clarify what the status or intent of that is? Edit: the commit on the @todo references #920600: Document the deprecated $conditions parameter in entity_load().

The $conditions parameter in entity_load() and its callees is deprecated in favor of first calling EntityFieldQuery::execute() to retrieve the list of entity IDs matching a given set of conditions.

So what we need to know is if switching taxonomy_term_load_multiple() to EFQ will alter its signature, I guess? Can someone explain what we'd need to change to make this forward-compatible?

Updated issue summary.

Updated issue summary.

Updated issue summary.

Added note to summary about the @todo.

Explicitly listing the default value for optional parameters is part of our doxygen standards. On http://drupal.org/node/1354, use your browser to search for the text "Defaults".

I disagree with your interpretation of that page. The value "NULL" has no meaning in this case, and represents the *lack of* an explicit machine id to be used for restricting the result set. In fact the page you refer to includes:

* @param $third
 *   (optional) TRUE if Third should be done. Defaults to FALSE.
 *   Only optional parameters are explicitly stated as such. The description
 *   should clarify the default value if omitted.

I would suggest that this falls into "The description should clarify the default value if omitted"

Hrm, I don't quite understand #110. From my perspective the comment in the patch uses exactly the pattern in the example snippet, and it does exactly "The description should clarify the default value if omitted". That is, it documents the default supplied in the function signature:

function taxonomy_get_term_by_name($name, $vocabulary = NULL)

Perhaps the doxygen standard should be clearer in indicating that the documentation for optional parameters should end in the sentence, "Defaults to foo," where foo is the default value supplied in the signature.

Is this better in your opinion?

(optional) Vocabulary machine name to limit the search, if any. Defaults to NULL.

Hi xjm, and thanks for listening to my concern. My objection is that "NULL" has no semantic meaning - it's simply a "magic value" used to denote the non-existence of this optional parameter. I would suggest that the only comment required is:

(optional) Vocabulary machine name to limit the search

This seems to be meeting the objective "The description should clarify the default value if omitted" by clarifying via "optional" rather than explicitly including the value.

i.e. if you don't pass the machine name, you don't care what the default value was, it might as well be "DONOTUSE YES YES FROB" :)

But, this is quibble.

This pattern is already used in core; see, for example, field_has_translation_handler().

Rerolled for core/.

Cross-referencing #143817: API: Get just term ID by name.

Marked #1161248: Add vid argument to taxonomy_get_term_by_name as a duplicate of this issue.

#114: taxonomy_vid_arg-336697-POST-APOCALYPSE.patch queued for re-testing.

It's an api addition, we have an issue summary, we have discused, we have tests, testbot it's happy, so... anybody thinks that this is RTBC?

Looks good to me. All I have are a few comment style issues:

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1063,12 +1063,27 @@ function taxonomy_get_tree($vid, $parent = 0, $max_depth = NULL, $load_entities
+ *   (optional) Vocabulary machine name to limit the search. Defaults to
+ *   NULL.

This should fit on one line, I think.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1063,12 +1063,27 @@ function taxonomy_get_tree($vid, $parent = 0, $max_depth = NULL, $load_entities
+      // Return an empty array when filtering by a non-existing
+      // vocabulary.

Same here, probably.

+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -825,6 +825,34 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
+    // Try to load a term by name that doesn't exist in this vocabulary
+    // but exists in another vocabulary.

One more word fits on the first line.

Docs fixes from #119

Alright, I think this is RTBC (and long since). :)

Whitespace error here:

+    if(isset($vocabularies[$vocabulary])){

But I fixed that on commit. Committed/pushed to 8.x, not sure webchick will want to backport this, but that's up to her :)

Ok, API addition backported.

Consider this API addition for D7.

Summary for webchick

• If you have multiple vocabularies with the same term taxonomy_get_term_by_name() is not useful because you cannot filter by vocabulary.
• If some contrib uses this can make the function unstable, as it may select a term from an unexpected vocabulary.
• We use machine-name instead of vid for portability between sites/servers.
• No existing code will be affected, backwards-compatible addition only.

+++ b/modules/taxonomy/taxonomy.moduleundefined
@@ -1070,12 +1070,25 @@ function taxonomy_get_tree($vid, $parent = 0, $max_depth = NULL, $load_entities
+    if(isset($vocabularies[$vocabulary])) {

Let's fix the first part here too (space after if).

#126: taxonomy-vocabulary-336697-126.patch queued for re-testing.

That failing test case passes on my local machine, so one more try before more investigation follows.

Edit: Yay, random test failure no longer!

Mysterious temporal error that works locally many times and works in a second testbot attempt.

Technically you shouldn't RTBC your own backport, but it looks fine to me now. :)

This is a feature request, so I'll have to have a look at it after we're below thresholds.

We are finally below thresholds, so features are back on the table. Re-testing the latest patch, since it's been awhile.

#126: taxonomy-vocabulary-336697-126.patch queued for re-testing.

Whew! Thanks for all the hard work on this folks. :) Nice to get this helper in!

Committed and pushed to 7.x. Thanks! Marking as something to mention in the 7.13 release notes.