Issue #2022897 by dawehner, sdboyer, tim.plunkett: Shift responsibility for creating unique config id/machine name off of (views) block plugins and onto BlockFormController.

core/modules/block/lib/Drupal/block/BlockBase.php

@@ -9,6 +9,8 @@ namespace Drupal\block;
 
 use Drupal\Component\Plugin\PluginBase;
 use Drupal\block\BlockInterface;
+use Drupal\Component\Utility\Unicode;
+use Drupal\Core\Language\Language;
 
 /**
  * Defines a base block implementation that most blocks plugins will extend.
@@ -165,4 +167,28 @@ abstract class BlockBase extends PluginBase implements BlockPluginInterface {
    */
   public function blockSubmit($form, &$form_state) {}
 
+  /**
+   * {@inheritdoc}
+   */
+  public function getMachineNameSuggestion() {
+    $definition = $this->getPluginDefinition();
+    $admin_label = $definition['admin_label'];
+
+    // @todo This is basically the same as what is done in
+    //   \Drupal\system\MachineNameController::transliterate(), so it might make
+    //   sense to provide a common service for the two.
+    $transliteration_service = \Drupal::service('transliteration');
+    $transliterated = $transliteration_service->transliterate($admin_label, Language::LANGCODE_DEFAULT, '_');
+
+    $replace_pattern = '[^a-z0-9_.]+';
+
+    $transliterated = Unicode::strtolower($transliterated);
+
+    if (isset($replace_pattern)) {
+      $transliterated = preg_replace('@' . $replace_pattern . '@', '', $transliterated);
+    }
+
+    return $transliterated;
+  }
+
 }

core/modules/block/lib/Drupal/block/BlockFormController.php

@@ -7,13 +7,60 @@
 
 namespace Drupal\block;
 
+use Drupal\Core\Entity\EntityControllerInterface;
 use Drupal\Core\Entity\EntityFormController;
+use Drupal\Core\Entity\EntityManager;
+use Drupal\Core\Entity\Query\QueryFactory;
+use Drupal\Core\Extension\ModuleHandlerInterface;
 use Drupal\Core\Language\Language;
+use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
  * Provides form controller for block instance forms.
  */
-class BlockFormController extends EntityFormController {
+class BlockFormController extends EntityFormController implements EntityControllerInterface {
+
+  /**
+   * The block storage controller.
+   *
+   * @var \Drupal\Core\Entity\EntityStorageControllerInterface
+   */
+  protected $storageController;
+
+  /**
+   * The entity query factory.
+   *
+   * @var \Drupal\Core\Entity\Query\QueryFactory
+   */
+  protected $entityQueryFactory;
+
+  /**
+   * Constructs a BlockFormController object.
+   *
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface
+   *   The module handler service.
+   * @param \Drupal\Core\Entity\EntityManager $entity_manager
+   *   The entity manager.
+   * @param \Drupal\Core\Entity\Query\QueryFactory $entity_query_factory
+   *   The entity query factory.
+   */
+  public function __construct(ModuleHandlerInterface $module_handler, EntityManager $entity_manager, QueryFactory $entity_query_factory) {
+    parent::__construct($module_handler);
+
+    $this->storageController = $entity_manager->getStorageController('block');
+    $this->entityQueryFactory = $entity_query_factory;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function createInstance(ContainerInterface $container, $entity_type, array $entity_info) {
+    return new static(
+      $container->get('module_handler'),
+      $container->get('plugin.manager.entity'),
+      $container->get('entity.query')
+    );
+  }
 
   /**
    * Overrides \Drupal\Core\Entity\EntityFormController::form().
@@ -27,12 +74,17 @@ class BlockFormController extends EntityFormController {
     );
     $form['settings'] = $entity->getPlugin()->buildConfigurationForm(array(), $form_state);
 
+    // If creating a new block, calculate a safe default machine name.
+    if ($entity->isNew()) {
+      $machine_default = $this->getUniqueMachineName($entity);
+    }
+
     $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(),
+      '#description' => t('A unique name for this block instance. Must be alpha-numeric and underscore separated.'),
+      '#default_value' => !$entity->isNew() ? $entity->id() : $machine_default,
       '#machine_name' => array(
         'exists' => 'block_load',
         'replace_pattern' => '[^a-z0-9_.]+',
@@ -234,4 +286,36 @@ class BlockFormController extends EntityFormController {
     $form_state['redirect'] = 'admin/structure/block/list/block_plugin_ui:' . $entity->get('theme');
   }
 
+  /**
+   * Generates a unique machine name for a block.
+   *
+   * @param \Drupal\block\BlockInterface $block
+   *   The block entity.
+   *
+   * @return string
+   *   Returns the unique name.
+   */
+  public function getUniqueMachineName(BlockInterface $block) {
+    $suggestion = $block->getPlugin()->getMachineNameSuggestion();
+
+    // Get all the blocks which starts with the suggested machine name.
+    $query = $this->entityQueryFactory->get('block');
+    $query->condition('id', $suggestion, 'CONTAINS');
+    $block_ids = $query->execute();
+
+    $block_ids = array_map(function ($block_id) {
+      $parts = explode('.', $block_id);
+      return end($parts);
+    }, $block_ids);
+
+    // Iterate through potential IDs until we get a new one. E.g.
+    // 'plugin', 'plugin_2', 'plugin_3'...
+    $count = 1;
+    $machine_default = $suggestion;
+    while (in_array($machine_default, $block_ids)) {
+      $machine_default = $suggestion . '_' . ++$count;
+    }
+    return $machine_default;
+  }
+
 }

core/modules/block/lib/Drupal/block/BlockPluginInterface.php

@@ -122,4 +122,16 @@ interface BlockPluginInterface extends ConfigurablePluginInterface, PluginFormIn
    */
   public function blockSubmit($form, &$form_state);
 
+  /**
+   * Suggests a machine name to identify an instance of this block.
+   *
+   * The block plugin need not verify that the machine name is at all unique. It
+   * is only responsible for providing a baseline suggestion; calling code is
+   * responsible for ensuring whatever uniqueness is required for the use case.
+   *
+   * @return string
+   *   The suggested machine name.
+   */
+  public function getMachineNameSuggestion();
+
 }

core/modules/block/lib/Drupal/block/Tests/BlockInterfaceTest.php

@@ -92,5 +92,9 @@ class BlockInterfaceTest extends DrupalUnitTestBase {
     );
     // Ensure the build array is proper.
     $this->assertIdentical($display_block->build(), $expected_build, 'The plugin returned the appropriate build array.');
+
+    // Ensure the machine name suggestion is correct. In truth, this is actually
+    // testing BlockBase's implementation, not the interface itself.
+    $this->assertIdentical($display_block->getMachineNameSuggestion(), 'displaymessage', 'The plugin returned the expected machine name suggestion.');
   }
 }

core/modules/block/lib/Drupal/block/Tests/BlockUiTest.php

@@ -19,7 +19,7 @@ class BlockUiTest extends WebTestBase {
    *
    * @var array
    */
-  public static $modules = array('block');
+  public static $modules = array('block', 'block_test');
 
   protected $regions;
 
@@ -113,4 +113,24 @@ class BlockUiTest extends WebTestBase {
     $blocks = drupal_json_decode($this->drupalGet('system/autocomplete/block_plugin_ui:stark', array('query' => array('q' => $block))));
     $this->assertEqual($blocks['system_menu_block:menu-admin'], $block, t('Can search for block with name !block.', array('!block' => $block)));
   }
+
+  /**
+   * Tests that the BlockFormController populates machine name correctly.
+   */
+  public function testMachineNameSuggestion() {
+    $url = 'admin/structure/block/add/test_block_instantiation/stark';
+    $this->drupalGet($url);
+    $this->assertFieldByName('machine_name', 'displaymessage', 'Block form uses raw machine name suggestion when no instance already exists.');
+    $this->drupalPost($url, array(), 'Save block');
+
+    // Now, check to make sure the form starts by autoincrementing correctly.
+    $this->drupalGet($url);
+    $this->assertFieldByName('machine_name', 'displaymessage_2', 'Block form appends _2 to plugin-suggested machine name when an instance already exists.');
+    $this->drupalPost($url, array(), 'Save block');
+
+    // And verify that it continues working beyond just the first two.
+    $this->drupalGet($url);
+    $this->assertFieldByName('machine_name', 'displaymessage_3', 'Block form appends _3 to plugin-suggested machine name when two instances already exist.');
+  }
+
 }

core/modules/block/tests/lib/Drupal/block/Tests/BlockBaseTest.php

@@ -0,0 +1,61 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Tests\block\BlockBaseTest.
+ */
+
+namespace Drupal\Tests\block;
+
+use Drupal\block_test\Plugin\Block\TestBlockInstantiation;
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\Core\Transliteration\PHPTransliteration;
+use Drupal\Tests\UnitTestCase;
+
+// @todo Remove once the constants are replaced with constants on classes.
+if (!defined('DRUPAL_NO_CACHE')) {
+  define('DRUPAL_NO_CACHE', -1);
+}
+
+/**
+ * Tests the base block plugin.
+ *
+ * @see \Drupal\block\BlockBase
+ */
+class BlockBaseTest extends UnitTestCase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Base plugin',
+      'description' => 'Tests the base block plugin.',
+      'group' => 'Block',
+    );
+  }
+
+  /**
+   * Tests the machine name suggestion.
+   *
+   * @see \Drupal\block\BlockBase::getMachineNameSuggestion().
+   */
+  public function testGetMachineNameSuggestion() {
+    $transliteraton = $this->getMockBuilder('Drupal\Core\Transliteration\PHPTransliteration')
+      // @todo Inject the module handler into PHPTransliteration.
+      ->setMethods(array('readLanguageOverrides'))
+      ->getMock();
+
+    $container = new ContainerBuilder();
+    $container->set('transliteration', $transliteraton);
+    \Drupal::setContainer($container);
+
+    $config = array();
+    $definition = array('admin_label' => 'Admin label', 'module' => 'block_test');
+    $block_base = new TestBlockInstantiation($config, 'test_block_instantiation', $definition);
+    $this->assertEquals('adminlabel', $block_base->getMachineNameSuggestion());
+
+    // Test with more unicodes.
+    $definition = array('admin_label' =>'über åwesome', 'module' => 'block_test');
+    $block_base = new TestBlockInstantiation($config, 'test_block_instantiation', $definition);
+    $this->assertEquals('uberawesome', $block_base->getMachineNameSuggestion());
+  }
+
+}

core/modules/block/tests/lib/Drupal/block/Tests/BlockFormControllerTest.php

@@ -0,0 +1,102 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Tests\block\BlockFormControllerTest.
+ */
+
+namespace Drupal\Tests\block;
+
+use Drupal\block\BlockFormController;
+use Drupal\Tests\UnitTestCase;
+
+// @todo Remove once the constants are replaced with constants on classes.
+if (!defined('BLOCK_REGION_NONE')) {
+  define('BLOCK_REGION_NONE', -1);
+}
+
+// @todo Remove once the constants are replaced with constants on classes.
+if (!defined('FIELD_LOAD_CURRENT')) {
+  define('FIELD_LOAD_CURRENT', 'FIELD_LOAD_CURRENT');
+}
+
+/**
+ * Tests the block form controller.
+ *
+ * @see \Drupal\block\BlockFormController
+ */
+class BlockFormControllerTest extends UnitTestCase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Block form controller',
+      'description' => 'Tests the block form controller.',
+      'group' => 'Block',
+    );
+  }
+
+  /**
+   * Tests the unique machine name generator.
+   *
+   * @see \Drupal\block\BlockFormController::getUniqueMachineName()
+   */
+  public function testGetUniqueMachineName() {
+    $block_storage = $this->getMockBuilder('Drupal\block\BlockStorageController')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $blocks = array();
+
+    $blocks['test'] = $this->getBlockMockWithMachineName('test');
+    $blocks['other_test'] = $this->getBlockMockWithMachineName('other_test');
+    $blocks['other_test_1'] = $this->getBlockMockWithMachineName('other_test');
+    $blocks['other_test_2'] = $this->getBlockMockWithMachineName('other_test');
+
+    $query = $this->getMockBuilder('Drupal\Core\Entity\Query\QueryInterface')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $query->expects($this->exactly(5))
+      ->method('condition')
+      ->will($this->returnValue($query));
+
+    $query->expects($this->exactly(5))
+      ->method('execute')
+      ->will($this->returnValue(array('test', 'other_test', 'other_test_1', 'other_test_2')));
+
+    $query_factory = $this->getMockBuilder('Drupal\Core\Entity\Query\QueryFactory')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $query_factory->expects($this->exactly(5))
+      ->method('get')
+      ->with('block', 'AND')
+      ->will($this->returnValue($query));
+
+    $entity_manager = $this->getMockBuilder('Drupal\Core\Entity\EntityManager')
+      ->disableOriginalConstructor()
+      ->getMock();
+
+    $entity_manager->expects($this->any())
+      ->method('getStorageController')
+      ->will($this->returnValue($block_storage));
+
+    $module_handler = $this->getMockBuilder('Drupal\Core\Extension\ModuleHandlerInterface')
+      ->getMock();
+
+    $block_form_controller = new BlockFormController($module_handler, $entity_manager, $query_factory);
+
+    // Ensure that the block with just one other instance gets the next available
+    // name suggestion.
+    $this->assertEquals('test_2', $block_form_controller->getUniqueMachineName($blocks['test']));
+
+    // Ensure that the block with already three instances (_0, _1, _2) gets the
+    // 4th available name.
+    $this->assertEquals('other_test_3', $block_form_controller->getUniqueMachineName($blocks['other_test']));
+    $this->assertEquals('other_test_3', $block_form_controller->getUniqueMachineName($blocks['other_test_1']));
+    $this->assertEquals('other_test_3', $block_form_controller->getUniqueMachineName($blocks['other_test_2']));
+
+    // Ensure that a block without an instance yet gets the suggestion as
+    // unique machine name.
+    $last_block = $this->getBlockMockWithMachineName('last_test');
+    $this->assertEquals('last_test', $block_form_controller->getUniqueMachineName($last_block));
+  }
+
+}

core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php

@@ -218,4 +218,13 @@ class ViewsBlock extends BlockBase implements ContainerFactoryPluginInterface {
      return $id;
    }
 
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getMachineNameSuggestion() {
+    $this->view->setDisplay($this->displayID);
+    return 'views_block__' . $this->view->storage->id() . '_' . $this->view->current_display;
+  }
+
 }

core/modules/views/lib/Drupal/views/Tests/Plugin/ViewsBlockTest.php

@@ -34,7 +34,7 @@ class ViewsBlockTest extends ViewUnitTestBase {
   public static function getInfo() {
     return array(
       'name' => 'Views block',
-      'description' => 'Tests the block views plugin.',
+      'description' => 'Tests native behaviors of the block views plugin.',
       'group' => 'Views Plugins',
     );
   }
@@ -49,38 +49,18 @@ class ViewsBlockTest extends ViewUnitTestBase {
   }
 
   /**
-   * Tests generateBlockInstanceID.
+   * Tests that ViewsBlock::getMachineNameSuggestion() produces the right value.
    *
-   * @see \Drupal\views\Plugin\Block::generateBlockInstanceID().
+   * @see \Drupal\views\Plugin\Block::getmachineNameSuggestion().
    */
-  public function testGenerateBlockInstanceID() {
-
+  public function testMachineNameSuggestion() {
     $plugin_definition = array(
       'module' => 'views',
     );
     $plugin_id = 'views_block:test_view_block-block_1';
     $views_block = ViewsBlock::create($this->container, array(), $plugin_id, $plugin_definition);
 
-    $storage_controller = $this->container->get('plugin.manager.entity')->getStorageController('block');
-
-    // Generate a instance ID on a block without any instances.
-    $this->assertEqual($views_block->generateBlockInstanceID($storage_controller), 'views_block__test_view_block_block_1');
-
-    $values = array(
-      'plugin' => $plugin_id,
-      'id' => 'stark.views_block__test_view_block_block_1',
-      'module' => 'views',
-      'settings' => array(
-        'module' => 'views',
-      )
-    );
-    $storage_controller->create($values)->save();
-    $this->assertEqual($views_block->generateBlockInstanceID($storage_controller), 'views_block__test_view_block_block_1_2');
-
-    // Add another one block instance and ensure the block instance went up.
-    $values['id'] = 'stark.views_block__test_view_block_block_1_2';
-    $storage_controller->create($values)->save();
-    $this->assertEqual($views_block->generateBlockInstanceID($storage_controller), 'views_block__test_view_block_block_1_3');
+    $this->assertEqual($views_block->getMachineNameSuggestion(), 'views_block__test_view_block_block_1');
   }
 
 }

core/modules/views/views.module

@@ -1663,25 +1663,3 @@ function views_cache_get($cid, $use_language = FALSE) {
   return cache('views_info')->get($cid);
 }
 
-/**
- * Implements hook_form_FORM_ID_alter for block_form().
- *
- * Views overrides block configuration form elements during
- * \Drupal\views\Plugin\Block\ViewsBlock::form() but machine_name assignment is
- * added later by \Drupal\block\BlockFormController::form() so we provide an
- * override for the block machine_name here.
- */
-function views_form_block_form_alter(&$form, &$form_state) {
-  // Ensure the block-form being altered is a Views block configuration form.
-  if (($form['settings']['module']['#value'] == 'views') && empty($form['machine_name']['#default_value'])) {
-    // Unset the machine_name provided by BlockFormController.
-    unset($form['machine_name']['#machine_name']['source']);
-    // Load the Views plugin object using form_state array and create a
-    // block machine_name based on the View ID and View Display ID.
-    $block_plugin = $form_state['controller']->getEntity()->getPlugin();
-    // Override the Views block's machine_name by providing a default_value.
-    $form['machine_name']['#default_value'] = $block_plugin->generateBlockInstanceID(Drupal::entityManager()->getStorageController('block'));
-    // Prevent users from changing the auto-generated block machine_name.
-    $form['machine_name']['#access'] = FALSE;
-  }
-}

core/tests/Drupal/Tests/UnitTestCase.php

@@ -121,4 +121,30 @@ abstract class UnitTestCase extends \PHPUnit_Framework_TestCase {
     return $config_storage;
   }
 
+  /**
+   * Mocks a block with a block plugin.
+   *
+   * @param string $machine_name
+   *   The machine name of the block plugin.
+   *
+   * @return \Drupal\block\BlockInterface|\PHPUnit_Framework_MockObject_MockObject
+   *   The mocked block.
+   */
+  protected function getBlockMockWithMachineName($machine_name) {
+    $plugin = $this->getMockBuilder('Drupal\block\BlockBase')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $plugin->expects($this->any())
+      ->method('getMachineNameSuggestion')
+      ->will($this->returnValue($machine_name));
+
+    $block = $this->getMockBuilder('Drupal\block\Plugin\Core\Entity\Block')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $block->expects($this->any())
+      ->method('getPlugin')
+      ->will($this->returnValue($plugin));
+    return $block;
+  }
+
 }