hook_field_extra_fields() is fired on any page rendering any fieldable entity. Since term bundles are vocabularies, this means taxonomy_vocabulary_get_names() is run on all those requests - even if the entity being registered doesn't care at all about the ordering of term names and descriptions on the term edit form :(

This would be fixed by making term names and descriptions fields, and not using taxonomy_field_extra_fields() at all, but that's a D8 issue (and term name may never end up a field). So the best we can do is add caching for it. So attached patch adds both a static and persistent cache to taxonomy_get_vocabulary_names(). Tests pass.

CommentFileSizeAuthor
taxonomy_vocabulary_get_names.patch1.86 KBcatch

Comments

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
Issue tags: +Performance
catch’s picture

catch
he/him
Primary language English
commented
Title: Unneccessary query in taxonomy_field_extra_fields() » taxonomy_vocabulary_get_names() called on every field_attach_view()
moshe weitzman’s picture

moshe weitzman commented

whats a path thats affected by this?

as you surely guessed, i'm pretty peeved about adding a new (rather obscure) persistent cache. we've got too many in d7. the static cache is no prob.

catch’s picture

catch
he/him
Primary language English
commented

Any path where a field gets rendered, so all node pages including listings, all user pages, any custom entity getting rendered anywhere.

i.e. any page calling either of these two functions:

field_attach_form()

field_attach_view()

The other option would be to not implement taxonomy_field_extra_fields().

moshe weitzman’s picture

moshe weitzman commented

I like that other option to implement taxonomy field sort in some way that isn't this hook. i would need to research in order to give more concrete suggestion.

catch’s picture

catch
he/him
Primary language English
commented

Hmm, well it looks like hook_field_extra_fields() is missing some parameters anyway:

http://api.drupal.org/api/function/hook_field_extra_fields/7

$bundle is undefined...

If we could pass in $entity_type and $bundle, same as field_extra_fields() gets, then taxonomy_field_extra_fields() would only have to query when the type is entity. Which would be taxonomy listings and forms, then at least we'd actually use the data we're querying.

That fix should go in regardless of the caching here. And if it does then I could probably live with just a static cache, although vocabularies are updated once in a blue moon, and entity cache can't swap out this particular query because it has to bypass the loader by nature.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue tags: -Performance

taxonomy_vocabulary_get_names.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance

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

yched’s picture

yched commented
Status: Needs work » Closed (fixed)

The issue doesn't exist anymore :
- taxonomy_field_extra_fields() now uses entity_get_info() (cached) instead of taxonomy_vocabulary_get_names()
- hook_field_extra_fields() is now cached along with _field_info_collate_fields(), and thus not invoked on every request.