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.
Comment | File | Size | Author |
---|---|---|---|
#14 | drupal.field-info-extra-fields.14.patch | 7.76 KB | yched |
#13 | drupal.field-info-extra-fields.13.patch | 8.33 KB | sun |
#7 | drupal.field-extra-fields-cache.7.patch | 7 KB | sun |
#5 | field_extra_fields_cache-870292-5.patch | 5.95 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedtrue. subscribe for now.
Comment #2
yched CreditAttribution: yched commentedWonder 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.
Comment #3
sunGiven 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.
Comment #4
yched CreditAttribution: yched commentedHm, that's how it was done in D6, BTW :-/.
Will try to look into this, but cannot guarantee an ETA.
Comment #5
yched CreditAttribution: yched commentedHere'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.
Comment #6
sunThe 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. :)
yay! Hookomania! :)
Powered by Dreditor.
Comment #7
sunJust realized that the alter hook already exists. Made that minor array casting correction, RTBC. Thanks, yched!
Comment #8
yched CreditAttribution: yched commentedUse 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.
Comment #9
Dries CreditAttribution: Dries commentedOki-doki. Committed to CVS HEAD. Thanks.
Comment #10
rfayDoes this break BC and need to be announced? Or does it just make the world one slight bit happier?
Comment #11
sunNothing should break due to this patch, BC is preserved. We only cache the hook results once for all for performance.
Comment #12
yched CreditAttribution: yched commentedOnly 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.
Comment #13
sunProbably 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.
Comment #14
yched CreditAttribution: yched commentedThks sun :-)
Same patch, just moved field_info_extra_fields() a little down field.info.inc.
Comment #15
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #16
yched CreditAttribution: yched commentedCrap. 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.
Comment #17
sunProblem 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.
Comment #18
sun.core CreditAttribution: sun.core commentedWe *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).
Comment #19
catchDowngrading to normal, this should be a quick fix though.
Comment #20
catchComment #21
BerdirDuplicate of #1810178: field_info_extra_fields() is not language-aware, may return wrong values I think, fixed in D8.