Allow the block category for Views block displays to be edited

You have to adapt the schema as well. The rest looks perfect. It seems to be that webchick requires to use the base table as the default value?

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
@@ -173,6 +180,14 @@ public function buildOptionsForm(&$form, &$form_state) {
+          '#description' => t('This will appear as the category of this block in administer >> structure >> blocks.'),

I think this should be something like:
"... as the category of this block on the blocks placement page" (or whatever that page is called).

1. +++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
   @@ -108,12 +109,18 @@ public function optionsSummary(&$categories, &$options) {
   +      'value' => views_ui_truncate($block_category, 24),
   

   Using a views_ui method here bugs me. That is not the fault of this patch though, I might make an issue to sort that out.

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsBlock.php
   @@ -90,6 +90,7 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
   +          $category = $display->getOption('block_category');
   
   @@ -100,6 +101,7 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
   +            'category' => $category,
   

   Might as well just not assign the $category variable?

I agree with #3, but maybe tweaked; something like 'The category this block will appear under on the blocks placement page' ?

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
@@ -46,7 +47,10 @@ protected function defineOptions() {
+    $base_tables = Views::viewsData()->fetchBaseTables();
+    $base_table = $this->view->storage->get('base_table');
+    $block_category = isset($base_tables[$base_table]['title']) ? $base_tables[$base_table]['title'] : t('Views');
+    $options['block_category'] = array('default' => $block_category, 'translatable' => TRUE);

You are a lucky guy that this does not break anything. My previous attempts to use configuration in defineOptions all went into the problem that at some point defineOptions is called again. I am feared that this could bite us at some point.

Additional I think we should not use translated values here as default values.

1. +++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
   @@ -173,6 +184,14 @@ public function buildOptionsForm(&$form, &$form_state) {
   +          '#description' => t('The category this block will appear under on the <a href="@href">blocks placement page</a>.', array('@href' => 'admin/structure/blocks')),
   

   The link doesn't work, it takes me to admin/structure/views/view/archive/edit/admin/structure/blocks.

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsBlock.php
   @@ -100,6 +100,7 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
   +            'category' => $display->getOption('block_category'),
   

   I think we need to sanitize the category if it's user input.

+++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.php
@@ -47,6 +47,43 @@ protected function setUp() {
+    $elements = $this->xpath('//details[@id=:id]//ul[contains(@class, "block-list")]/li[contains(@class, :li_class)]/a[contains(@href, :href) and text()=:text]', $arguments);
...
+    $elements = $this->xpath('//details[@id=:id]//ul[contains(@class, "block-list")]/li[contains(@class, :li_class)]/a[contains(@href, :href) and text()=:text]', $arguments);

Is it just me that we test the actual produced HTML? I don't know but it feels better to not add everything into the select chain?

+++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.php
@@ -52,20 +52,30 @@ protected function setUp() {
+      ':href' => 'admin/structure/block/add/views_block%3A' . $edit['id'] . '-block_1/stark',

Is there already a follow up to fix the ":" in URLs problems?

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
@@ -188,8 +189,8 @@ public function buildOptionsForm(&$form, &$form_state) {
+          '#description' => t('The category this block will appear under on the <a href="@href">blocks placement page</a>.', array('@href' => '/admin/structure/blocks')),

I think the standard is to use url() in these kinds of situations. Since we're still using t() also, I think that would be OK, even nowadays.

Patch no longer applies

Re #16: Sorry for not being more precise. Embedding the <a> tag in the string is correct indeed. But for the t() placeholder I think it makes sense to use url('admin/structure/blocks') instead of embedding the path directly. That's what's done throughout core.

Right, of course I was totally arguing on a technical level and not for mere consistency/nit-picking-reasons... ;-P

If this was RTBC before, it certainly is now. Unless bot disagrees again.

Wow, nice debugging!

Still looks good.

Sorry, found two small bugs while simplytesting.

1. +++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
   @@ -173,6 +182,14 @@ public function buildOptionsForm(&$form, &$form_state) {
   +          '#description' => t('The category this block will appear under on the <a href="@href">blocks placement page</a>.', array('@href' => url('admin/structure/blocks'))),
   

   The path is admin/structure/block.

2. +++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
   @@ -173,6 +182,14 @@ public function buildOptionsForm(&$form, &$form_state) {
   +          '#default_value' => String::checkPlain($this->getOption('block_category')),
   

   String::checkPlain is not needed here and would cause double escaping.

3. Should we add block_category to views.display.schema.yml?

We totally should add the schema, see my review in #2 :)

No longer applies. :(

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
@@ -2662,6 +2666,12 @@ public function validate() {
   /**
+   * Reacts on adding a display.
+   */
+  public function newDisplay() {
+  }

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewEditFormController.php
@@ -790,7 +791,8 @@ public function submitDisplayAdd($form, &$form_state) {
-    $display_id = $view->addDisplay($display_type);
+    $display = $view->newDisplay($display_type);

It is not clear what addDisplay() vs. newDisplay() is. I approached bikeshedding this with Tim a bit, but he pointed out that this confusion is a pre-existing condition (newDisplay is defined on the View object, this is simply re-propogating it here where it's needed). So let's get a follow-up to fix this from a more holistic POV, and also fix the docs so they refer to where this method is properly documented.

Otherwise, this looks good code-wise. Haven't had a chance to manually test yet.

That is fine for now.

Ok, now played around with this as a user. Overall, it worked great! Here's a walkthrough:

I LOVE this, since it picks a smart default that'll work in 90% of cases, most likely:

However...

A text field for this isn't going to fly; it opens a site up to data integrity issues. Since we lack a "select or add one" pattern in core to employ here, we should fall back to autocomplete as a workaround. Needs work for that.

I also found one other weird thing:

I've no idea why we are exposing this kind of implementation detail. We don't prefix other blocks with "Module:" or "Custom:" From talking to Tim, this is a historical artifact from Views D7 and below, since often you'd end up in a situation where there was both a "Core" Recent comments block and also a "Views" Recent comments block, and you needed to differentiate between them. However, with Views in core, and with an initiative well underway to try and convert all of those crappy listing blocks to Views, I don't think this makes sense anymore. However, it wasn't introduced in this patch, so marking "Needs followup" for that.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.php
@@ -108,4 +108,18 @@ protected function t($string, array $args = array(), array $options = array()) {
+    $categories = array_unique(array_values(array_map(function ($definition) {
+      return $definition['category'];
+    }, $this->getDefinitions())));

Oh I like this part.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.php
@@ -108,4 +108,18 @@ protected function t($string, array $args = array(), array $options = array()) {
+    natcasesort($categories);

That function sounds a bit like net cheese in german.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
@@ -768,6 +768,10 @@ public function getPath() {
+
+    if ($this->getOption('link_display') == 'custom_url' && $link_url = $this->getOption('link_url')) {
+      return $link_url;
+    }

Oh, cr.. #32. This was for another issue, sorry!

Nice work @tim.plunkett!

1. +++ b/core/modules/block/lib/Drupal/block/Controller/CategoryAutocompleteController.php
   @@ -0,0 +1,67 @@
   +      if (strpos($category, $typed_category) === 0) {
   

   Should we use stripos instead?

2. +++ b/core/modules/block/lib/Drupal/block/Controller/CategoryAutocompleteController.php
   @@ -0,0 +1,67 @@
   +        $matches[$category] = $category;
   

   Needs a checkplain for the value.

As #37 is addressed. Screenshot is in #38 and we have nice test coverage. Unable to find aything in code review so putting back to RTBC.

Perfect!!

Committed and pushed to 8.x. Still need a follow-up for the "Views:" string. I'd create it myself, but I'm not sure I could adequately describe where that code is and what it's doing.

Created that follow up; I should that should be adequate, and very simple. Unless I'm missing something :) #2086447: Remove 'View:' prefix from views block derivative admin labels