Entity loading needs protection from infinite recursion

I probably agree with the approach (I'm not that familiar with RDF code), but this raises a DX question:

Originally the field_attach_*() namespace was supposed to be "the functions that fieldable entities are supposed to call in their workflow".
field_attach_load() is now already taken care of by the entity layer. #367006: [meta] Field attach API integration for entity forms is ungrokable automates all the 'form-related' f_a_*() calls (form, form_submit, form_validate) to the entity layer as well. entity modules need not and in fact should not call those functions.

This patch is a little different, it results in "call entity_prepare_view() where you called f_a_prepare_view() previously".

It feels like the "Field Attach API", from 'push' in D6 (CCK pushes its stuff in hook_nodeapi()), and 'pull' in early D7 Field API (entity modules call the right functions at the right time), is slowly moving to an intermediate state that smells like a DX WTF...

Holy cow. Nice finding.

subscribe

+++ includes/common.inc	30 Nov 2009 06:24:10 -0000
@@ -6395,6 +6395,18 @@ function entity_get_controller($entity_t
+ * Invoke hook_ENTITY_prepare_view() and field_attach_prepare_view().

Not true anymore.

+++ includes/common.inc	30 Nov 2009 06:24:10 -0000
@@ -6395,6 +6395,18 @@ function entity_get_controller($entity_t
+function entity_prepare_view($entities, $entity_type) {

Can we move $entity_type as the 1st param, to match Field API ? or is "$entities first" a pattern on the entity API side ?

+++ modules/system/system.api.php	30 Nov 2009 06:24:12 -0000
@@ -166,6 +166,29 @@ function hook_entity_load($entities, $ty
+ * This module allows you to operate on multiple entities as they are being

"This module" ?

+++ modules/system/system.api.php	30 Nov 2009 06:24:12 -0000
@@ -166,6 +166,29 @@ function hook_entity_load($entities, $ty
+ * prepared for view. It should be used to allow data to be loaded which should
+ * not be part of the object returned from entity_load() - usually when loading

Convoluted ;-)

More generally on the approach - er, I'm a bit confused right now ;-)

This review is powered by Dreditor.

"field_attach_prepare_view() isn't re-entrant" : is this the same issue than #493314-58: Add multiple hook for formatters ? ("direct calls to [comment|node]_build($single_object) do not trigger field_attach_prepare_view().")
Then maybe we need some '#view_prepared = TRUE' flags - a bit similar to FAPI's ' #validated. Not sure this flag can apply to an $entity as a whole, or needs to be field-by-field...

Stumbled on ""node_build() doesn't trigger field_attach_prepare_view()" once more in #652834: Field formatters as render arrays.

Two tricky things:

- we cannot always assume a field_attach_*() (processes all fields of an object) workflow. Fields can be displayed 'on their own' using a custom formatter (by the currently broken field_display_field() / field_format() functions or their updated counterparts in #612894: field_format() is just a pile of code that doesn't work)
- The build mode is important. Which build mode you're being built in determines the formatter to use and thus which hook_field_formatter_prepare_view() will be called.
The same node could be displayed in several build modes within the same page request (e.g 'full' node in main content, but with a couple fields deported in a 'sideblock' build mode in a sidebar).

I'd say a global $entity->#field_prepared flag might be enough if we find the proper place(s) to reset it after the job is done.

I'm still wondering whether making field_attach_view() itself 'multiple objects' (as apposed to f_a_prepare_view() being multiple-object because it does some db requests, and f_a_view() being single-object because originally it doesn't really need to be 'multiple' in itself) would not let both ops be merged into a single one (f_a_view() takes care of prepare_view), and solve the mess of 'sometimes one of them is not run'.
Would mainly affect field_default_view() (internal code). Only API change is hook_field_attach_view_alter() - not even necessarily, I think.

"make field_attach_view() itself multiple-objects": forget it, I guess. This still wouldn't solve the issue that node_build_multiple($nodes) calls node_build() / node_build_content(), but the last 2 can also be called directly on a single node.

catch: agreed.

+++ includes/common.inc	10 Dec 2009 01:42:24 -0000
@@ -6423,6 +6423,30 @@ function entity_get_controller($entity_t
+ * @param $entities
+ *   The entity objects which are being prepared for view.

Minor: we should specify that the array has to be keyed by object id.

+++ includes/common.inc	10 Dec 2009 01:42:24 -0000
@@ -6423,6 +6423,30 @@ function entity_get_controller($entity_t
+  module_invoke_all('entity_prepare_view', $prepare, $entity_type);

Shouldn't we call the hook only with entities where entity_view_prepared is not TRUE ?

+++ modules/comment/comment.module	10 Dec 2009 01:42:24 -0000
@@ -836,6 +836,7 @@ function comment_build_content($comment,
   field_attach_prepare_view('comment', array($comment->cid => $comment), $build_mode);
+  entity_prepare_view('comment', array($comment->cid => $comment));

(and friends)
Nitpick: entity API being lower level that field API, I'd suggest calling e_prepare_view() before f_a_prepare_view()

+++ modules/system/system.api.php	10 Dec 2009 01:42:26 -0000
@@ -212,6 +212,28 @@ function hook_admin_paths_alter(&$paths)
+function hook_entity_prepare_view($entities, $type) {

In an earlier comment I suggested having the $type param come first. Was done for entity_prepare_view(), but now hook_entity_prepare_view() is off.

Also, given the entity_view_prepared flag, shouldn't we also call entity_prepare_view() from 'single' node_build() ?

I'm on crack. Are you, too?

Agreed with catch's answers in #22.

"Only prepare items without the flag - it already does that see the $prepare array"
Doh, my bad.
Minor drawback is that hook_entity_prepare_view receives $entities where entity_view_prepared is already set to TRUE. No biggie I guess, since by contract it receives $entities that do need to be prepared, so it shouldn't need to actually check the flag. Setting the flag after the actual 'prepare' hook has run would mean an additional loop on the $entities array. Probably not worth.

Note that I didn't really read any of the follow-ups, just the patch.

+++ includes/common.inc	11 Dec 2009 03:03:47 -0000
@@ -6423,6 +6423,34 @@ function entity_get_controller($entity_t
 /**
+ * Invoke hook_entity_prepare_view().
+ *
...
+function entity_prepare_view($entity_type, $entities) {

+++ modules/system/system.api.php	11 Dec 2009 03:03:51 -0000
@@ -212,6 +212,28 @@ function hook_admin_paths_alter(&$paths)
 /**
+ * Act on entities as they are being prepared for view.
+ *
+ * Allows you to operate on multiple entities as they are being prepared for
+ * view. Only use this if attaching the data during the entity_load() phase
+ * is not appropriate, for example when attaching other 'entity' style objects.
...
+function hook_entity_prepare_view($entities, $type) {

The PHPDoc of entity_prepare_view() lacks some information, IMHO. At least, I don't really have a clue what it does, and why I would want to invoke it.

The same applies more or less to the corresponding hook, although I get a slightly better idea of it after reading that.

On a related note, I don't really understand why entity_prepare_view() is invoked *after* field_attach_prepare_view()... I would have assumed that fields are the cause for potential recursion, which may not be true, but then this really needs much more docs to be not confusing.

In general though, none of the PHPDoc mentions the critical cause for this function, why we implemented it, and how it is supposed to work - i.e. "API documentation". ;)

Lastly, can we test this?

I'm on crack. Are you, too?

+++ includes/common.inc	14 Dec 2009 04:05:22 -0000
@@ -6426,6 +6426,46 @@ function entity_get_controller($entity_t
+ * If adding a new entity similar to nodes, comments or users, you may
+ * optionally invoke this function during the ENTITY_build_content() or

"You may optionally" sounds weird. Shouldn't it be compulsory ? "If you're an entity, you need to call this". You cannot predict what contrib or 3rd party use cases will be, right ?

+++ includes/common.inc	14 Dec 2009 04:05:22 -0000
@@ -6426,6 +6426,46 @@ function entity_get_controller($entity_t
+ * the objects  during this phase. This is needed for situations where

double space

+++ includes/common.inc	14 Dec 2009 04:05:22 -0000
@@ -6426,6 +6426,46 @@ function entity_get_controller($entity_t
+ * recursion. By convention, entity_prepare_view() is called after
+ * field_attach_prepare_view() to allow entity level hooks to act on content
+ * loaded by field API.

Hm, for those cases we generally have hook_field_attach_[op]() hooks. f_a_prepare_view() fails to call such a hook currently - different issue though.

This review is powered by Dreditor.

Thanks !

We do need to fix this for field's own _prepare_view() op. I'll try to do this asap, but it won't be within this week.

+ opened #660514: f_a_prepare_view() has no associated hook

Posted a fix for the field API counterpart in #661494: direct calls to node_view() do not trigger f_a_prepare_view()

Sorry, this no longer applies.

Rerolled after #661494: direct calls to node_view() do not trigger f_a_prepare_view() and the 'build / view' renames.

Since this is one of the top 5 performance improvements, and it looks like a reasonable solution to the problem. I do get concerned about how many of these 'phases' we're adding everywhere, but also can't exactly think of anything better to do.

Committed to HEAD. But one thing I noticed too late is that:

 * invoke this function during the ENTITY_build_content() or
 * ENTITY_build_multiple() 

are wrong. They're both view something something. But since I couldn't remember what and couldn't find it easily, marking CNW for that, as well as documenting this API change.

We also should add tests for this. So adding a tag for that as well.

Great, thanks! :D

Committed that small folllow-up to HEAD.