Download & Extend
Issues

Refactor viewSpecialBlocks architecture once blocks are plugins

Posted by tim.plunkett on January 3, 2013 at 7:41pm
Project:Drupal core
Version:8.x-dev
Component:views_ui.module
Category:task
Priority:normal
Assigned:Unassigned
Status:needs work
Issue tags:Block plugins, Blocks-Layouts, VDC

Issue Summary

In #1535868: Change notice: Convert all blocks into plugins, we did a straight port of the Views block integration.
With plugins and derivatives and all the new stuff, we'll want to revamp our code.

Specific things to target:

  • The naming of \Drupal\views\Plugin\views\display\DisplayPluginBase::viewSpecialBlocks()
  • The $type parameter of viewSpecialBlocks(), which is currently always 'exp' for exposed blocks
  • Maybe make views_add_block_contextual_links() a method, or find a way to move that to contextual.module
Login or register to post comments

Comments

#1

Posted by xjm on January 3, 2013 at 7:42pm
Issue tags:+Blocks-Layouts

#2

Posted by xjm on January 4, 2013 at 5:24pm
Status:postponed» active

#3

Posted by dawehner on January 5, 2013 at 12:07am
Status:active» needs review

There really seems no use-case for anything else then exposed forms, and if someone needs it, nothing prevents him from doing that in any kind of other custom code.

AttachmentSizeStatusTest resultOperations
drupal-1879256-3.patch3.3 KBIdlePASSED: [[SimpleTest]]: [MySQL] 50,479 pass(es).View details | Re-test

#4

Posted by tim.plunkett on January 5, 2013 at 12:18am

There's still a reference to special_block above views_add_block_contextual_links()

#5

Posted by dawehner on January 5, 2013 at 12:26am

Good point, thank you!

I would be really surprised if there would be test-coverage for these kind of blocks.

AttachmentSizeStatusTest resultOperations
drupal-1879256-5.patch3.74 KBIdlePASSED: [[SimpleTest]]: [MySQL] 50,603 pass(es).View details | Re-test
interdiff.txt638 bytesIgnored: Check issue status.NoneNone

#6

Posted by tim.plunkett on January 5, 2013 at 12:56am

I think the only thing left is #3 from the OP, making views_add_block_contextual_links() a method on ViewsBlock

#7

Posted by dawehner on January 5, 2013 at 11:07am

Oh totally forgot that, did I talked about working at night before ;)

AttachmentSizeStatusTest resultOperations
drupal-1879256-7.patch6.61 KBIdlePASSED: [[SimpleTest]]: [MySQL] 50,489 pass(es).View details | Re-test
interdiff.txt6.61 KBIgnored: Check issue status.NoneNone

#8

Posted by xjm on January 13, 2013 at 8:40pm
Issue tags:+Block plugins

#9

Posted by tim.plunkett on January 25, 2013 at 2:59am

Rerolled.

AttachmentSizeStatusTest resultOperations
vdc-1879256-9.patch6.61 KBIdlePASSED: [[SimpleTest]]: [MySQL] 49,268 pass(es).View details | Re-test

#10

Posted by dawehner on January 25, 2013 at 12:55pm

Fixed some small bit.

As you didn't made any other suggestions is that RTBC?

AttachmentSizeStatusTest resultOperations
interdiff.txt735 bytesIgnored: Check issue status.NoneNone
drupal-1879256-10.patch6.62 KBIdlePASSED: [[SimpleTest]]: [MySQL] 49,255 pass(es).View details | Re-test

#11

Posted by tim.plunkett on January 25, 2013 at 2:40pm
Status:needs review» reviewed & tested by the community

Yes, I believe it is!

#12

Posted by catch on February 4, 2013 at 2:40pm
Status:reviewed & tested by the community» needs review

+++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
@@ -78,9 +78,35 @@ public function build() {
+    // Do not add contextual links to an empty block.
+    if (!empty($output)) {
+      // Contextual links only work on blocks whose content is a renderable
+      // array, so if the block contains a string of already-rendered markup,
+      // convert it to an array.
+      if (is_string($output)) {

I'm surprised that Views has to check this for its own blocks, can they really be both?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -2640,20 +2640,21 @@ public function getSpecialBlocks() {
+    if (arg(0) == 'admin' && arg(1) == 'structure' && arg(2) == 'views') {
+      return;

Couldn't this use menu_get_item()?

#13

Posted by dawehner on February 4, 2013 at 10:22pm

Good points, here is a new patch.

According to b261004a4825e1b6eb1398e849ef3729acd4de97 this went in, so this just changes this specific parts of it.

AttachmentSizeStatusTest resultOperations
drupal-1879256-13.patch2.45 KBIdleFAILED: [[SimpleTest]]: [MySQL] 49,182 pass(es), 20 fail(s), and 24 exception(s).View details | Re-test

#14

Posted by tim.plunkett on February 4, 2013 at 11:36pm
Status:needs review» reviewed & tested by the community

Oh hell yes, this patch removes two of my least favorite parts of Views :)

#15

Posted by System Message on February 4, 2013 at 11:48pm
Status:reviewed & tested by the community» needs work

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

#16

Posted by tim.plunkett on February 5, 2013 at 1:37am
Status:needs work» needs review

Ahh, when the drupal_render() is removed, it's not built fully.
This "fixes" it, but seems like overkill (those methods have an isset at the top of each in case they've run before, so its not too bad).

@dawehner, I hope you have better ideas :)

AttachmentSizeStatusTest resultOperations
vdc-1879256-16.patch3.51 KBIdleFAILED: [[SimpleTest]]: [MySQL] 52,739 pass(es), 8 fail(s), and 0 exception(s).View details | Re-test
interdiff.txt1.07 KBIgnored: Check issue status.NoneNone

#17

Posted by System Message on February 5, 2013 at 2:35am
Status:needs review» needs work

The last submitted patch, vdc-1879256-16.patch, failed testing.

#18

Posted by dawehner on March 9, 2013 at 3:21pm
Status:needs work» needs review

#16: vdc-1879256-16.patch queued for re-testing.

#19

Posted by System Message on March 9, 2013 at 4:30pm
Status:needs review» needs work

The last submitted patch, vdc-1879256-16.patch, failed testing.