Overhaul and extend node build modes [#409750]

The 'display' part of the field API is still fairly node-centric. It relies mainly on the notion of 'build mode', that was introduced on D6 nodes mainly to allow richer CCK features, while trying to be as little intrusive as possible with respect to the existing node API. The goal of this thread is to generalize the concept to non-nodes, and take the chance to enhance it with lessons learned meanwhile.

Filing this below 'field system' for now, but it will mainly affect fieldable entities, and essentially node API and rendering workflow, so it deserves a broader audience that 'just' the Field API team IMO.

Proposed approach (no particular order, but I'll add numbers to ease potential discussion) :

0) reminder : build modes are what field display settings and visibility are attached to. Fields have one set of display settings for each build mode. Not implemented yet : field order should be per build-mode too (UI challenge, but easy API-wise)

1) generalize build modes to non nodes. Any fieldable entity potentially needs to be listed in 'short versions', indexed and searched, displayed in feeds...

2) let modes be truly extensible from contrib by moving away from numeric constants (define('NODE_BUILD_SEARCH_INDEX', 2);) to (node-agnostic) string constants :
NODE_BUILD_SEARCH_INDEX --> 'search_index', NODE_BUILD_RSS --> 'rss' ...

3) it is up to each entity to decide which set of build modes make sense for them (can be extended from a 3rd party module anyway). Build modes are not tied to any specific entity type. All entities come with at least a 'full' mode, the ones that want to be listable come with 'teaser' and possibly 'rss' modes, the ones that want to be searchable come with 'search_index' and 'search_result', etc... An entity type can also declare build modes that only make sense inside that entity.

4) a registry of build modes for each entity type is available for any module to build on (in D6 CCK held that registry for its 'display fields' UI).

5) move away from $node->build_mode property, set before calling node_build(), to a $mode parameter replacing and extending the current $teaser boolean param in field_attach_view(), node_build(), node_build_content(), hook_view(), hook_node_view(), hook_link()...

6) consequently, 'full' and 'teaser' are their own modes, instead of the current aberration where 'teaser' is $node->build_mode == NODE_BUILD_NORMAL && $teaser == TRUE
For backwards compatibility, node templates can still be given a $teaser variable (= $mode == 'teaser').

7) reintroduce D5's $node->in_preview flag. Preview is *not* a build mode, since you currently can have previews of both full and teaser. It is a special case of node rendering, orthogonal to the build mode. For extra bonus points (possibly in contrib) : let admins decide which build modes are shown on preview.

8) Likewise, deprecate the 'teaser' and 'fulltext' settings for the feed_item_length variable used in node_feed() (they will make no sense when #372743: Body and teaser as fields lands anyway). Instead you can adjust fields visibility for the 'rss' build mode, and/or use custom build modes for Views feeds.

The ultimate goal IMO is to have 'modes' be just empty shells, with as little actual behavior hardcoded as possible (merely providing sensible defaults). 'teaser' is just the mode used for default entity listings, 'rss' is the one used by default feeds, etc. What actually happens in each mode is configurable, the main entry point being the fields display settings.

Work on this will build on #372743: Body and teaser as fields, since it basically strips $teaser of its hardcoded semantics - yay ! It won't be easy for me to post patches until that patch lands. Short of installing cygwin, I can only roll patches against CVS on my lame windows setup :-(.

Meanwhile, feedback welcome on the approach described above.

Comment	File	Size	Author
#28	build_modes-409750-28.patch	45.84 KB	yched
#28
#25	build_mode.patch	47.22 KB	moshe weitzman
#25
#21	build_modes-409750-21.patch	44.68 KB	yched
#21
#20	build_modes-409750-20.patch	44.64 KB	yched
#20
#15	build_modes-409750-14.patch	44.61 KB	yched
#15
#12	build_modes-409750-12.patch	44.17 KB	yched
#12
#10	build_modes-409750-10.patch	42.71 KB	yched
#10
#8	build_modes-409750-8.patch	42.42 KB	yched
#8

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

yched CreditAttribution: yched commented 21 March 2009 at 23:57

Side note : currently field module holds the build modes registry, through field_build_modes() and hook_field_build_modes(). Can easily be moved out, only I'm not sure where to.

At the moment, field is the only thing in core that's relevant to nodes, users, etc. Build modes are not strictly tied to the field API, but right now an object type needs build modes essentially because it is fieldable, so maybe field.module is the right place for this after all.

Comment #2

joachim CreditAttribution: joachim commented 22 March 2009 at 23:21

This touches on an idea I've been pondering for ages -- that the $node object should get information about where it's being built for -- to use an overused word, its context.
Eg: taxonomy_list, blog_roll, blog_roll_user, view, etc.
This would generalize the $teaser / $page / $isfront flags, and allow more refined theming options.
Would node context be orthogonal to this, or a subset of the regular node build more?

Comment #3

yched CreditAttribution: yched commented 23 March 2009 at 00:00

By default, build modes wouldn't be as granular as what you describe. Several contexts can use the same build mode.
For instance, the default front page listing and the default taxonomy listings would both use 'teaser' mode.
So in a way, it's orthogonal.

This being said, with views-defined taxonomy listings, frontpage listing, custom listing, rss feeds, etc...
+ the views patch in #349157: Use CCK-exposed build modes in node row styles
+ a UI for user-defined build modes (currently build modes are 'defined' by code only - fairly simple code),
you can customize any list or feed to use whatever build mode you want (and customize these build modes to use whatever fields combination and display settings you want). That's the whole idea.

Comment #4

mfer CreditAttribution: mfer commented 23 March 2009 at 13:37

/subscribe

Comment #5

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 12 April 2009 at 00:10

This is yummy! subscribing.

Is it possible at all to use the $page object now to provide this context? I would love Moshe's feedback on this issue.

Comment #6

sun

German

Karlsruhe

CreditAttribution: sun commented 27 April 2009 at 22:09

Coming from #445902: (bool) menu_get_object() is *not* an equivalent of $page - yes, that's the very same conclusion I suggested over there. The $page argument is gone, now $teaser should go as well.

I guess the caller decides which build mode is requested. node_view_page() will obviously use the corresponding build mode. Likewise, user_view(). I don't intend and don't want to derail with contexts, but there is indeed the question whether both node AND user should use the same internal build mode name. I mean, what's a 'teaser' with regard to users? Sure, both nodes and users can appear in search results (though I'm not sure whether that counts as build mode). But in general, I think that each module should define its own build modes that can apply to all kind of objects. Search would define 'search_index', and the not-yet-existing rss.module (globally enabling and providing RSS support) would define 'rss'.

Hm. Oh, wow! Rendering in build modes is also the first part of User Display API in core (called "user display styles" there). :)

Comment #7

yched CreditAttribution: yched commented 27 April 2009 at 23:16

sun #6:

- there is indeed currently a NODE_BUILD_SEARCH_RESULT mode, and CCK D6 exposes it on its 'Display fields' tab.

- "should both node AND user use the same internal build mode name ?" :
We at least need one 'full' mode common to all object types, as a 'default mode' Field API can internally rely on, an that can be used by modules programmatically creating their own $field definitions.
After that, I think the proposal in "3) it is up to each entity to decide which set of build modes make sense for them (can be extended from a 3rd party module anyway)" is open enough :
IMO it would be wise to leave 'teaser' mode open to more than node objects (that just means calling it 'teaser' and not 'node_teaser').
Your object type has list pages ? Simply expose the fact that it makes use of the 'teaser' mode.
Print.module D7 wants to provide printable versions of user pages ? It alters the exposed build modes and adds the 'print' mode to users.

- definitely sounds like User Display API's "display styles" :-)

Comment #8

yched CreditAttribution: yched commented 14 June 2009 at 23:05

Status:

Active

» Needs review

File	Size
build_modes-409750-8.patch	42.42 KB

1st patch, posting to see what the bot has to say.

Still TODO : point 8) in the OP + some PHPdocs + probably some test fixes.

Comment #9

14 June 2009 at 23:20

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #10

yched CreditAttribution: yched commented 16 June 2009 at 00:02

Status:

Needs work

» Needs review

File	Size
build_modes-409750-10.patch	42.71 KB

This should fix the fails and exceptions.
- Fixed a couple typos
- Forgot to update the node_build_multiple() call in node_show()

Comment #11

sun

German

Karlsruhe

CreditAttribution: sun commented 16 June 2009 at 00:40

Status:

Needs review

» Needs work

Mind to explain this hunk?

@@ -1130,6 +1100,7 @@
 /**
  * Builds a structured array representing the node's content.
  *
+ * TODO
  * The content built for the node will vary depending on the $node->build_mode
  * attribute.  The node module defines a set of common build mode constants:
  *   - NODE_BUILD_NORMAL: Node is being built to be viewed normally.

That said, there is also a better syntax for todo remarks in code:

 * @todo Foo bar baz; foo bar baz foo bar baz foo bar baz foo bar baz foo bar
 *   baz, foo bar baz foo bar baz foo bar baz.

...

-  if ($node->build_mode != NODE_BUILD_RSS) {
-    book_node_view_link($node, $teaser);
+  if ($build_mode == 'rss') {
+    book_node_view_link($node, $build_mode);

This condition looks negated now. Seems like there is no test for those links... ;) (separate issue)

@@ -501,7 +501,7 @@
...
     // Append the list of comments to $node->content for node detail pages.
-    if ($node->comment && (bool)menu_get_object() && $node->build_mode != NODE_BUILD_PREVIEW) {
+    if ($node->comment && (bool)menu_get_object() && empty($node->in_preview)) {

Does this mean that comments are unconditionally appended to all build modes, except when being in preview? Sorry, no context here without diff -p... :( Hm, ok, menu_get_object() will limit them additionally to node/123 paths. Cries for a separate issue.

@@ -1928,7 +1928,7 @@
       $variables['classes_array'][] = 'comment-new';
     }
   }
-  
+
 }

Good catch on trailing white-space :) However, that empty line should actually not be there at all.

Aside from that, RTBC.

Comment #12

yched CreditAttribution: yched commented 16 June 2009 at 18:34

File	Size
build_modes-409750-12.patch	44.17 KB

Updated per sun's review:

- The TODO part was a forget-me-not to rewrite the PHPdoc according to the changes

- Fixed the $node->build_mode != NODE_BUILD_RSS / $build_mode == 'rss' typo

- "Does this mean that comments are unconditionally appended to all build modes, except when being in preview".
Yes, that's what the current code does.
Ideally, every bit of currently hardcoded behavior (comments + form on 'full', comment count on 'teaser', 'add new comment' on other modes) would be customizable per build-mode.
We're obviously not there yet in terms of UI, but this patch is a clear step towards this.

- White spaces: well, my IDE automagically removes them on file save, and I don't always manually clean them on large patches.
Removed that line.

Still TODO: item 8) in the OP: "deprecate the 'teaser' and 'fulltext' settings for the feed_item_length variable used in node_feed()".
Could possibly be done in a followup patch, since this is already broken by the 'Body as field' patch.

Comment #13

yched CreditAttribution: yched commented 17 June 2009 at 00:33

Status:

Needs work

» Needs review

The point about 'feed_length_item' is in fact a separate followup issue from 'Body as field'.
Created #493674: 'Feed content' setting broken after 'Body as field' for this.

The patch here doesn't interfere, so there's no reason to hold this up. Back to CNR after updates in #12.

Comment #14

moshe weitzman CreditAttribution: moshe weitzman commented 17 June 2009 at 04:31

Looks very nice.

Perhaps add a comment to node_field_build_modes() about why the search build modes are there. How about "Since node_search() is responsible for rendering its search results, we declare the build_modes here instead of search.module."
Is $build_mode redundant since it is already in $node? Here is one of several examples: function hook_node_build_alter($node, $build_mode). I'm OK with keeping $build_mode even if it redundant if you think it is better.

Comment #15

yched CreditAttribution: yched commented 17 June 2009 at 11:02

File	Size
build_modes-409750-14.patch	44.61 KB

re #14:
1. Added a word about search modes in node_field_build_modes()
Also moved 'print' mode into book_field_build_modes(). If book.module is disabled, nothing in core uses 'print' mode.
The contrib print.module can expose the 'print' mode in its own print_field_build_modes().

2. There is no redundancy: the patch removes $node->build_mode as a property, and makes it a parameter of the build() functions (replaces and extends the previous $teaser param). That's what it is, really: not an inherent property of the node, but a specification of how you want this node built.
Or am I missing your point ?

Comment #16

moshe weitzman CreditAttribution: moshe weitzman commented 17 June 2009 at 11:18

Status:

Needs review

» Reviewed & tested by the community

this fine work is rtbc.

Comment #17

17 June 2009 at 11:20

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch failed testing.

Comment #18

sun

German

Karlsruhe

CreditAttribution: sun commented 17 June 2009 at 11:39

"Failed to install HEAD" ? Strange.

Super-minor nitpicks:

+++ modules/node/node.tpl.php	17 Jun 2009 11:01:52 -0000
...
+ * - $teaser: Flag for the teaser state (shortcut for $build_mode == 'teaser)

Missing trailing quote for 'teaser'.

 /**
- * An implementation of hook_node_view().
+ * Implement hook_node_view().
  */
-function comment_node_view($node, $teaser) {
+function comment_node_view($node, $build_mode) {

Should be "Implementation of hook_node_view()."

Aside from that: AWESOME.

Comment #19

lilou CreditAttribution: lilou commented 17 June 2009 at 12:27

Status:

Needs work

» Reviewed & tested by the community

HEAD is broken : http://testing.drupal.org/node/35

Comment #20

yched CreditAttribution: yched commented 17 June 2009 at 12:50

File	Size
build_modes-409750-20.patch	44.64 KB

re #18:
- fixed the trailing quote (and added missing trailing point)
- "Implement hook_foo()." now seems the official pattern, used throughout all of core. The patch simply fixes a couple remaining occurrences of the old form in areas touched by diffed lines. New patch fixes another occurrence (taxonomy_node_view)

Leaving RTBC for now, but running tests one last time when the bot is up again would probably be a good thing.

Comment #21

yched CreditAttribution: yched commented 17 June 2009 at 12:54

File	Size
build_modes-409750-21.patch	44.68 KB

Oops, typo in PHPdoc.

Comment #22

yched CreditAttribution: yched commented 17 June 2009 at 21:13

Status:

Reviewed & tested by the community

» Needs review

Bot is back, it seems.

Comment #23

yched CreditAttribution: yched commented 18 June 2009 at 19:11

Status:

Needs review

» Reviewed & tested by the community

Bot is happy, so back to RTBC.

Comment #24

20 June 2009 at 00:20

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch failed testing.

Comment #25

moshe weitzman CreditAttribution: moshe weitzman commented 21 June 2009 at 15:16

Status:

Needs work

» Reviewed & tested by the community

File	Size
build_mode.patch	47.22 KB

Odd that the forum hunk would no longer apply. here is a straight re-roll. please wait for bot to go green before committing.

Comment #26

yched CreditAttribution: yched commented 21 June 2009 at 15:26

forum hunk didn't apply because of a minor recent change in forum_node_view(), breaks patch context lines.
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/forum/forum.module...

Thks for the reroll, moshe.

Comment #27

21 June 2009 at 15:30

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch failed testing.

Comment #28

yched CreditAttribution: yched commented 21 June 2009 at 16:00

Status:

Needs work

» Reviewed & tested by the community

File	Size
build_modes-409750-28.patch	45.84 KB

Rerolled patch missed the

-function forum_node_view($node, $teaser) {
+function forum_node_view($node, $build_mode) {

hunk.

Comment #29

moshe weitzman CreditAttribution: moshe weitzman commented 21 June 2009 at 17:11

Doh. Got my patches confused. Thanks yched.

Comment #30

Dries CreditAttribution: Dries commented 21 June 2009 at 18:22

I'd like to get your thought on the following ...

After staring at this patch for 15 minutes, it feels like a natural next step, but at the same time, it feels like build mode is overloaded -- we shoe-horned a whole lot of stuff in a single variable and as a result, this "overhaul and extend" (cfr. title) might not live up its potential. While that could be left to future patches, I wonder if we need to pave the path some more.

The build mode specifies both the output format (HTML, XML/RSS), output variant (preview, full node) and a workflow state (the odd preview'-toggle). That is probably OK but at some point it might be nice to be able to say: give me the JSON version of the teaser-node, or give me the RDF version as a search result snippet, or give me the Atom version of the full-node. The current build mode is not going to scale.

As I said, it is not something I expect us to tackle in this issue but it kept me thinking about the name of the 'build mode' variable. Really, what it is is a key that describes any combination of output format, output variant and -- to some ugly extend -- the workflow state. Maybe something like 'render_engine' is slightly more accurate and a tiny bit more visionary? Or maybe we should adopt a format like format_variant (e.g. html_teaser, rss_teaser) so we get better separation of those concepts. Or maybe build_mode should be an array like array('format' => 'html', 'variant' => 'teaser')?

Anyway, thanks for listening to my quick braindump.

PS: I think 'is_preview' is slightly better than 'in_preview' (and slightly more consistent).

Comment #31

yched CreditAttribution: yched commented 22 June 2009 at 00:09

"The build mode specifies both the output format (HTML, XML/RSS), output variant (preview, full node) and a workflow state (the odd preview'-toggle). "

No, with the current patch, build mode only controls 'output variant' (teaser, full node, search index...):
- it is orthogonal to 'output format': node_build($node, $build_mode) generates a render array, that's all. What you generate out of that render array (HTML, XML/RSS) is entirely undecided at this point. The output format would be a feature for drupal_render(), which currently only generates HTML. Two separate steps: build time, render time. Build modes only take part in the former.
Note that the current uses of 'rss' mode still render into *HTML*, which then happens to be crammed in the 'description' tag of an RSS item.

- it is orthogonal to 'in preview': the patch precisely moves 'in preview' from a build mode in D6 into a state flag ($node->in_preview).
You can use any build mode to display in preview. Current code displays 'full' and 'teaser', but, as I wrote in the OP, a cool feature would be to allow a contrib module control which build modes are displayed on preview (full, teaser, custom_1, custom_2...)

"it feels like build mode is overloaded -- we shoe-horned a whole lot of stuff in a single variable"

I disagree; on the contrary, the patch makes it clearer that build modes are one single thing: a context, that node components can use to provide variants. Also from the OP:
"The ultimate goal IMO is to have 'modes' be just empty shells, with as little actual behavior hardcoded as possible (merely providing sensible defaults). 'teaser' is just the mode used for default entity listings, 'rss' is the one used by default feeds, etc. What actually happens in each mode is configurable, the main entry point being the fields display settings."
Legacy code still keeps us away from this, but in an ideal future, the part of the Fields UI which controls display settings would also let you decide, for instance, how comments appear in each build mode: full comments, only the last n, only a counter..., or which build modes include the book navigation.
Also note that with Views D6 you can already generate lists or feeds using other build modes than 'teaser' or 'rss'. Build modes are only strings...

Comment #32

yched CreditAttribution: yched commented 22 June 2009 at 00:08

Forgot to add: the 'extend' in the issue title only means 'extend build modes to non nodes', not 'cram more meaning in there'.

Comment #33

recidive CreditAttribution: recidive commented 22 June 2009 at 07:02

Maybe it's not in the scope of this patch but I'd like to see hook_view() and hook_node_view() renamed to something else, maybe hook_build()? Later we can have hook_render() for rendering tasks? hook_view() made sense in the past, but don't make sense anymore IMO.

Anyway, nice improvement!