This issue arose from #336697: Optional vid argument for taxonomy_get_term_by_name().

As outlined in that issue, there may be a need for filtering the retrieval of $term (name) from $vocabularies array($A, $B, $C). The suggested solution is to invoke a loop repeatedly calling taxonomy_get_term_by_name() with the $vocabulary constraint set to $A or $B or $C. Within taxonomy_get_term_by_name() is a call to taxonomy_vocabulary_get_names(), which queries the data base for defined $vocabularies and the returns the resulting array.

To make taxonomy_vocabulary_get_names() more performant for a particular page request, the return value should be stored in a static array to minimize the data base query. An open issue is whether the function profile should be changed to include a parameter $reset = FALSE.

@xjm has suggested that this is a patch for 'novice' contributors.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

karschsp’s picture

Status: Active » Needs review
FileSize
628 bytes

Here's an initial stab at this.

Lars Toomre’s picture

Thanks @karschap for your initial stab. I note that you are attempting to change taxonomy_get_term_by_name(). That function is being worked on in #336697: Optional vid argument for taxonomy_get_term_by_name(). The purpose of this issue is to add a static to taxonomy_vocabulary_get_names() which will be called by that modified taxonomy_get_term_by_name().

On your patch attempt, there a couple of other things that would need to be changed. First, you need set $term before returning; otherwise the static storage has no effect. Second, Drupal coding standards call for formatting else statements like:

if (...) {
  ...
}
else {
  ...
}
Lars Toomre’s picture

Assigned: Unassigned »
Status: Needs review » Needs work

Forgot to change status.

Lars Toomre’s picture

Assigned: » Unassigned

Cleaning up spammer data.

karschsp’s picture

Status: Needs work » Needs review
FileSize
678 bytes

Got it (I think)...so, something like the attached?

Status: Needs review » Needs work

The last submitted patch, 1274674.003.patch, failed testing.

pgrond’s picture

FileSize
707 bytes

Lets use &drupal_static for this. Reworked patch attached.

pgrond’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1274674-5001044.patch, failed testing.

mdupont’s picture

Patch in #8 is a step in the right direction but still have some issues:
- indentation is incorrect (Drupal coding standards use 2 spaces)
- drupal_static() is called with a default value, so $names will always be set and the underlying conditional block of code will never be run

Here is a minimal patch that fixes these.

pgrond’s picture

Ah yes. Stupid. Thanks for the fix.

arithmetric’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, taxonomy_vocabulary_get_names_static_cache-1274674-11.patch, failed testing.

aroq’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

added static reset function call for "taxonomy_vocabulary_get_names" (in analogy to taxonomy_terms_static_reset() ) in taxonomy_vocabulary_save($vocabulary) and taxonomy_vocabulary_delete($vid) functions to have a cache reset after changing vocabularues.

aroq’s picture

BTW, while checking the code I wondered if reset logic should be placed in TaxonomyVocabularyController->resetCache() method?

mdupont’s picture

@aroq: the same logic is applied in taxonomy_terms_static_reset() so I guess your approach is the good one.

mdupont’s picture

Status: Needs review » Reviewed & tested by the community

Let's get gurus to have a look.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs backport to D7

taxonomy_vocabulary_get_names() is a hack because we're using the vocabulary (which is an entity) as a bundle, and you get infinite recursion in hook_entity_info() if you don't have the workaround function.

However while I'd like to see us change vocabularies to configuration in Drupal 8, while they're entities we need to encourage use of this function when you only need a list of all bundle names or mapping vid to name, so adding the static cache here makes sense.

Patch failures in here show it is already tested and the static reset is consistent with terms (which also have meta functions on top of loading).

So I've committed this to 8.x, moving back to 7.x for webchick to consider since there's the same issue there.

karschsp’s picture

Cleaning up whitespace and removing a stray ".". Passes locally.

Lars Toomre’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to go.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

karschsp’s picture

Version: 7.x-dev » 8.x-dev
Status: Fixed » Needs review
FileSize
456 bytes

Really minor. Removing an extra "." from a comment.

Devin Carlson’s picture

Status: Needs review » Reviewed & tested by the community

The above patch simply fixes punctuation.

The extra "." has already been removed from Drupal 7, so the patch should only be committed to Drupal 8.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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