As catch identified over in #826832: mollom_field_extra_fields() only works for nodes, needlessly loads $mollom_forms, the results of hook_field_extra_fields() implementations are not cached.

I think we can assume that many implementations will have to do some more expensive work to figure out what extra fields are available for which entities and which bundles. Since Field API caches almost everything else on its own, I naturally expect that the results of this hook are equally cached once for all.

CommentFileSizeAuthor
#14 drupal.field-info-extra-fields.14.patch7.76 KByched
#13 drupal.field-info-extra-fields.13.patch8.33 KBsun
#7 drupal.field-extra-fields-cache.7.patch7 KBsun
#5 field_extra_fields_cache-870292-5.patch5.95 KByched

Comments

yched’s picture

yched commented

true. subscribe for now.

yched’s picture

yched commented

Wonder if this should be merged into the data cached by _field_collate_fields(), or be dealt in a separate cache entry (that we then need to provide a separate way to refresh, etc...)

Anyway, sounds like code for field.info.inc.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Priority: Normal » Major
Issue tags: +API change

Given that already ported modules may expect to have to cache on their own, I think it's safe to bump this to major + tag as API change.

As for the cache entry, yes, merging the info into an existing field info cache makes sense to me.

yched’s picture

yched commented

Hm, that's how it was done in D6, BTW :-/.

Will try to look into this, but cannot guarantee an ETA.

yched’s picture

yched commented
Status: Active » Needs review
StatusFileSize
new field_extra_fields_cache-870292-5.patch5.95 KB

Here's a patch.

Strictly speaking, the field_extra_fields() function should now be moved to field.info.inc and the field_info_* namespace. That would be an API break, though minor since I don't think much contrib code has uses for getting this data. Left the name unchanged for now.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ modules/field/field.info.inc
@@ -225,6 +225,18 @@ function _field_info_collate_fields($reset = FALSE) {
+      $extra = (array) module_invoke_all('field_extra_fields');

The cast to array shouldn't be necessary - module_invoke_all() is always an array.

Aside from that, looks RTBC to me if bot passes. Nice patch. :)

+++ modules/field/field.info.inc
@@ -225,6 +225,18 @@ function _field_info_collate_fields($reset = FALSE) {
+      drupal_alter('field_extra_fields', $extra);

yay! Hookomania! :)

Powered by Dreditor.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new drupal.field-extra-fields-cache.7.patch7 KB

Just realized that the alter hook already exists. Made that minor array casting correction, RTBC. Thanks, yched!

yched’s picture

yched commented

Use case for alter_hook : autonodetitle will want to remove the 'Title' as an extra field in forms, in addition to hiding it in actual $form definitions.
This kind of stuff.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Oki-doki. Committed to CVS HEAD. Thanks.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

Does this break BC and need to be announced? Or does it just make the world one slight bit happier?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Nothing should break due to this patch, BC is preserved. We only cache the hook results once for all for performance.

yched’s picture

yched commented

Only change should be :
"If your module implements hook_field_extra_fields() and the return values can vary depending on external conditions (module settings, whatever), you need to call field_info_cache_clear() whenever the conditions changes"

As mentioned in #5, I'd support renaming field_extra_fields() to field_info_extra_fields() for API consistency. I don't think contrib has cases for calling this function currently. I left that out of the patch that got committed, though.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Fixed » Needs review
StatusFileSize
new drupal.field-info-extra-fields.13.patch8.33 KB

Probably true, the Field API info function itself would only be called by better_field_ui.module or similar.

So let's try this. Not sure whether more functions should be moved, and, whether the resulting position/location in field.info.inc is correct.

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new drupal.field-info-extra-fields.14.patch7.76 KB

Thks sun :-)

Same patch, just moved field_info_extra_fields() a little down field.info.inc.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

yched’s picture

yched commented
Status: Fixed » Active

Crap. It just occurred to me that this reintroduced an occurrence of #503550: Translated strings are cached in _info() hooks - hook_field_extra_fields() has t() strings, and we cache them in a language-agnostic entry.

Not that serious, because those t() strings (label and description of the 'extra fields') only appear on Field UI overview screens.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Problem space being that this is not like regular info hooks -- potx extractor cannot easily identify translatable strings (although I'm not sure how that works actually).

As usual, either t() in the hook and already translated strings in cached data, or t() at runtime with potx extracting the strings without t().

If it wasn't 1 year after official code freeze, I'd suggest to introduce a new t_record(), so we are finally able to cleanly resolve this translatable strings disaster and translate all strings at runtime.

sun.core’s picture

sun.core commented

We *really* need that t()-no-op function for potx. Given the amount of info and alter hooks we introduced for D7, it's close to impossible to make potx catch all of them (and even doing so would only catch Drupal core).

catch’s picture

catch
he/him
Primary language English
commented
Priority: Major » Normal

Downgrading to normal, this should be a quick fix though.

catch’s picture

catch
he/him
Primary language English
commented
Title: hook_field_extra_fields() results are not cached » hook_field_extra_fields() results should be cached by language
Version: 7.x-dev » 8.x-dev
Issue tags: -API change +Needs backport to D7
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue summary: View changes
Status: Active » Closed (duplicate)

Duplicate of #1810178: field_info_extra_fields() is not language-aware, may return wrong values I think, fixed in D8.