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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

true. subscribe for now.

yched’s picture

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

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

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

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

yched’s picture

Status: Active » Needs review
FileSize
5.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

+++ 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

Status: Needs review » Reviewed & tested by the community
FileSize
7 KB

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

yched’s picture

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

Status: Reviewed & tested by the community » Fixed

Oki-doki. Committed to CVS HEAD. Thanks.

rfay’s picture

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

sun’s picture

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

yched’s picture

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

Status: Fixed » Needs review
FileSize
8.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

Status: Needs review » Reviewed & tested by the community
FileSize
7.76 KB

Thks sun :-)

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

yched’s picture

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

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

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

Priority: Major » Normal

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

catch’s picture

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

Issue summary: View changes
Status: Active » Closed (duplicate)