Add multiple hook for formatters

I'm wondering if this is just making field_attach_sanitize() multiple ?

Other annoying thing is that, really, only the formatter knows what it will need to do its job. Doing a full node_load() at pre-render is pointless if you only end up displaying 'node title as link'.

So maybe what we need in fact is a new *formatter* hook instead ? (a 'multiple' one)

Just a quick note: I like the idea, sounds great and +1 to keep loading referenced entities to the formatter, as only the formatter knows what kind of data is needed for the display. So it would be the formatter that implements the new hook. I don't like the name of the hook, though - when I read the issue's title I immediately though of drupal_render()'s #pre_render property. This is confusing. I would propose hook_field_build() or hook_field_build_multiple(). This keeps it in line with the regular node process.

Thanks for tackling this, catch. A couple remarks:

- field_attach_formatter_load() needs a $build_mode param, so we know which formatter to use.

- _field_invoke_multiple('formatter_load') will call the field-type module's hook_field_formatter_load(). Not good if the formatter is implemented by a 3rd party module.
This needs to be _field_invoke_multiple_default('formatter_load'), backed by a field_default_formatter_load() function, that:
finds the right formatter used for that build mode (I think _field_get_formatter() does that - see the 1st lines in field_default_view()),
invokes the right hook in the formatter module.
[edit: Note that _field_invoke_default('form_errors') in field_attach_form_validate() works exactly like that - except the intent is to call a widget hook instead of a formatter hook here]

- detail, but I'm not sure of the _load in the name. This happens way past the object 'load' time, so it might be confusing. Well, we can fine-tune later on if needed.

"what do we put for users and terms at the moment though"
I'd say like field_attach_view(), have the $build_mode arg default to 'full'.

Thanks catch.
Fixed a few glitches in field_default_formatter_load() ($instance only contains the name of the formatter type, we need to load the formatter-type info + backup to default formatter for the field type if the original formatter happens to be unavailabe).

The logic implied by 'multiple' is really hairy in there :-(. Attaching what I have so far, will revisit shortly.

Also, we probably need to implement an actual use case to fine tune the list of parameters we need in hook_field_formatter_load().

Additionally: I'm not too happy that we add a new field_attach_[op]() to the list of functions that entites need to call in order to be 'fieldable'.
So far those functions are bound to common workflow steps that make general sense in a reasonable workflow for any entity, fieldable or not: f_a_[op] where [op] is load, submit, update, view... This new f_a_formatter_load(), OTOH, doesn't really make sense if you don't know the guts of Field API.

Strictly speaking, this step should be hardcoded within field_attach_view(). But: we want a 'multiple' op, and field_attach_view() is single :-(
Not sure what's the cost of making field_attach_view() multiple...
It would make field_default_view() a little more complex, but that's internal. Not sure how it would impact node.module's code for frontpage listing, for instance.

Or maybe it's just a question of finding the right name for that new op: f_a_prepare_view() or something...

Updated patch.

I figured out the logic in field_default_formatter_load() a little bit better, by comparing with hook_widget() and hook_field_load().

In fact, we need to group objects, instances, items par formatter-module. Then we call hook_field_formatter_load() on each module, with the relevant objects, instances and items.
hook_field_formatter_load() works by altering the $items, just like hook_field_load().

Now we need to find the best way to test this :-), and validate by actual implementations in followup patches.
We also need to solve #13 (at least settle on a name)

On testing - well, strictly speaking, all other field_attach_*() are explicitly tested in field.test (against dummy test entities and field types defined in field_test.module), so this one should be as well.
I'll try to look into the exceptions later today.

Attached patch fixes the test exceptions and renames to _prepare_view().
CNR for the bot, but still needs tests.

"Failed to run tests" - what does this mean ?

With test for hook_field_formatter_prepare_view().
This should be ready now.

Should fix the failure.

If we both agree on the code, let's be bold and set this to RTBC.

Doh!

Straight reroll. The failed hunks were only whitespace fixes in user.module, automatically added by my IDE.
I removed them.

[edit: actually they were removed today in http://drupal.org/cvs?commit=240510, hence the fail here]

Rerolled.

- removed unneeded code to figure out the actual formatter used for the build mode in field_default_prepare_view(), now that #535034: Clean-up how fields and instances are prepared for runtime. fills that in cached field_info_instances() for us.
- added field_attach_prepare_view() call to comment_build_multiple() (like was previously done for node_build_multiple).

Which outlines an issue: direct calls to [comment|node]_build($single_object) do not trigger field_attach_prepare_view().
[comment|node]_build_multiple() should probably set an $object->_prepared = TRUE flag after calling field_attach_prepare_view($objects). Then, [comment|node]_build() calls field_attach_prepare_view($single_object) if empty($object->_prepared).

No time for this tonight, but feedback welcome.

Leaving to CNR for the bot.

Rerolled. Back to RTBC where it was in #31.

Patch :-p

Rerolled, + should fix tests.

Oh, testing bot...

.

This all looks proper to me, and yched/frando have already positively reviewed.

I have a concern about field_default_prepare_view():

* It builds $modules into an array of formatter modules, one module per $instance, and then groups objects, instances, and items by module. Then, it calls $module_field_formatter_prepare_view() for every module in $module.

* If $instances contain multiple instances using formatters from the same module, won't $module_field_formatter_prepare_view() be called multiple times instead of a single time?

* I think this can be fixed with "$modules[$module] = $module".

field_attach_prepare_view() is documented to add additional data items to field data items. In the test case, nothing *explicitly* tests that 'additional_formatter_value' gets added to the field's data; it only implicitly tests it by checking the themed output. I'd prefer an explicit test, though this isn't a biggie.

If I'm wrong about the $modules bug, or once it is fixed, I'll RTBC this.

Hm. I can see the utility of this hook. I'm a little worried about the name which associates it with a 'view' operation, rather than a 'load' operation, and load comes up at times when view is not involved, which will probably be a DX WTF. However, since this has the thumbs-up from all of the Field API maintainers, and since I can't think of anything better, I can't really justify holding up commit over this.

Catch gave me a documentation snippet on IRC to add to hook_field_load() to help other modules not get into this infinite recursion situation. Added that and committed to HEAD!

Sorry for not chiming back earlier.
In #33 I outlined :
"direct calls to [comment|node]_build($single_object) do not trigger field_attach_prepare_view().
[comment|node]_build_multiple() should probably set an $object->_prepared = TRUE flag after calling field_attach_prepare_view($objects). Then, [comment|node]_build() calls field_attach_prepare_view($single_object) if empty($object->_prepared)."

(which also means I should probably not have marked RTBC 10 days later - er... :-/)

Not easy for me to check wheter the committed patch adressed that concern, but turning back to active

Marked #632188: Notices on comment preview when node has terms as duplicate of "direct calls to [comment|node]_build($single_object) do not trigger field_attach_prepare_view()"

this issue is deprecated by recent formatter as array patch?

No, not deprecated. the formatters as array patch builds on this hook. This issue was kept open to address "direct calls to [comment|node]_build($single_object) do not trigger field_attach_prepare_view()." See #58.

However, that's best as a separate followup thread. I opened #661494: direct calls to node_view() do not trigger f_a_prepare_view().
Setting this one to 'fixed'