Convert (HTML) view rendering to a render array

Updated issue summary, particularly to include the info in #3.

AWESOME! :)

1. +++ b/core/modules/views/src/Annotation/ViewsDisplay.php
   @@ -131,4 +131,11 @@ class ViewsDisplay extends ViewsPluginAnnotationBase {
   +   * Whether the plugin returns a response object.
   +   *
   +   * @var bool
   +   */
   +  public $return_response;
   

   Shouldn't this then be called returns_response?

   And s/the plugin/the views display plugin/?

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2221,6 +2219,25 @@ public function preExecute() {
   +   * Builds a render array when the view is rendered as render array.
   

   This reads a bit bizarre :P

3. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2221,6 +2219,25 @@ public function preExecute() {
   +    $build = [
   +      '#type' => 'view',
   +      '#name' => $this->view->storage->id(),
   +      '#display_id' => $this->display['id'],
   +      '#arguments' => $args,
   +      '#embed' => FALSE,
   +      '#view_executable' => $this->view,
   +    ];
   +
   +    return $build;
   

   Why assign to a variable first if you're just going to return it anyway?

4. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1396,6 +1396,31 @@ public function render($display_id = NULL) {
   +   * This render array has a #pre_render callback which will call
   +   * ::executeDisplay in order to actually execute the view etc.
   

   Seems like this comment could use some polish.

5. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1396,6 +1396,31 @@ public function render($display_id = NULL) {
   +  public function buildRenderForDisplay($display_id = NULL, $args = array()) {
   

   We usually call the building of a render array build(). This is true for block rendering, entity rendering, etc. Hence buildDisplay() would be a more consistent name, if that would still make sense within Views land?

6. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1396,6 +1396,31 @@ public function render($display_id = NULL) {
   +    // @todo Extract that into a generic method.
   

   Should this still happen in this patch, or…?

7. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1396,6 +1396,31 @@ public function render($display_id = NULL) {
   +    return $this->display_handler->buildRender($args);
   

   … and this would then be called build().

8. +++ b/core/modules/views/views.module
   @@ -97,9 +99,27 @@ function views_views_pre_render($view) {
   +      // Add contextual links to the view. We need to attach them to the dummy
   +      // $view_array variable, since contextual_preprocess() requires that they be
   +      // attached to an array (not an object) in order to process them. For our
   +      // purposes, it doesn't matter what we attach them to, since once they are
   +      // processed by contextual_preprocess() they will appear in the $title_suffix
   +      // variable (which we will then render in views-view.html.twig).
   

   80 cols.

9. +++ b/core/modules/views/views.module
   @@ -97,9 +99,27 @@ function views_views_pre_render($view) {
   +      views_add_contextual_links($element, 'view', $view, $view->current_display);
   

   WOOT! This changes it so that contextual links are added in the #pre_render stage rather than in the theme preprocess stage, which is much cleaner!

Can we PLEASE stop the business of make it hard for people to think out loud in code? A todo itself is a fine thing and having one
is 100times MUCH better than be silent about it. Having a follow up just adds a bit on top of it.

I know! I agree!
But at the same time, you can't blame reviewers for not knowing what the scope of a @todo is: it can be either meant as a reminder for something that should happen in a next reroll of the patch, or it could be meant to be committed.

Decided to go directly with buildRender to be more in sync with the method on the display.

Why not just build()? buildRender() is confusing because it's two consecutive verbs. If you want to stick with what you have, buildRenderable() would be easier to understand.

Plus one final nitpick:

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2221,6 +2219,26 @@ public function preExecute() {
+   * Note: This does not contain yet the executed view, but just the loaded

s/does not contain yet/does not yet contain/

ViewExecutable::build() already exists out there.

Good point. Missed that, sorry.

I still think ::buildRenderable() would be better though. I think you might've missed that?

Hehe :)

No more complaints!

If needed for performance (caching), should be Major priority.

This patch needs a quick re-roll because some test files where moved around. Otherwise this looks good to me.

For the record, the most recent patch did not apply cleanly but it would have applied cleanly as a git merge (or rebase). I posted the details to www.mosheweitzman.me/post/96720635168/improve-drupal-org-code-workflow-w...

It'd be great if the summary can be updated with the actual resolution.

Meant to set to needs work to get the issue summary updated.

Great that this was set to NR so testbot picks this up, but back to NW per #26/#27.

Hurray, #2318377: Determine whether a view is cacheable and its required contexts, store this i/t config entity is in! This still/again looks great, so: RTBC!

Committed/pushed to 8.0.x, thanks!