On #1938062: Convert the recent_comments block to a view it came up, that people should maybe not have access to the full view settings, but change just some certain bits of it,
in order to maintain the UX of Drupal 7 and possible access problems when dealing with the full views UI.

The proposed overrideable settings are:

  • items per page
  • link to the view / should there be a more link

Therefore the block display plugin can alter the settings of a block view, so contrib could provide more then the two mentioned points above.

Files: 
CommentFileSizeAuthor
#115 1957346-115.patch23.01 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,507 pass(es).
[ View ]
#115 interdiff-1957346-115.txt2.45 KBdamiankloip
#110 vdc-1957346-110.patch22.68 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,566 pass(es), 3 fail(s), and 1 exception(s).
[ View ]
#110 interdiff.txt5.97 KBdawehner
#108 vdc-1957346-108.patch22.27 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,487 pass(es).
[ View ]
#108 interdiff.txt1014 bytesdawehner
#106 vdc-1957346-106.patch22.33 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,545 pass(es).
[ View ]
#106 interdiff.txt599 bytesdawehner
#104 vdc-1957346-104.patch22.3 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,470 pass(es).
[ View ]
#104 interdiff.txt2.82 KBdawehner
#102 vdc-1957346-102.patch22.46 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,433 pass(es).
[ View ]
#102 interdiff.txt1.69 KBdawehner
#98 interdiff.txt1.59 KBdawehner
#97 vdc-1957346-97.patch22.23 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,346 pass(es).
[ View ]
#92 vdc-1957346-92.patch22.49 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,135 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#92 interdiff.txt4.3 KBdawehner
#89 1957346-select-list-defaults.png55.58 KByoroy
#89 1957346-ui-cleanup.png167.82 KByoroy
#87 vdc-1957346-87.patch23.76 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,315 pass(es).
[ View ]
#87 interdiff.txt1.71 KBdawehner
#86 vdc-1957346-86.patch23.51 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,314 pass(es).
[ View ]
#86 interdiff.txt839 bytesdawehner
#84 vdc-1957346-84.patch23.52 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,116 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#84 interdiff.txt737 bytesdawehner
#80 vdc-1957346-80.patch23.31 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found.
[ View ]
#77 vdc-1957346-77.patch20.95 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,169 pass(es).
[ View ]
#77 interdiff.txt1.78 KBdawehner
#72 vdc-1957346-71.patch19.16 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,883 pass(es).
[ View ]
#72 interdiff.txt2.76 KBdawehner
#67 vdc-1957346-67.patch19.3 KBolli
PASSED: [[SimpleTest]]: [MySQL] 56,858 pass(es).
[ View ]
#67 interdiff.txt834 bytesolli
#65 vdc-1957346-65.patch19.16 KBolli
FAILED: [[SimpleTest]]: [MySQL] 56,357 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#65 interdiff.txt709 bytesolli
#63 vdc-1957346-63.patch19.12 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,792 pass(es), 3 fail(s), and 1 exception(s).
[ View ]
#63 interdiff.txt1.5 KBdawehner
#61 vdc-1957346-60.patch18.4 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,349 pass(es), 30 fail(s), and 10 exception(s).
[ View ]
#61 interdiff.txt2.43 KBdawehner
#60 vdc-1957346-60.patch18.4 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,841 pass(es), 7 fail(s), and 10 exception(s).
[ View ]
#60 interdiff.txt2.43 KBdawehner
#58 1957346-58.patch19.01 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 56,853 pass(es), 4 fail(s), and 4 exception(s).
[ View ]
#58 interdiff-1957346-58.txt665 bytesdamiankloip
#57 vdc-1957346-57.patch19.05 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,634 pass(es), 7 fail(s), and 10 exception(s).
[ View ]
#57 interdiff.txt1.17 KBdawehner
#55 drupal-1957346-55.patch18.99 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,585 pass(es), 28 fail(s), and 11 exception(s).
[ View ]
#55 interdiff.txt3.13 KBdawehner
#53 vdc-1957346-53.patch18.94 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 56,670 pass(es), 11 fail(s), and 11 exception(s).
[ View ]
#53 interdiff.txt841 bytesParisLiakos
#49 interdiff.txt6.4 KBdawehner
#49 vdc-1957346-49.patch18.94 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,987 pass(es), 11 fail(s), and 17 exception(s).
[ View ]
#45 override.png8.99 KBdawehner
#45 vdc-1969388-15.patch19.39 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,480 pass(es), 12 fail(s), and 5 exception(s).
[ View ]
#43 interdiff.txt1.9 KBdawehner
#43 drupal-1957346-43.patch19.82 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,493 pass(es), 10 fail(s), and 5 exception(s).
[ View ]
#40 vdc-1957346-40.patch19.98 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,240 pass(es), 10 fail(s), and 5 exception(s).
[ View ]
#40 interdiff.txt584 bytesdawehner
#37 vdc-1957346-37.patch13.74 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,057 pass(es), 7 fail(s), and 17 exception(s).
[ View ]
#37 vdc-1957346-37.patch19.99 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,995 pass(es), 10 fail(s), and 17 exception(s).
[ View ]
#37 interdiff.txt8.89 KBdawehner
#35 1957346-35.patch9.35 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 58,399 pass(es), 6 fail(s), and 6 exception(s).
[ View ]
#35 interdiff.txt5 KBjibran
#31 drupal-1957346-31.patch7.57 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,764 pass(es), 4 fail(s), and 4 exception(s).
[ View ]
#17 blocks-views-override.png82.01 KBBojhan
#17 overrideblock-view.png146.71 KBBojhan
#15 drupal-1957346-13.patch7.58 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1957346-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 drupal-1957346-1.patch4.85 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 54,910 pass(es).
[ View ]
#1 views.png29.86 KBdawehner

Comments

dawehner’s picture

StatusFileSize
new29.86 KB
new4.85 KB
PASSED: [[SimpleTest]]: [MySQL] 54,910 pass(es).
[ View ]

Here is a start for the views part of it, though we should certainly discuss how it should look on the block configuration page.

My idea was to group the settings into: pager related settings, link related settings and exposed_form and finally a fields override.
All of these groups could be put together in a vertical tab.

The title override would probably take care about the label #access setting.

Bojhan’s picture

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?

xjm’s picture

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.
dawehner’s picture

As on the title we should certainly try to allow the user to choose whether he actually want's to override it.

YesCT’s picture

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.

Bojhan’s picture

@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.

YesCT’s picture

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.

xjm’s picture

@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.

xjm’s picture

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.

xjm’s picture

Component:block.module» views.module

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

YesCT’s picture

@xjm thanks. :)

tim.plunkett’s picture

Category:feature» task

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.

webchick’s picture

Category:task» feature

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.

webchick’s picture

Category:feature» task

Oops sorry, cross-post.

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new7.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1957346-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reduced the items to items per page and more link. Additional I talked with tim and we agreed that the display block plugin should define the available options, so this passes further all the block related functions.

Status:Needs review» Needs work

The last submitted patch, drupal-1957346-13.patch, failed testing.

Bojhan’s picture

StatusFileSize
new146.71 KB
new82.01 KB

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.
overrideblock-view.png

A pointer that changing this is fine, but it will leave Blocks X, Block Y unaffected.
blocks-views-override.png

webchick’s picture

"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.

tim.plunkett’s picture

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.

Bojhan’s picture

@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.

peteruithoven’s picture

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.

dawehner’s picture

I guess such an improvement would be too complicated for drupal core, though maybe views should expose a way that contrib can extend this functionality later.

peteruithoven’s picture

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.

tim.plunkett’s picture

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.

peteruithoven’s picture

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

tim.plunkett’s picture

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

I think adding links, as above, will scale.

jibran’s picture

Status:Needs work» Needs review
Issue tags:-Needs issue summary update, -VDC

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

Status:Needs review» Needs work
Issue tags:+Needs issue summary update, +VDC

The last submitted patch, drupal-1957346-13.patch, failed testing.

xjm’s picture

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.

xjm’s picture

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. ;)

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new7.57 KB
FAILED: [[SimpleTest]]: [MySQL] 56,764 pass(es), 4 fail(s), and 4 exception(s).
[ View ]

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

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

Just a rerole.

The last submitted patch, drupal-1957346-31.patch, failed testing.

Status:Needs review» Needs work
Issue tags:-Needs issue summary update, -VDC

The last submitted patch, drupal-1957346-31.patch, failed testing.

xjm’s picture

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.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new5 KB
new9.35 KB
FAILED: [[SimpleTest]]: [MySQL] 58,399 pass(es), 6 fail(s), and 6 exception(s).
[ View ]

Reroll and some doc and form fixes.

Status:Needs review» Needs work

The last submitted patch, 1957346-35.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new8.89 KB
new19.99 KB
FAILED: [[SimpleTest]]: [MySQL] 57,995 pass(es), 10 fail(s), and 17 exception(s).
[ View ]
new13.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,057 pass(es), 7 fail(s), and 17 exception(s).
[ View ]

Worked on getting some actual UI, not failing settings() method and some proper unit tests for the block plugin in general.

Status:Needs review» Needs work

The last submitted patch, vdc-1957346-37.patch, failed testing.

jibran’s picture

+++ 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.

dawehner’s picture

Status:Needs work» Needs review
Issue tags:+phpunit
StatusFileSize
new584 bytes
new19.98 KB
FAILED: [[SimpleTest]]: [MySQL] 58,240 pass(es), 10 fail(s), and 5 exception(s).
[ View ]

Well, but t() is stilled called from everywhere so we have to replace it.

Status:Needs review» Needs work

The last submitted patch, vdc-1957346-40.patch, failed testing.

ParisLiakos’s picture

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

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new19.82 KB
FAILED: [[SimpleTest]]: [MySQL] 58,493 pass(es), 10 fail(s), and 5 exception(s).
[ View ]
new1.9 KB

Yeah this is a good idea.

Status:Needs review» Needs work

The last submitted patch, drupal-1957346-43.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new19.39 KB
FAILED: [[SimpleTest]]: [MySQL] 58,480 pass(es), 12 fail(s), and 5 exception(s).
[ View ]
new8.99 KB

Let's upload a screenshot, let's hope the WLAN allowed the upload.
override.png

jibran’s picture

+++ 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

Status:Needs review» Needs work

The last submitted patch, vdc-1969388-15.patch, failed testing.

damiankloip’s picture

+++ 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!!

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new18.94 KB
FAILED: [[SimpleTest]]: [MySQL] 57,987 pass(es), 11 fail(s), and 17 exception(s).
[ View ]
new6.4 KB

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

The problem is , this method is coming from the block so it is not defined as the parent.

This patch fixes the reviews as well but one of the php unit tests don't work yet.

Status:Needs review» Needs work
Issue tags:-Needs issue summary update, -phpunit, -VDC

The last submitted patch, vdc-1957346-49.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+Needs issue summary update, +phpunit, +VDC

The last submitted patch, vdc-1957346-49.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new841 bytes
new18.94 KB
FAILED: [[SimpleTest]]: [MySQL] 56,670 pass(es), 11 fail(s), and 11 exception(s).
[ View ]

this will fix the phpunit test

Status:Needs review» Needs work

The last submitted patch, vdc-1957346-53.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new3.13 KB
new18.99 KB
FAILED: [[SimpleTest]]: [MySQL] 58,585 pass(es), 28 fail(s), and 11 exception(s).
[ View ]

Let's fix some of those.

Status:Needs review» Needs work

The last submitted patch, drupal-1957346-55.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.17 KB
new19.05 KB
FAILED: [[SimpleTest]]: [MySQL] 56,634 pass(es), 7 fail(s), and 10 exception(s).
[ View ]

I hope this should fix as many as possible.

damiankloip’s picture

StatusFileSize
new665 bytes
new19.01 KB
FAILED: [[SimpleTest]]: [MySQL] 56,853 pass(es), 4 fail(s), and 4 exception(s).
[ View ]

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.

olli’s picture

+++ 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?

dawehner’s picture

StatusFileSize
new2.43 KB
new18.4 KB
FAILED: [[SimpleTest]]: [MySQL] 56,841 pass(es), 7 fail(s), and 10 exception(s).
[ View ]
+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -90,6 +90,7 @@ public function access() {
+    $form = parent::form($form, $form_state);
     // Set the default label to '' so the views internal title is used.

jess told me that the concept of the block interface is that just the plugin will care about it, so don't call the parent.

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

But then it again does not match with the option definition etc. I think it is fine to go with it like it is now.

dawehner’s picture

StatusFileSize
new2.43 KB
new18.4 KB
FAILED: [[SimpleTest]]: [MySQL] 56,349 pass(es), 30 fail(s), and 10 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, vdc-1957346-60.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.5 KB
new19.12 KB
FAILED: [[SimpleTest]]: [MySQL] 56,792 pass(es), 3 fail(s), and 1 exception(s).
[ View ]

Damian, i was so wrong.

Status:Needs review» Needs work

The last submitted patch, vdc-1957346-63.patch, failed testing.

olli’s picture

Status:Needs work» Needs review
StatusFileSize
new709 bytes
new19.16 KB
FAILED: [[SimpleTest]]: [MySQL] 56,357 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Added a check for invalid display.

Status:Needs review» Needs work

The last submitted patch, vdc-1957346-65.patch, failed testing.

olli’s picture

Status:Needs work» Needs review
StatusFileSize
new834 bytes
new19.3 KB
PASSED: [[SimpleTest]]: [MySQL] 56,858 pass(es).
[ View ]
dawehner’s picture

+1 for 65 and 67, though I hope this problem will never appear once we automatically remove the block entries.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community
jibran’s picture

Status:Reviewed & tested by the community» Needs review

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

tim.plunkett’s picture

Status:Needs review» Needs work
+++ 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.

dawehner’s picture

StatusFileSize
new2.76 KB
new19.16 KB
PASSED: [[SimpleTest]]: [MySQL] 56,883 pass(es).
[ View ]

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

Good point

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

Let's typecast for now.

This looks odd.

Do you have a better suggestion? This allows you to remove it really easy, once the workarounds for t()/views_add_contextual_links are in.

olli’s picture

Status:Needs work» Needs review
tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community
+++ 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!

alexpott’s picture

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

alexpott’s picture

Issue summary:View changes

Updated issue summary.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

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...

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.78 KB
new20.95 KB
PASSED: [[SimpleTest]]: [MySQL] 57,169 pass(es).
[ View ]

If you would want more options you would provide a different views display plugin, replacing the existing one provided by views.

Here is a UI test.

olli’s picture

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?

xjm’s picture

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

dawehner’s picture

StatusFileSize
new23.31 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found.
[ View ]

There is no need for manual testing, ... we have unit tests, so what should possible go wrong.

@olli
I agree with you, let move that bit.

Status:Needs review» Needs work
Issue tags:-Needs usability review, -Needs issue summary update, -phpunit, -Needs manual testing, -VDC

The last submitted patch, vdc-1957346-80.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+Needs usability review, +Needs issue summary update, +phpunit, +Needs manual testing, +VDC

The last submitted patch, vdc-1957346-80.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new737 bytes
new23.52 KB
FAILED: [[SimpleTest]]: [MySQL] 57,116 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

urgs.

Status:Needs review» Needs work

The last submitted patch, vdc-1957346-84.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new839 bytes
new23.51 KB
PASSED: [[SimpleTest]]: [MySQL] 57,314 pass(es).
[ View ]

let's fix it.

dawehner’s picture

StatusFileSize
new1.71 KB
new23.76 KB
PASSED: [[SimpleTest]]: [MySQL] 57,315 pass(es).
[ View ]

Here are some suggestions for better UX.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs manual testing

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

Thanks @dawehner and @olli!

yoroy’s picture

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new167.82 KB
new55.58 KB

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.

tim.plunkett’s picture

Status:Needs work» Needs review

@yoroy, you tested the wrong patch.

yoroy’s picture

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.

dawehner’s picture

StatusFileSize
new4.3 KB
new22.49 KB
FAILED: [[SimpleTest]]: [MySQL] 57,135 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Okay let's remove the more link setting for now, as it is just less important then the more link.

yoroy’s picture

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’s picture

Status:Needs review» Reviewed & tested by the community

We can't at the moment, seriously.

tim.plunkett’s picture

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

Status:Reviewed & tested by the community» Needs work

The last submitted patch, vdc-1957346-92.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new22.23 KB
PASSED: [[SimpleTest]]: [MySQL] 57,346 pass(es).
[ View ]

Rage patches never work.

dawehner’s picture

StatusFileSize
new1.59 KB

Rage patches never work.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Ahh, yes. Thanks for killing the leftover tests.

sdboyer’s picture

+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.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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?

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.69 KB
new22.46 KB
PASSED: [[SimpleTest]]: [MySQL] 57,433 pass(es).
[ View ]

Why we are making this change?

This potentially fixed some issues yoroy had while testing, but in general this is a good idea, as it is faster: #2046531: Change use_more_always to default to TRUE

The main reason why we remove it from the patch is, because it potentially changes too much. What would you expect if you have configured views to look up that there are really more items, but there are not. ... We can potentially work on that in another issue, but this patch is mostly about unblocking all kind of conversions.

tim.plunkett’s picture

+++ 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?

dawehner’s picture

StatusFileSize
new2.82 KB
new22.3 KB
PASSED: [[SimpleTest]]: [MySQL] 57,470 pass(es).
[ View ]
damiankloip’s picture

Status:Needs review» Needs work
+++ 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.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new599 bytes
new22.33 KB
PASSED: [[SimpleTest]]: [MySQL] 57,545 pass(es).
[ View ]

There we go.

damiankloip’s picture

Status:Needs review» Needs work

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

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1014 bytes
new22.27 KB
PASSED: [[SimpleTest]]: [MySQL] 57,487 pass(es).
[ View ]

I as tired yesterday, so am I today, but maybe this is a good fix.

tim.plunkett’s picture

+++ 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

dawehner’s picture

StatusFileSize
new5.97 KB
new22.68 KB
FAILED: [[SimpleTest]]: [MySQL] 57,566 pass(es), 3 fail(s), and 1 exception(s).
[ View ]

Thanks for the review!

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

I simply won't fight this.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

RTBC if green

Status:Reviewed & tested by the community» Needs work
Issue tags:-Needs issue summary update, -phpunit, -VDC

The last submitted patch, vdc-1957346-110.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+Needs issue summary update, +phpunit, +VDC

The last submitted patch, vdc-1957346-110.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new2.45 KB
new23.01 KB
PASSED: [[SimpleTest]]: [MySQL] 57,507 pass(es).
[ View ]

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.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

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

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

updated the summary