Replace theme() with drupal_render() in comment module

Replaced theme with drupal_render() in the comment module. Patch attached.

Fixed whitespace issues.

I am sorry, Anonymous!

Made changes as suggested by thedavidmeister.

Unassigning this issue.

Needs a reroll

curl https://drupal.org/files/comment-replace_theme_with_drupal_render-2008980-8.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8012  100  8012    0     0   5200      0  0:00:01  0:00:01 --:--:--  6529
error: patch failed: core/modules/comment/lib/Drupal/comment/Plugin/views/field/NcsLastCommentName.php:62
error: core/modules/comment/lib/Drupal/comment/Plugin/views/field/NcsLastCommentName.php: patch does not apply
error: patch failed: core/modules/comment/lib/Drupal/comment/Plugin/views/field/Username.php:54
error: core/modules/comment/lib/Drupal/comment/Plugin/views/field/Username.php: patch does not apply

Attaching patch after reroll

#12: comment-replace_theme_with_drupal_render-2008980-12.patch queued for re-testing.

back to RTBC per #10.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/row/Rss.phpundefined
@@ -154,11 +154,13 @@ function render($row) {
+    $build = array(
+      '#theme' => $this->themeFunctions(),
+      '#view' => $this->view,
+      '#options' => $this->options,
+      '#row' => $item,
+    );
+    return drupal_render($build);

Looking at RowPluginBase I think we can just do

    return array(
      '#theme' => $this->themeFunctions(),
      '#view' => $this->view,
      '#options' => $this->options,
      '#row' => $item,
    );

... needs testing though

Actually potentially the best way of doing this would be to do this... instead of the array building...

  return parent::render($item);

I have made the change suggested in #17. While looking for an example, I looked in this class implementation for node and found the same code as this:

    $theme_function = array(
      '#theme' => $this->themeFunctions(),
      '#view' => $this->view,
      '#options' => $this->options,
      '#row' => $item,
    );
    return drupal_render($theme_function);

If this is the more correct way to do this, would it be better to reopen #2009660: Replace theme() with drupal_render() in node module?

The recent change broke the test. Here is my analysis:

The code breaks because of this block of code in \Drupal\views\Plugin\views\style\Rss. File: core/module/views/lib/Drupal/views/Plugin/views/style/Rss.php, line 136.

    foreach ($this->view->result as $row_index => $row) {
      $this->view->row_index = $row_index;
      $rows .= $this->view->rowPlugin->render($row);
    }

It is clear that the string concatenation operation expects the value to be string, whereas we return an array. However, like @alexpott pointed out in #16, the base class RowPluginBase returns an array. This is clearly a problem.

To confirm, I checked in D7 and the string concatenation is indeed present. So, I checked the style plugin and this is the code in views/plugins/views_plugin_row.inc.

  /**
   * Render a row object. This usually passes through to a theme template
   * of some form, but not always.
   */
  function render($row) {
    return theme($this->theme_functions(),
      array(
        'view' => $this->view,
        'options' => $this->options,
        'row' => $row,
        'field_alias' => isset($this->field_alias) ? $this->field_alias : '',
      ));
  }

It seems that the theme() call was removed in 8.x, but not replaced with drupal_render. This happened in #77 of #1811828: Use #attached to find the css/js of a view. I looked briefly through the issue and I am not clear why this change was introduced there.

Now, that issue is marked fixed and I don't think that should be opened. Does this warrant a new issue? From what I see, we have to call drupal_render in the line where we concatenate strings or we need to return strings from RowPluginBase::render (and other methods changed in #1811828: Use #attached to find the css/js of a view).

For now, I think we can go ahead with patch from #12 as this might be a different issue altogether. Or we can create a new issue and mark this issue as postponed until that is fixed. Thoughts?

Or we can forget about #16 since as #20 points out core/module/views/lib/Drupal/views/Plugin/views/style/Rss.php, does not support return an array form render()... and we can go back to the patch in #12... which actually still applies.

I agree with @alexpott in #22. But I think it applies to create a new issue for #20. What do you think?

I have created a new issue at #2023635: Return render arrays from RowPluginBase::render overrides to discuss the fix the problems in #18 and #20. Meanwhile, I think the patch in #12 can be used for now.

Oops... added Novice tag by mistake.

#12: comment-replace_theme_with_drupal_render-2008980-12.patch queued for re-testing.

Patch in #12 still applies cleanly, code review looks good. Queuing for re-test just in case anything has changed and then it can be RTBC'd.

It looks like all the tests have passed and this can be applied cleanly. I too think patch in #12 is the way to go here and the suggested changes should go in the new issue at #2023635: Return render arrays from RowPluginBase::render overrides.

Yep, if the decision to adopt 12 is taken, then I'm happy to RTBC #12.

Committed 39cc830 and pushed to 8.x. Thanks!