Add some settings on the block display to allow overrides on the block instance configuration

This honestly sounds like a really bad idea, that should be placed in contrib and not core. You are duplicating functionality, which means there is a large chance that people will confuse in which two places to go to edit the settings. Given that this usecase could easily be dealt with contib, I dont see why we would introduce such a large usability risk in core?

This honestly sounds like a really bad idea, that should be placed in contrib and not core. You are duplicating functionality, which means there is a large chance that people will confuse in which two places to go to edit the settings. Given that this usecase could easily be dealt with contib, I dont see why we would introduce such a large usability risk in core?

@Bojhan, see #1938062-58: Convert the recent_comments block to a view. Something like this is necessary to allow the configuration of the number of items through the block UI. @webchick has stated that this is a commit blocker for using Views for block listings, because content authors should be able to configure these options without going to the Views UI. I don't feel strongly about it, but I am willing to make this change to convert blocks to Views.

@dawehner, my idea was that we would always expose (some of) these options in the Block UI, so there would not need to be configure the options in the View.

Looking at the list:

• use a pager -- since this has performance implications, I wouldn't expose this one.
• items per page -- Expose in the Block UI, always.
• offset of the pager -- this might make sense to expose. Site builders could create some useful functionality by having two instances of the same block with different offsets.
• link to the view -- is this different from the more link?
• should there be a more link -- This makes sense to always expose.
• Override the title of the view -- This should always be available, through the Block UI, like in #1957276: Let users set the block instance title for Views blocks in the Block UI and for other blocks. The one enhancement would be to allow the Block UI to use Views' title placeholders, but that's beyond the scope of anything in SCOTCH yet.
• Should the exposed form be shown -- Do this in contrib instead.
• Display just certain fields -- Contrib.

So this issue can just do:
Number of items
More link

The rest in contrib.. Which someone could write now and we could play with.

@xjm I am not sure, webchick is be correct that we shouldn't regress on usability. But I feel little for just hopping over functionality at our own determination, if anything we should provide 1:1 functionality for the blocks we convert but adding new functionality is not something I think we should even try.

Ideally though, we still don't go down this road because it means duplicating functionality, override troubles etc. Even though it regresses usability, its also the result of moving to a new model - trying to keep the old model just because the views UI isn't as user friendly sounds like a bad idea. Centralisation of functionality means we can provide more resources to optimise it, having it all over the place means a distributed effort and will be harder in the long run. Although it might not be in scope of D8, providing work arounds to the real problem always ends up with nasty UX problems - so if anything lets try to minimize this as much as possible.

needs issue summary update to clarify the scaled back approach. Also that #1938062: Convert the recent_comments block to a view is blocked, waiting on this.

@YesCT, we don't have consensus here yet. For the moment, I'll simply update the summary to indicate that. I'll add a more complete summary once the VDC team has discussed it more with the usability team.

Also, we eventually need to discuss with webchick (and possibly Dries and catch if we can't come to an agreement) whether this is actually a blocker for block views. #1957276: Let users set the block instance title for Views blocks in the Block UI also blocks that issue, and the solution we choose there might have implications on what we do here.

I think this is actually a views.module issue, because we own the plugin.

@xjm thanks. :)

I think we have to double down on this approach to get *any* blocks converted for D8.

This is the best option we have for a bridge between the Block UI and the Views UI, and it is straight from contrib.

Just to clarify my stance:

- I don't care as much about configuring number of records; core is inconsistent with itself on whether or not that's supported on a per-block basis, and it's pretty rare to have to change it, so not the end of the world to tweak it in Views UI.
- I care a lot about configuring the block title, though. Every single other modules' blocks offers this capability, and users aren't going to understand why they can do this on e.g. their $foo block but not their $bar block. It's just going to be a completely inconsistent UX. And because this is directly in the face of users, it's something that less technical people like content authors are likely to want to tweak themselves.

Oops sorry, cross-post.

I just wanted to add some context, because I feel my position is being misunderstood. I am mostly worried about duplication and inheritance, from my point of view the Views interface should be the end point - we wish to get people at. Having intermediate steps, like block configuration of certain variables - is very useful, to a lot of users, however it also introduces the possibility of confusion - and when it comes to inheritance and duplication, this can get bad really quickly (e.g. people reaching for the wrong tool for the job).

Tim explained to me that we don't want people to make 5 display's (for 5 different pagers), although I understand this position - I am puzzled that creating more displays adds so much overhead. I think our long-term goal, should always be to provide one interface for the job - not two, that might mean turning technical concepts like displays on their head and/or vastly improving on the UX of Views. But from a UX point of view the desirable option in my mind is to unify towards one interface, not distribute to several. That way people know where to go, and know if its not there - it won't be at another place. I can see, that this is D9 talk.

A solution for what we currently have is to make the "connection" more clear, or we presented it over the past few years "to connect the dots". In this case there are two places where you can connect the dots, when configuring the block and when configuring the view. I have mocked up an example of this below:

A quick pointer, that this is comming from a view and you are overriding it here. The wording will need some work.

A pointer that changing this is fine, but it will leave Blocks X, Block Y unaffected.

"But from a UX point of view the desirable option in my mind is to unify towards one interface, not distribute to several."

I don't think that holds from a security POV, though, which was one of my points in the other issue. Giving people access to Views admin gives them access to all the content, users, etc. in your database. I think we really do need an interim solution that gives people access to edit some parts of the View (primarily the end user-facing bits) without giving them access to harvest peoples' e-mail addresses or view normally OG-protected content. I agree that explaining inheritance is difficult, but I don't think giving "access full database" permissions to every weirdo who needs to make a landing page on your site is a workable solution.

Yes, and this solution is one easy way to delegate limited power on a per-view basis.
You can say "this view can have a configurable pager" and "this other view can have a configurable more link" and those who can only place blocks don't need an admin to tweak views each time.

It's one of the major benefits of panels/panelizer and the panels IPE.

@webchick I know its D9 talk, but I'd imagine we could also provide a "limited view access" that gives acces to the same functionality we now wish to expose in blocks. I don't think its feasible to split up all permissions, but it should be possible to provide limited acces.

Interesting discussion. I agree with Bojhan especially with the following:
"moving to a new model - trying to keep the old model just because the views UI isn't as user friendly sounds like a bad idea."

Why not apply the trick we apply in the rest of Drupal? We add more permissions that give users more or less acces to parts of the Views UI. When a user only has the permission to edit the "Page settings" and "Pager settings" he only sees thinks like the Title and pager settings.

This way a site admin can control the acces (and complexity) for roles as he is used to.

I'm probably heavily under estimating the complexity of the Views UI, but shouldn't this be something like rendering or not rendering parts of the interface using if statements based on the permissions of the current user? The fact that this permission system is already used throughout Drupal should also help.

Also compared to adding a new interface, thinking out how stuff should overrule each other, design warnings here and there to point people towards these overrulings, designing the extra configuration to edit the blocks UI from the Views UI.

If this were D7, where each Views block display resulted in one Block placed into a region, that'd be trivial.

But now that a single Views block display can be placed infinite times by the Block UI, we have to devise an addition to the Views UI that will scale.

Hi Tim,
Could you clarify? What do you think will not scale? What do you think will scale?

I think adding any functionality of the Block UI to the Views UI will not scale.

I think adding links, as above, will scale.

#15: drupal-1957346-13.patch queued for re-testing.

One minor note:

+++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
@@ -67,13 +67,52 @@ public function form($form, &$form_state) {
+    parent::blockSubmit($form, $form_state);

This isn't necessary--the parent implementation's submit handling is in the submit() method, and BlockBase::blockSubmit() is an empty stub.

I think adding any functionality of the Block UI to the Views UI will not scale.

I think adding links, as above, will scale.

I think that, just in case, we should have a point at which it switches over to saying "in N blocks" in case some contrib on some giant site is using the block system in a way we don't expect. ;)

Well, maybe there will be something in there in the future so adding it prevents a potential bug.

Well, actually the whole point of the blockFoo() methods is that you don't have to call the parent.

Reroll and some doc and form fixes.

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/Block/ViewsBlockTest.phpundefined
@@ -0,0 +1,209 @@
+// @todo Remove this once t() is converted to a service.
+namespace {
+  if (!function_exists('t')) {
+    function t($string) {
+      return $string;
+    }
+  }

I think t is service now.

about t(): lets not start injecting things, unless absolutely necessary till #2018411: Figure out a nice DX when working with injected translation is in:) (RTBC plz!)

but we should link to that issue instead in the @todo note, since jibran is right, its already a service:)

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -109,7 +192,9 @@ protected function addContextualLinks(&$output, $block_type = 'block') {
+      if (function_exists('views_add_contextual_links')) {
+        views_add_contextual_links($output, $block_type, $this->view, $this->displayID);
+      }

hmm dunno about that:/
why dont just do the same with t()? add a views_add_contextual_links function that does nothing at all

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/Block/ViewsBlockTest.phpundefined
@@ -0,0 +1,209 @@
+    parent::setUp(); // TODO: Change the autogenerated stub

needs removal

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -87,6 +128,46 @@ public function build() {
+    $settings = parent::settings();
...
+    parent::blockForm($form, $form_state);
...
+    parent::blockValidate($form, $form_state);
...
+    parent::blockSubmit($form, $form_state);

These are not required as @xjm suggestion in #34

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -166,15 +220,133 @@ public function submitOptionsForm(&$form, &$form_state) {
+      if (!$enabled) {

Can we use empty() or isset() instead here?

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -166,15 +220,133 @@ public function submitOptionsForm(&$form, &$form_state) {
+   * Allows to change the display settings right before executing the block.

Needs a param, or can it use an {@inheritdoc}, not sure.

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -40,15 +43,40 @@ class ViewsBlock extends BlockBase {
-    $this->view = views_get_view($name);

thank you!!

this will fix the phpunit test

This patch looks great, and works nicely. You just forgot to call the parent in ViewsBlock::form, so I added that while I was testing and peace is restored once more.

I will RTBC once it comes back ok.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
@@ -155,6 +193,22 @@ public function buildOptionsForm(&$form, &$form_state) {
+        $allow = array_filter($this->getOption('allow'));
+        $form['allow'] = array(
+          '#type' => 'checkboxes',
+          '#default_value' => $allow,

Should this use array_keys(array_filter(..))?

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
@@ -166,15 +220,133 @@ public function submitOptionsForm(&$form, &$form_state) {
+  public function preBlockBuild(ViewsBlock $block) {

Is this method still needed?

Added a check for invalid display.

Let's wait for @xjm or @tim.plunkett

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -40,15 +43,40 @@ class ViewsBlock extends BlockBase {
+    $views = $storage_controller->load(array($name));

This can now just be $view = $storage_controller->load($name);

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -74,6 +103,20 @@ public function form($form, &$form_state) {
+      if ($config['more_link'] === 0) {
...
+      elseif ($config['more_link'] === 1) {

The === scares me, what if its '0' or '1' from CMI?

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/ViewsBlockTest.phpundefined
@@ -59,7 +59,7 @@ public function testGenerateBlockInstanceID() {
-    $views_block = new ViewsBlock(array(), $plugin_id, $plugin_definition);
+    $views_block = new ViewsBlock(array(), $plugin_id, $plugin_definition, $this->container->get('views.executable'), $this->container->get('plugin.manager.entity')->getStorageController('view'));

Now you should be able to do ViewsBlock::create($this->container, array(), $plugin_id, $plugin_definition);

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/Block/ViewsBlockTest.phpundefined
@@ -0,0 +1,208 @@
+}
+
+}

This looks odd.

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/Block/ViewsBlockTest.phpundefined
@@ -0,0 +1,208 @@
+namespace Drupal\views\Tests\Plugin\Block {

Oh, I just missed this curly brace, which throws off the indentation.

Interdiff looks good, thanks!

We need an issue summary update as we are at rtbc but the first time of the issue summary is:

Note: we do not yet have consensus on which direction to take in this issue.

:D

Updated issue summary.

I love PHPUnit tests as much as anyone but we also should be testing that the new form elements appear and work as expected though the Block UI. Also reading the issue summary can we confirm that we've tested the ability for contrib to extend? I would have expected a test module to do this...

Re #59 "Is this method still needed?":

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
@@ -166,15 +220,133 @@ public function submitOptionsForm(&$form, &$form_state) {
+  /**
+   * Allows to change the display settings right before executing the block.
+   */
+  public function preBlockBuild(ViewsBlock $block) {

I think that method was not called, but actually, it would make sense to add that back and move these from ViewsBlock::build() in there:

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php
@@ -74,6 +102,20 @@ public function form($form, &$form_state) {
+    $config = $this->getConfig();
+    if ($config['items_per_page'] !== 'none') {
+      $this->view->setItemsPerPage($config['items_per_page']);
+    }
+
+    if ($config['more_link'] !== 'none') {
+      if ((int) $config['more_link'] == 0) {
+        $this->view->display_handler->setOption('use_more', FALSE);
+      }
+      elseif ((int) $config['more_link'] == 1) {
+        $this->view->display_handler->setOption('use_more', TRUE);
+      }
+    }
+

This way all overrides related code would be in display plugin, right?

Also. Since we're purportedly doing all this for the content author UX, let's make sure we meet that goal.

I RTBC'd this before, and manually tested it.
The last patch is a nice improvement in the usability.

Thanks @dawehner and @olli!

Display more link setting didn't work for me. I had 6 articles, a views block set to show 10. I configure the block to show 5 (this works) and to show the more link. No more link was shown

UI review:

I think these added settings should be shown below the 'region' select list. One of the ways to get to this page is through 'Place block' link on the Blocks overview page. The region select list is how you place a block. Of course when you click to configure from a contextual link you might well come for the two settings added here, but then it's only 1 select list that you have to skip over.

Also, do we have to communicate that you're overriding things? I'm not sure about the usefulness of having these in their own details section.

I don't understand the 'override items per page' and 'override more link' as the default values in the select lists. What are these supposed to communicate? Is this what you often see phrased as "-- select a value -- ?

It's also not clear what the current values are.

@yoroy, you tested the wrong patch.

I did :-\

Still leaves to review:

- wether these need their own details section
- ordering of all ui elements. I think these should come below the region select list

Does my more link not show because I don't have a page defined for this view? Then I shouldn't be able to select it.

I think that makes most sense yeah.
At the very least lets not have the non-editable machine name sit in between two actionable items.

@dawehner shouldn't have RTBC'd, but I was about to anyway.
Thanks @yoroy!

Ahh, yes. Thanks for killing the leftover tests.

+1 RTBC

very glad we're adding these. looks like it should fit in nicely with #2025649: Add title-related methods to BlockPluginInterface to help clarify and resolve many issues, which i'm trying to get moving again.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -155,6 +191,22 @@ public function buildOptionsForm(&$form, &$form_state) {
+        $options = array(
+          'items_per_page' => t('Items per page'),
+          'more_link' => t('More link'),
+        );

I thought the "More link" was not going to be configurable from the block settings... looking at BlockForm method in the same class we only support items_per_page. Manually testing has confirmed the we need to remove the more link option here...

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -523,7 +523,7 @@ protected function defineOptions() {
       'use_more_always' => array(
-        'default' => FALSE,
+        'default' => TRUE,
         'bool' => TRUE,
       ),

Why we are making this change?

+++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.phpundefined
@@ -127,6 +127,25 @@ public function testViewsBlockForm() {
+    // Tests the override capability of items per page and the more link.

This comment is wrong.

+++ b/core/modules/block/tests/lib/Drupal/block/Tests/Plugin/views/display/BlockTest.phpundefined
@@ -0,0 +1,106 @@
+   * Test the build method with no overriding.

Tests

+++ b/core/modules/block/tests/lib/Drupal/block/Tests/Plugin/views/display/BlockTest.phpundefined
@@ -0,0 +1,106 @@
+  public function testBuildNoOverride() {
...
+
+    $this->assertFalse((bool) $this->blockDisplay->getOption('use_more'));

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -462,7 +462,7 @@ protected function defineOptions() {
-          'use_more_always' => TRUE,
+          'use_more_always' => FALSE,
           'use_more_text' => TRUE,

@@ -523,7 +523,7 @@ protected function defineOptions() {
-        'default' => FALSE,
+        'default' => TRUE,

I guess we don't want this anymore?

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -43,10 +47,33 @@ protected function defineOptions() {
+        'items_per_page' => array('default' => TRUE),

If we want this to default to have the checkbox checked as default, this would need to be 'items_per_page' instead of TRUE. This would be stored as an associative array anyway. or you could make the change in buildOptionsForm?

Otherwise, code looks good, and manual testing was good. That's all I found.

I think you just want: 'items_per_page' => array('default' => 'items_per_page'), instead, otherwise the checkbox will still not be checked by default.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -87,6 +114,15 @@ public function optionsSummary(&$categories, &$options) {
+    $allow = $this->getOption('allow');
+    $filtered_allow = array_filter($allow);

This could be a oneliner

+++ b/core/modules/block/tests/lib/Drupal/block/Tests/Plugin/views/display/BlockTest.phpundefined
@@ -0,0 +1,104 @@
+   * @var \PHPUnit_Framework_MockObject_MockObject
...
+   * @var \PHPUnit_Framework_MockObject_MockObject
...
+   * @var \Drupal\block\Plugin\views\display\Block|\PHPUnit_Framework_MockObject_MockObject

I know this is technically correct, but I'm pretty sure we just use the non-mocked class name in typehints...

+++ b/core/modules/block/tests/lib/Drupal/block/Tests/Plugin/views/display/BlockTest.phpundefined
@@ -0,0 +1,104 @@
+   * Test the build method with no overriding.

Tests

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -87,6 +117,41 @@ public function build() {
+    $this->view->setDisplay($this->displayID);
...
+    $this->view->setDisplay($this->displayID);
...
+    $this->view->setDisplay($this->displayID);

Do we have to call this all three times?

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/Block/ViewsBlockTest.phpundefined
@@ -0,0 +1,168 @@
+   * @var \PHPUnit_Framework_MockObject_MockObject
...
+   * @var \PHPUnit_Framework_MockObject_MockObject
...
+   * @var \PHPUnit_Framework_MockObject_MockObject
...
+   * @var \PHPUnit_Framework_MockObject_MockObject

Same as before

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/Block/ViewsBlockTest.phpundefined
@@ -0,0 +1,168 @@
+    parent::setUp(); // TODO: Change the autogenerated stub

:)

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/Block/ViewsBlockTest.phpundefined
@@ -0,0 +1,168 @@
+   * Test the build method.
...
+   * Test the build method with a failed execution.

Tests

RTBC if green

#110: vdc-1957346-110.patch queued for re-testing.

Hmm, we can't really just do setDisplay() in the constructor as it blows up when the plugin ID is invalid, and still tries to call methods on the wrong display, which the previous if () calls took care of.

Class property is missing from interdiff but in the patch. See interdiff for what I meant in #105 and #107.

Ah, very good point.
Thanks @dawehner and @damiankloip!

Committed 9dd98ea and pushed to 8.x. Thanks!