Commit message

Issue #1998582 by damiankloip, brantwynn, dawehner, xjm, effulgentsia: Auto-generate machine_name for Views Blocks using [viewname]_[displayname] rather than forcing manual entry.

Problem/Motivation

Currently, users must type in a custom machine_name when placing a new Views block instance.

views-block-machine-name-manual.png

Normally, a block machine_name is generated from the label. This doesn't work for Views blocks since ViewsBlock:form() unsets this property. See: #1957276: Let users set the block instance title for Views blocks in the Block UI

Drupal should handle assigning the machine_name for the user, generating it based on the View name and the display that the block is using (ie: block_1).

Views sets up its own plugin portions of the block configuration form but assigning machine names is not handled by the plugin. This happens afterward, when the block system hardcodes this in BlockFormController:form()

...
/**
   * Overrides \Drupal\Core\Entity\EntityFormController::form().
   */
  public function form(array $form, array &$form_state) {
    $entity = $this->entity;
    $form['#tree'] = TRUE;
    $form['id'] = array(
      '#type' => 'value',
      '#value' => $entity->id(),
    );
    $form['settings'] = $entity->getPlugin()->form(array(), $form_state);

    $form['machine_name'] = array(
      '#type' => 'machine_name',
      '#title' => t('Machine name'),
      '#maxlength' => 64,
      '#description' => t('A unique name to save this block configuration. Must be alpha-numeric and be underscore separated.'),
      '#default_value' => $entity->id(),
      '#machine_name' => array(
        'exists' => 'block_load',
        'replace_pattern' => '[^a-z0-9_.]+',
        'source' => array('settings', 'label'),
      ),
      '#required' => TRUE,
      '#disabled' => !$entity->isNew(),
    );
...

Proposed resolution

Talking to xjm, effulgentsia and dawehner, we've determined a patch should provide a form_alter in views.module that gives Views this special ability to assign auto-generated machine name for its blocks.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

saltednut’s picture

Assigned: saltednut » Unassigned
Status: Active » Needs review
Issue tags: +VDC, +Block plugins
FileSize
3.51 KB

Here is our ugly little form alter for your review.

saltednut’s picture

My apologies, in my haste I've included code in the previous patch from another issue. Here is the proper patch!

dawehner’s picture

Issue tags: +Needs tests
+++ b/core/modules/views/views.moduleundefined
@@ -1817,3 +1817,21 @@ function views_cache_get($cid, $use_language = FALSE) {
+    $block_plugin = $form_state['build_info']['callback_object']->getEntity()->getPlugin();
+    list($plugin, $delta) = explode(':', $block_plugin->getPluginId());
+    list($name, $displayID) = explode('-', $delta, 2);

It seems to be good to document what the hell we are doing on this lines ...

+++ b/core/modules/views/views.moduleundefined
@@ -1817,3 +1817,21 @@ function views_cache_get($cid, $use_language = FALSE) {
+    $form['machine_name']['#default_value'] = 'views_block__' . $name . '_' . $displayID;

The codestyle requires $display_id instead of $displayID. I know $name is coming from a views snippet, but let's use $view_id to be consistent with the rest of the code.

+++ b/core/modules/views/views.moduleundefined
@@ -1817,3 +1817,21 @@ function views_cache_get($cid, $use_language = FALSE) {
+  } ¶

Ups missing space.

saltednut’s picture

Comments appended to and added for what we are doing.

I've also addressed the code style issues and removed the trailing whitespace.

damiankloip’s picture

@brantwynn, This patch is looking quite good. Do you want to add some tests? If not, I will :)

saltednut’s picture

@damiankloip Yes, if you have the bandwidth to add tests today, go for it. I probably won't be able to get back to it for a few days.

damiankloip’s picture

FileSize
1.33 KB
2.89 KB

Here are a couple of quick tests. I think if we just test the machine name field is hidden from the markup and that the default value saved for the block id is what we expect, we are ok.

dawehner’s picture

+++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.phpundefined
@@ -146,6 +146,16 @@ public function testViewsBlockForm() {
+    $blocks = $storage->load(array('stark.views_block__test_view_block_block_1'));

+++ b/core/modules/views/views.moduleundefined
@@ -1814,3 +1814,28 @@ function views_cache_get($cid, $use_language = FALSE) {
+    $form['machine_name']['#default_value'] = 'views_block__' . $view_id . '_' . $display_id;

Just wondering how we deal with multiple block instances in this patch.

saltednut’s picture

Regarding #8 - we don't - this should probably be handled by BlockFormController:form() to append an integer to the end of a machine_name if that machine_name already exists. That could be a solution to solve a somewhat related issue: #1998664: An existing block instance will be overwritten by a newly created block instance if the two share the same machine_name

damiankloip’s picture

How about something like this then?

dawehner’s picture

+++ b/core/modules/views/views.moduleundefined
@@ -1814,3 +1814,48 @@ function views_cache_get($cid, $use_language = FALSE) {
+function views_generate_block_instance_id($view) {

Urgs procedural code ... you know just make it a static method on the block plugin ... then we can just unit test that piece.

dawehner’s picture

FileSize
7.21 KB

Urgs, I give up now.

Status: Needs review » Needs work

The last submitted patch, drupal-1998582-12.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
436 bytes
7.1 KB

Let's not do a unit tests for that, I don't think it's worth it. The web tests assert the created machine names, which tests that logic pretty well.

dawehner’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
index 0000000..a0a514b
--- /dev/null

--- /dev/null
+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/ViewsBlockTest.phpundefined

I thought we wanted to stop writing a test ...

damiankloip’s picture

FileSize
5.08 KB

I was sure I removed that from the patch!

dawehner’s picture

FileSize
3.5 KB
7.76 KB

I hated to not have more unit tests like code.

Status: Needs review » Needs work

The last submitted patch, drupal-1998582-17.patch, failed testing.

xjm’s picture

Added a commit message template to the summary--dawehner, effulgentsia and I did some major pair programming with @brantwynn on the initial patch. :)

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
7.75 KB

This will fix that last remaining test.

dawehner’s picture

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

Great, thank you. That's a good step towards a more sane views block.

saltednut’s picture

Added a commit message template to the summary--dawehner, effulgentsia and I did some major pair programming with @brantwynn on the initial patch. :)

Thank you!

Status: Reviewed & tested by the community » Needs work
Issue tags: -VDC, -Block plugins

The last submitted patch, 1998582-20.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

#20: 1998582-20.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1998582-20.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
Issue tags: +VDC, +Block plugins

#20: 1998582-20.patch queued for re-testing.

xjm’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This has been RTBC before.

dawehner’s picture

Issue tags: -VDC, -Block plugins

#20: 1998582-20.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +VDC, +Block plugins

The last submitted patch, 1998582-20.patch, failed testing.

dawehner’s picture

+++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.phpundefined
@@ -146,6 +146,26 @@ public function testViewsBlockForm() {
+    $storage = \Drupal::entityManager()->getStorageController('block');
...
+      $storage = \Drupal::entityManager()->getStorageController('block');

While fixing the bugs ... you could also just use $this->controller->get here.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
7.82 KB
jibran’s picture

+++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.phpundefined
@@ -108,6 +108,26 @@ public function testViewsBlockForm() {
+      $storage = \Drupal::entityManager()->getStorageController('block');

I think we don't need this.

damiankloip’s picture

FileSize
915 bytes
7.75 KB

Good spot, we don't need this at all. :)

tim.plunkett’s picture

+++ b/core/modules/views/views.moduleundefined
@@ -1727,3 +1727,25 @@ function views_cache_get($cid, $use_language = FALSE) {
+ * Views overrides block configuration form elements during ViewsBlock:form()
+ * but machine_name assignment is added later by BlockFormController:form()
+ * so we provide an override for the block machine_name here.

This sucks a lot. Oh well.

+++ b/core/modules/views/views.moduleundefined
@@ -1727,3 +1727,25 @@ function views_cache_get($cid, $use_language = FALSE) {
+    $block_plugin = $form_state['build_info']['callback_object']->getEntity()->getPlugin();

This should be $block_plugin = $form_state['controller']->getEntity()->getPlugin();

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice!

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
934 bytes
7.73 KB

I guess that was a cross post?! Here is that change too. Tim can you re-RTBC?

saltednut’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
69.07 KB

This looks good. I am able to place a new Views block without entering a machine_name. After placing the block, I am able to see the auto-magic-machine-name.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, a couple more nitpicks :(

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -112,4 +113,41 @@ protected function addContextualLinks(&$output, $block_type = 'block') {
+  /**
+   * Returns the ViewExecutable instance for this block.
+   *
+   * @return \Drupal\views\ViewExecutable
+   */
+  public function getView() {
+    return $this->view;

This isn't used by the patch

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -112,4 +113,41 @@ protected function addContextualLinks(&$output, $block_type = 'block') {
+     $count = 1;
+     $id = $original_id;
+     while (in_array($id, $block_ids)) {
+       $id = $original_id . '_' . ++$count;

This is fairly obvious to me, but it could use a code comment.

+++ b/core/modules/views/views.moduleundefined
@@ -1727,3 +1727,25 @@ function views_cache_get($cid, $use_language = FALSE) {
+ * Implement hook_form_alter for the views block form.

Implements hook_form_BASE_FORM_ID_alter() for the block form.

+++ b/core/modules/views/views.moduleundefined
@@ -1727,3 +1727,25 @@ function views_cache_get($cid, $use_language = FALSE) {
+ * Views overrides block configuration form elements during ViewsBlock:form()
+ * but machine_name assignment is added later by BlockFormController:form()

Needs ::, and should technically use the fully-qualified class name

+++ b/core/modules/views/views.moduleundefined
@@ -1727,3 +1727,25 @@ function views_cache_get($cid, $use_language = FALSE) {
+  if (($form['settings']['module']['#value'] == 'views') && empty($form['machine_name']['#default_value'])) {

Is this really the best way to identify this form?

+++ b/core/modules/views/views.moduleundefined
@@ -1727,3 +1727,25 @@ function views_cache_get($cid, $use_language = FALSE) {
+    // Unset the machine_name provided by BlockFormController

Missing .

+++ b/core/modules/views/views.moduleundefined
@@ -1727,3 +1727,25 @@ function views_cache_get($cid, $use_language = FALSE) {
+    // Prevent users from changing the auto-generate block machine_name.

auto-generated

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.97 KB
7.69 KB

Is this really the best way to identify this form?

Alas, I'm afraid it looks like it is... :(

Fixed the other points.

saltednut’s picture

+++ b/core/modules/views/views.moduleundefined
@@ -1722,3 +1722,26 @@ function views_cache_get($cid, $use_language = FALSE) {
+/**
+ * Implements hook_form_block_form_alter.

We're actually implementing hook_form_FORM_ID_alter but I don't know what best practice for commenting around that strange hook is.

Is this really the best way to identify this form?

Could be worse - could be using hook_form_alter and also looking for the form_id. :)

Unsure if this needs work to fix the comment.

dawehner’s picture

The standard is according to http://drupal.org/node/1354

 * Implements hook_form_FORM_ID_alter() for node_type_form().
damiankloip’s picture

FileSize
519 bytes
7.71 KB

Let's try and put this issue to bed!

saltednut’s picture

Status: Needs review » Reviewed & tested by the community

Manually tested this again, for the sake of brevity, everything I said in #37 remains the same.

I also ran the testbot locally for all Blocks and Views tests and did not see any issues.

xjm’s picture

Issue summary: View changes

Get rid of dumb parens.

xjm’s picture

Priority: Normal » Major

This blocks our block conversions.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1a290b5 and pushed to 8.x. Thanks!

damiankloip’s picture

Good god - thank you! :)

xjm’s picture

sdboyer’s picture

Priority: Major » Normal
Status: Fixed » Reviewed & tested by the community

i like this pattern. we should do it more. a tiddly bit cleaner, though.

so, i wrote another patch: #2022897: Shift responsibility for creating unique config id/machine name off of (views) block plugins and onto BlockFormController

sdboyer’s picture

Priority: Normal » Major
Status: Reviewed & tested by the community » Fixed

ugh, sorry, zombie form values.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.