Clean up views rendering. Move stuff from template_preprocess_views_view(), into a #pre_render callback

1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
   @@ -2130,11 +2130,59 @@ public function executeHookMenuLinkDefaults(array &$existing_links) {
   +      '#attached' => &$this->view->element['#attached'],
   +      '#cache' => array(
   +        'keys' => array('views', 'view', $view_id, $this->view->current_display, $this->view->getCurrentPage()),
   +        'tags' => array('view' => array($view_id => $view_id)),
   +      ),
   +      '#pre_render' => array(array(get_class($this), 'preRender')),
   

   It would be kinda neat to at least have the #cache element generation in an extra method, just because it will probably contain more bits in the future with the cache plugin?

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
   @@ -2130,11 +2130,59 @@ public function executeHookMenuLinkDefaults(array &$existing_links) {
   +  public static function preRender($element) {
   

   what about using elementPreRender, to distinct it from the preRender parts in views itself?

3. +++ b/core/modules/views/views.theme.inc
   @@ -49,39 +44,6 @@ function template_preprocess_views_view(&$variables) {
   diff --git a/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
   
   diff --git a/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
   index f959d0d..43bf409 100644
   
   index f959d0d..43bf409 100644
   --- a/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
   
   --- a/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
   +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
   
   +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
   +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
   @@ -566,7 +566,6 @@ public function renderPreview($display_id, $args = array()) {
   
   @@ -566,7 +566,6 @@ public function renderPreview($display_id, $args = array()) {
        if (empty($errors)) {
          $this->ajax = TRUE;
          $this->executable->live_preview = TRUE;
   -      $this->views_ui_context = TRUE;
    
          // AJAX happens via HTTP POST but everything expects exposed data to
   

   Yeah that is not used anymore but kinda out of scope tbh. I kinda like the semantic of this variable ...

#4: that probably makes more sense, yes :)

Drupal\views\ViewExecutable::$row_index now being undefined is responsible for pretty much all exceptions, and possibly also quite a few fails.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
@@ -2133,8 +2134,79 @@ public function render() {
+      $form_object = new ViewsForm($container->get('controller_resolver'), $container->get('url_generator'), $container->get('request'), $view->storage->id(), $view->current_display);

Should't we just call ViewsForm::create($container) instead?

This is just a little bit nicer to maintain

Is #9 a straight reroll?

Let's see ...

meh.

Replacing $view->row_index with ResultRow->index, which is much nicer in general.

Certain additional issue, which is needed: #1849822: Convert (HTML) view rendering to a render array

Did some work though this is blocked by another issue.

muh

Currently blocked on #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render

Let's make it clear #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render

-    $attached = $this->storage['output']['#attached'];
+    $attached = drupal_render_collect_attached($render_array, TRUE);

drupal_render_collect_attached() doesn't exist anymore. I don't think you still want this change. That should help solve the 2 fails :)

Oh, hah, sorry! Silly me :) Just trying to help, but being useless :P

+++ b/core/modules/views/src/Plugin/views/style/Rss.php
@@ -44,14 +44,10 @@ public function attachTo($display_id, $path, $title) {
-        $build['#attached']['drupal_add_feed'][] = array($url, $title);
-        drupal_process_attached($build);
+        drupal_add_feed($url, $title);

Are we sue we want to do it that way instead of attaching it to some render array?

I really like the status of the patch.

@damiankloip
The IS looks really nice and clean!

So much better and not actually braindead!

+++ b/core/modules/views/views.theme.inc
@@ -23,20 +23,13 @@
+  $variables['css_name'] = drupal_clean_css_identifier($id);

What about Html::cleanCssIdentifier directly?

Code looks good to me too. Can we get an issue summary please? I'll set back to RTBC afterwards.

+++ b/core/modules/views/src/Plugin/views/style/Rss.php
@@ -44,7 +44,8 @@ public function attachTo($display_id, $path, $title) {
       if (empty($this->preview)) {
-        drupal_add_feed($url, $title);
+        // Add a call for drupal_add_feed to the view attached data.
+        $this->view->element['#attached']['drupal_add_feed'][] = array($url, $title);
       }
     }
     else {
@@ -53,14 +54,15 @@ public function attachTo($display_id, $path, $title) {

@@ -53,14 +54,15 @@ public function attachTo($display_id, $path, $title) {
         '#url' => $url,
         '#title' => $title,
       );
-      $feed_icon['#attached']['drupal_add_html_head_link'][][] = array(
+      $this->view->feed_icon = $feed_icon;
+
+      // Add a call for drupal_add_html_head_link to the view attached data.
+      $this->view->element['#attached']['drupal_add_html_head_link'][][] = array(
         'rel' => 'alternate',
         'type' => 'application/rss+xml',
         'title' => $title,
         'href' => $url,
       );
-      $this->view->feed_icon = $feed_icon;
-      drupal_process_attached($feed_icon);
     }

The reason why this doesn't work at the moment is that DisplayPluginBase::attachDisplays works on a clone of the view, see $this->displayHandlers->get($id)->attachTo($cloned_view, $this->current_display); I wonder whether we considered setting the main render array to the cloned view temporarily.

As talked in IRC, can we do something like the following?

Awesome, I really like it. So who can we force to RTBC that patch?

@Wim Leers @tim.plunkett?

One important thing, a few minor maintainability/grokkability things and a bunch of nitpicks. This is close.

1. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -142,8 +142,8 @@ public function cacheSet($type) {
   +        $this->gatherHeaders($this->view->display_handler->output);
   +        $this->storage['output'] = drupal_render($this->view->display_handler->output, TRUE);
   

   Shouldn't the order of this be the other way around?

   Only when drupal_render() has run, the #attached CSS and JS from the child, grandchild etc. levels will be available at the parent (root) level.

   I suspect the existing test coverage doesn't have any attached CSS/JS at levels beyond the root level, which would explain why this passes tests.

2. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -235,7 +240,7 @@ public function cacheStart() {
      /**
       * Gather the JS/CSS from the render array, the html head from the band data.
       */
   -  protected function gatherHeaders() {
   +  protected function gatherHeaders(array $render_array = []) {
   

   Docblock should be updated?

3. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -419,8 +420,15 @@ public function usesAreas() {
   +   *   The main view render array
   

   Missing trailing period.

4. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2125,11 +2133,82 @@ public function getMenuLinks() {
   +      '#attached' => &$this->view->element['#attached'],
   

   Please document why this needs to be assigned by reference.

5. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2125,11 +2133,82 @@ public function getMenuLinks() {
   +   * Pre render callback for view display rendering.
   

   I think we generally write "#pre_render callback", not "Pre render callback".

6. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2125,11 +2133,82 @@ public function getMenuLinks() {
   +   * @param array $element
   +   *
   +   * @return array
   

   Incomplete docblock. (I know… silly, but that's the rule.)

7. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2125,11 +2133,82 @@ public function getMenuLinks() {
   +  public function elementPreRender($element) {
   

   Typehint to array.

8. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2125,11 +2133,82 @@ public function getMenuLinks() {
   +    // Force a render array so CSS/JS can be added.
   

   s/added/attached/

9. +++ b/core/modules/views/src/Plugin/views/query/Sql.php
   @@ -1428,7 +1429,12 @@ function execute(ViewExecutable $view) {
   +        array_walk($view->result, function(ResultRow $row, $index) {
   +          $row->index = $index;
   +        });
   

   Elegant! :)

10. +++ b/core/modules/views/tests/src/Plugin/field/CounterTest.php
    @@ -86,8 +86,8 @@ protected function setUp() {
    +      $this->testData[] = new ResultRow($set + ['index' => $count]);
    

    s/$count/$index/

    ?

    $count makes little sense to me.

historically not for anything else really

Ah, right, that makes sense!

Do we want to make this change and support this? Probably, I guess.

I'd say so, yes. But it's probably wiser to defer that to a follow-up issue. That's really a new feature (or a bug fix, depending on how you look at it), so I'd rather not do it here if that turns out to open a new can of worms.

Committed 7ea1cd9 and pushed to 8.0.x. Thanks!

+++ b/core/modules/views/views.theme.inc
@@ -23,23 +22,16 @@
+  $variables['attributes']['class'][] = 'view-' . Html::cleanCssIdentifier($variables['id']);
   $variables['attributes']['class'][] = 'view-id-' . $variables['id'];
   $variables['attributes']['class'][] = 'view-display-id-' . $variables['display_id'];

Could just use $variables['css_name'] - also not sure why the two classes below are not using this? Perhaps file a follow up for this.

diff --git a/core/modules/field/src/Plugin/views/field/Field.php b/core/modules/field/src/Plugin/views/field/Field.php
index fd2a712..8a8dc18 100644
--- a/core/modules/field/src/Plugin/views/field/Field.php
+++ b/core/modules/field/src/Plugin/views/field/Field.php
@@ -721,7 +721,13 @@ protected function renderItems($items) {
   }
 
   /**
-   * Return an array of items for the field.
+   * Gets an array of items for the field.
+   *
+   * @param \Drupal\views\ResultRow $values
+   *   The result row object containing the values.
+   *
+   * @return array
+   *   An array of items for the field.
    */
   public function getItems(ResultRow $values) {
     $original_entity = $this->getEntity($values);
diff --git a/core/modules/views/src/Form/ViewsForm.php b/core/modules/views/src/Form/ViewsForm.php
index 6325b59..78e5207 100644
--- a/core/modules/views/src/Form/ViewsForm.php
+++ b/core/modules/views/src/Form/ViewsForm.php
@@ -8,7 +8,6 @@
 namespace Drupal\views\Form;
 
 use Drupal\Component\Utility\UrlHelper;
-use Drupal\Core\Controller\ControllerResolverInterface;
 use Drupal\Core\DependencyInjection\ClassResolverInterface;
 use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
 use Drupal\Core\DependencyInjection\DependencySerializationTrait;
@@ -68,7 +67,7 @@ class ViewsForm implements FormInterface, ContainerInjectionInterface {
   /**
    * Constructs a ViewsForm object.
    *
-   * @param \Drupal\Core\DependencyInjection\ClassResolverInterface $controller_resolver
+   * @param \Drupal\Core\DependencyInjection\ClassResolverInterface $class_resolver
    *   The class resolver to get the subform form objects.
    * @param \Drupal\Core\Routing\UrlGeneratorInterface $url_generator
    *   The url generator to generate the form action.
@@ -79,8 +78,8 @@ class ViewsForm implements FormInterface, ContainerInjectionInterface {
    * @param string $view_display_id
    *   The ID of the active view's display.
    */
-  public function __construct(ClassResolverInterface $controller_resolver, UrlGeneratorInterface $url_generator, RequestStack $requestStack, $view_id, $view_display_id) {
-    $this->classResolver = $controller_resolver;
+  public function __construct(ClassResolverInterface $class_resolver, UrlGeneratorInterface $url_generator, RequestStack $requestStack, $view_id, $view_display_id) {
+    $this->classResolver = $class_resolver;
     $this->urlGenerator = $url_generator;
     $this->requestStack = $requestStack;
     $this->viewId = $view_id;

Fixed the above on commit.

This issue caused #2350551: Views fields that have attached assets are lost when Views output caching is enabled.

This causes a notice when the Views output cache is being used: #2378815: DisplayPluginBase::elementPreRender() and View::preRenderViewElement() are called even when the Views output cache is being used, causing notice.