Redundant executeView() in ViewsBlock::build()

In theory we could unit test that but I am not sure whether it is worth to do it.

We could actually just not use the if at all, as currently it's a bit messed up. If $output returns it will be a string, otherwise it will be an empty array. The result of execute() from the Block display plugin should handle this for us instead, and return a render array, and NOT a string as it's currently doing. What do you think?

I think the if for executeDisplay is important ... as things maybe fail.

This touches all the code which is fixed by #1957346: Add some settings on the block display to allow overrides on the block instance configuration already.

Oh yeah, that does touch the same build() method. However, look at the changes in Drupal\views\Plugin\views\display\Block here. that patch is wrong, we should be checking the output too I guess BUT the block display plugin should not be returning a string, we are trying to get away from this aren't we?

The point is that executeDisplay can return FALSE, if the display is missing for example. Yes an exception is thrown but maybe something else failed in the meantime.

We could still use the if (), and return our render array. I think we can have the best of both here..

Yeah I totally think we should do that!

So, how about something like this? If this is fixing the same parts of code in the other issue, do you think we should just move this there too?

+1

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -1333,7 +1333,7 @@ public function render($display_id = NULL) {
       if (!$this->setDisplay($display_id)) {
-        return FALSE;
+        return NULL;

This change seems out of scope. FALSE or NULL is more or less the same meaning, but FALSE seems to indicate more then something went wrong.

Let's get it in as it is now and improve later.

Committed c66a78c and pushed to 8.x. Thanks!