Fix viewfield rendering

Moving this item to the 7.x branch.

theme_viewfield_select() and theme_viewfield_formatter_default() are gone.

theme_viewfield_select() only existed to provide a bit of css to ensure that help text for multiple-value viewfields wrapped correctly. It has been replaced with a small addition to the field settings form render array. I was tempted to turn to inline css here but I don't know any guidelines for this sort of thing so I left it as a file include.

theme_viewfield_formatter_default() wasn't a real theme function, consisting primarily of data access operations. The data access part has been relocated, and the theme part has been removed since D7 core provides field-level theming support already.

One other note: because the D7 branch pushes to $_viewfield_stack before token replacement (this is reversed in 6.x), it is unnecessary to perform an addition recursion check within token replacement. See how $replacing_tokens is used in the old _viewfield_get_view_args() vs. the new _viewfield_get_token_values().

@sun Do you have time to review this? I'd appreciate it.

+++ b/viewfield.module
@@ -71,21 +71,15 @@ function viewfield_field_formatter_info() {
-        $elements[$delta] = array(
-          '#markup' => theme('viewfield_formatter_default', array(
-            'item' => $item,
-            'entity_type' => $entity_type,
-            'entity' => $entity,
-          )),
-        );
+        $element[$delta] = array('#markup' => _viewfield_get_view_output($item, $entity_type, $entity));

I like the other changes concerning the theming/rendering, but I think we should keep this consistent with other field formatters and use a theme function.

In D7, we can offload the heavy technical preprocessing into a template_preprocess_viewfield_formatter_default() function, so the actual theme function can only deal with theme variables, as usual. This also allows us to provide the raw $view object as variable to the theme function (by assigning it in the preprocess function).

In the end, each item should use the #theme property (instead of #markup).

+++ b/viewfield.module
@@ -277,6 +258,131 @@ function viewfield_views_api() {
+function _viewfield_parse_args($vargs) {

It looks like we're storing the args as a comma-separated string? We should store the args in a way that's ready for usage; e.g., a serialized array.

The field widget then has to care for imploding/exploding those args in whatever way makes sense for it. (In the long run, we should replace that textfield with a #ajax-driven widget that allows to enter the individual args into separate textfields or whatnot.)

I additionally have the impression that this argument parser can be heavily simplified by using a combination of strtok() with preg_match(). It's also looking extremely complex, so we should add unit tests for it. While d.o doesn't allow us to test functionality depending on third-party modules yet, we can definitely write tests for low-level helper functions like this. Do you know how to write tests?

+++ b/viewfield.module
@@ -277,6 +258,131 @@ function viewfield_views_api() {
+/**
+ *  Detect if a token requires a full entity to be loaded.
+ *
+ *  We conservatively require a load if a token refers to the entity type but
+ *  the token key is not the entity id. For example, if the $entity_type ==
+ *  'node' and $entity_id_key == 'nid', we are searching for tokens of the
+ *  form [node:some-key] where some-key != 'nid'.
+ *  Global tokens (e.g., [current-user:uid]) never require entity loading.
+ */
+function _viewfield_token_requires_load($text, $entity_type) {

I'm not sure whether this entire function makes sense -- it looks like premature optimization to me - whereas it's not clear to me whether entity ID token replacements are able to work without a properly loaded entity.

Additionally, since these tokens are used within the context of a field of an entity, it's very likely that the entities referred in the tokens have already been loaded through other means.

In the end, I'm not sure whether this optimization makes sense, and whether it really buys us a huge performance gain -- compared to the possible bugs it might cause.

Also, minor: Duplicate leading space in the phpDoc.

Powered by Dreditor.

Note: I've added a test file + also cleaned up some other minor stuff: http://drupalcode.org/project/viewfield.git/commit/093c3d0

@sun Thanks for your review.

I like the other changes concerning the theming/rendering, but I think we should keep this consistent with other field formatters and use a theme function.

I had looked at retaining the theme function but in the end discarded the idea for three reasons. First, core fields (number, list, text) don't use them, they go straight to #markup. I thought if it's good enough for core, it's good enough for viewfield. Second, themers have field.tpl.php and views*.tpl.php available already. Third, I experimented with a theme function and ended up with a function that did nothing but call drupal_render(), which didn't seem very helpful. (Calling drupal_render seems like the only good option because views can't produce a render array, just markup (see http://drupal.org/node/1096286), so the input to the theme function looked like array('content' => array('#markup' => '<views stuff>')).

I might change my opinion about this though if we decide enhance viewfield to include the view title in the output (see #281409: Show view title in viewfield output). $view->preview() doesn't supply it, and while it's easy enough to dig the title out of the view, there would be viewfield specific markup to handle it, and we would want to allow themers to override that markup. In addition to a theme function, including the title would require some sort of administrative interface to at least enable/disable it. Certainly if we support a theme function, the formatter should use #theme rather than #markup.

Thanks too for the tip on template_preprocess_viewfield_formatter_default(). I think the new Drupal slogan should be "Drupal: We Think of Everything." (Though as a core developer you might not agree!)

Having gotten all this out, I suppose it wouldn't be a bad idea to continue support a theme function in case we need viewfield-specific markup in the future.

It looks like we're storing the args as a comma-separated string? We should store the args in a way that's ready for usage; e.g., a serialized array. [...] I additionally have the impression that this argument parser can be heavily simplified [...]

Yes, though I've tried to keep the changes in this patch limited to the rendering structure. How about handling this in a separate issue?

While d.o doesn't allow us to test functionality depending on third-party modules yet, we can definitely write tests for low-level helper functions like this. Do you know how to write tests?

I've used TDD in java and .net but not php or Drupal. From what I've seen it doesn't look too hard. Adding some unit tests probably makes sense. (From what I've seen however, the prospect of creating the views, nodes, and tokens to thoroughly test viewfield looks like a significant piece of work.)

I'm not sure whether this entire function makes sense -- it looks like premature optimization to me - whereas it's not clear to me whether entity ID token replacements are able to work without a properly loaded entity.

Additionally, since these tokens are used within the context of a field of an entity, it's very likely that the entities referred in the tokens have already been loaded through other means.

In the end, I'm not sure whether this optimization makes sense, and whether it really buys us a huge performance gain -- compared to the possible bugs it might cause.

Well that's interesting. I've observed that entity ID token replacements are able to work without a properly loaded entity but I don't know that's guaranteed in all cases. I'm not even sure how to find that out. In the end _viewfield_token_requires_load() smells a little strange, and if indeed it doesn't produce a significant performance gain I'm fine with losing it if you want.

1. Yup, let's do the #theme + template_preprocess_viewfield_formatter_default(). Whenever you know that there might be a chance in the future to have a possible need for more flexibility, and you're at a point having to make a technical design decision, it almost always pays out to account for the possibility already.
2. Yeah, the arguments serialization is definitely a separate issue. Ideally, let's fix that before the official release.
3. We can export and load a simple default view through a testing module to make testing easier. Overall, the initial setup for testing might be some work indeed, but as soon as that is done, we can write many tests very quickly, and it will definitely pay out in the end. Separate issue though.
4. Let's remove that premature performance optimization for tokens for now.

Posting my patch from #1216684: Merge include files into .module, since it probably makes sense to just deal with the include over there, and do the heavier render lifting here.

Re-rolled against latest branch HEAD.

sorry, lost a hunk

In #11 it looks like the old version of theme_viewfield_formatter_default() was left in.

+++ b/viewfield.module
@@ -180,6 +199,68 @@ function theme_viewfield_formatter_default($variables) {
+function viewfield_post_render($element) {
+  $stack = &drupal_static('viewfield_stack', array());
+
+  unset($stack[$element['#entity_type']][$element['#entity_id']]);
+
+  return $element;
+}

This should read:

function viewfield_post_render($content, $element) {
  $stack = &drupal_static('viewfield_stack', array());

  if (!empty($element['#entity_id'])) {
    unset($stack[$element['#entity_type']][$element['#entity_id']]);
  }

  return $content;
}

+++ b/viewfield.module
@@ -180,6 +199,68 @@ function theme_viewfield_formatter_default($variables) {
+  // @todo Move load + access check into #pre_render or viewfield_field_formatter_view()?

I took a crack at this since theme_viewfield_formatter_default() should be open to modification by themers, and we want to keep private functions out of it anyway.

+++ viewfield.module
@@ -119,16 +130,39 @@ function viewfield_field_formatter_info() {
+        if ($view && $view->access($view_display)) {
...
+          '#access' => $access,

Actually, #access should just simply be the value of $view->access() - i.e., we unconditionally add the $view.

The downside of this is that we're always extracting/parsing the args, but we need to store those serialized anyway, and as soon as we do that, it's no longer an issue.

BTW, nice patch :)

Actually, #access should just simply be the value of $view->access() - i.e., we unconditionally add the $view.

In #13 that is the value of #access: FALSE unless $view is defined and $view->access() is true. We could also go ahead and add the $view even if access is denied, but no one will ever see it. I NULLed it out because of consistency with argument handling (see next).

The downside of this is that we're always extracting/parsing the args, but we need to store those serialized anyway, and as soon as we do that, it's no longer an issue.

_viewfield_get_view_args() is misnamed. It not only parses the argument string, it also performs entity loading and token replacement. If access is denied, we shouldn't do those things, which is why I gave $views_arguments a NULL value under in the no-access case.

Am I missing something?

+++ viewfield.module
@@ -119,16 +130,39 @@ function viewfield_field_formatter_info() {
+          // @todo Store views arguments serialized.
+          $view_arguments = _viewfield_get_view_args($item['vargs'], $entity_type, $entity_id);
...
+          '#view_arguments' => $view_arguments,

In that case, let's directly assign 'vargs' as #view_arguments and do a delayed argument loading in the #pre_render? (replacing #view_arguments)

so... attached patch incorporates the last comments, and also...

makes "viewfield" available as a stand-alone element #type :)

Cool!

+++ b/viewfield.module
@@ -119,16 +130,24 @@ function viewfield_field_formatter_info() {
+        list($view_name, $view_display) = explode('|', $item['vname'], 2);
+
         $elements[$delta] = array(
+          '#type' => 'viewfield',
+          '#access' => $view->access($view_display),
+          '#view' => views_get_view($view_name),

Have to actually get the view :)

        list($view_name, $view_display) = explode('|', $item['vname'], 2);
        $view = views_get_view($view_name);
        $elements[$delta] = array(
          '#type' => 'viewfield',
          '#access' => $view && $view->access($view_display),
          '#view' => $view,

+++ b/viewfield.module
@@ -137,46 +156,72 @@ function viewfield_field_formatter_view($entity_type, $entity, $field, $instance
+    $stack[$element['#entity_type']][$element['#entity_id']] = TRUE;

This still has to be conditioned on nonempty valid entity_id.

+++ b/viewfield.module
@@ -137,46 +156,72 @@ function viewfield_field_formatter_view($entity_type, $entity, $field, $instance
+  unset($stack[$element['#entity_type']][$element['#entity_id']]);

Has to be conditioned on nonempty entity id.

Otherwise looks good to go.

wrong status then? ;)

Note that #entity_id can be NULL, and that's perfectly fine:

> php -r "$a = array(); $a[NULL] = TRUE; var_dump($a); unset($a[NULL]); var_dump($a);"
array(1) {
  [""]=>
  bool(true)
}
array(0) {
}

...and just to double-confirm, we're also not hitting a PHPWTF ;)

> php -r "$a = array(); $b = NULL; $a[$b] = TRUE; var_dump($a); unset($a[$b]); var_dump($a);"
array(1) {
  [""]=>
  bool(true)
}
array(0) {
}

Well, it's really a question of semantics. We're using the entity id to uniquely record the presence of an entity in the viewfield rendering process. NULL isn't really suitable for that job unless we can argue that there will only ever be at most one entity being previewed at a time, and thus that NULL is it's (temporary) unique identifier. Even if we can make that argument I somehow think the logic is clearer with the !empty() checks :)

Ok, short form is: I starred like 5 minutes at #pre_render and tried to think of the proper conditional logic to perform when special-casing NULL, in which case we want to render, but skip the stack, but also perform argument loading/replacement ;) If you can figure that logic out, be my guest :)

I stared for more than 5 minutes :) and concluded it would considerably hair up #pre_render for practically no benefit. So... attaching your latest patch with a tweak: $view assignment in viewfield_field_formatter_view().

heh, thanks! :)

Committed: http://drupalcode.org/project/viewfield.git/commit/9c7fcfd.

Right now, is there a way how to display the view title in the viewfield? Even a hackish one... Would have to use some ugly jQuery magic if not.

Thanks in advance,
zloutenka

@zloutenka: Please check #281409: Show view title in viewfield output for this issue.