From 8c384bc415c675e5abe6a32eafa52f91309fea6e Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Sat, 3 Aug 2013 23:28:28 +0200 Subject: [PATCH] Issue #2022897 by dawehner, sdboyer, tim.plunkett: Shift responsibility for creating unique config id/machine name off of (views) block plugins and onto BlockFormController. --- .../block/lib/Drupal/block/BlockBase.php | 26 +++++ .../lib/Drupal/block/BlockFormController.php | 90 +++++++++++++++- .../lib/Drupal/block/BlockPluginInterface.php | 12 +++ .../Drupal/block/Tests/BlockInterfaceTest.php | 4 + .../lib/Drupal/block/Tests/BlockUiTest.php | 22 +++- .../lib/Drupal/block/Tests/BlockBaseTest.php | 61 +++++++++++ .../block/Tests/BlockFormControllerTest.php | 102 ++++++++++++++++++ .../Drupal/views/Plugin/Block/ViewsBlock.php | 9 ++ .../views/Tests/Plugin/ViewsBlockTest.php | 30 +----- core/modules/views/views.module | 22 ---- core/tests/Drupal/Tests/UnitTestCase.php | 26 +++++ 11 files changed, 353 insertions(+), 51 deletions(-) create mode 100644 core/modules/block/tests/lib/Drupal/block/Tests/BlockBaseTest.php create mode 100644 core/modules/block/tests/lib/Drupal/block/Tests/BlockFormControllerTest.php diff --git a/core/modules/block/lib/Drupal/block/BlockBase.php b/core/modules/block/lib/Drupal/block/BlockBase.php index a8e4ed153fb..f1217c0c4a0 100644 --- a/core/modules/block/lib/Drupal/block/BlockBase.php +++ b/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; + } + } diff --git a/core/modules/block/lib/Drupal/block/BlockFormController.php b/core/modules/block/lib/Drupal/block/BlockFormController.php index 7789c026092..0386277e044 100644 --- a/core/modules/block/lib/Drupal/block/BlockFormController.php +++ b/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; + } + } diff --git a/core/modules/block/lib/Drupal/block/BlockPluginInterface.php b/core/modules/block/lib/Drupal/block/BlockPluginInterface.php index c7368ae553a..5f353a4a3de 100644 --- a/core/modules/block/lib/Drupal/block/BlockPluginInterface.php +++ b/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(); + } diff --git a/core/modules/block/lib/Drupal/block/Tests/BlockInterfaceTest.php b/core/modules/block/lib/Drupal/block/Tests/BlockInterfaceTest.php index 1c73cc27ad6..540598900b8 100644 --- a/core/modules/block/lib/Drupal/block/Tests/BlockInterfaceTest.php +++ b/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.'); } } diff --git a/core/modules/block/lib/Drupal/block/Tests/BlockUiTest.php b/core/modules/block/lib/Drupal/block/Tests/BlockUiTest.php index cd9d955682d..7ad84e19e89 100644 --- a/core/modules/block/lib/Drupal/block/Tests/BlockUiTest.php +++ b/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.'); + } + } diff --git a/core/modules/block/tests/lib/Drupal/block/Tests/BlockBaseTest.php b/core/modules/block/tests/lib/Drupal/block/Tests/BlockBaseTest.php new file mode 100644 index 00000000000..085343eeb40 --- /dev/null +++ b/core/modules/block/tests/lib/Drupal/block/Tests/BlockBaseTest.php @@ -0,0 +1,61 @@ + '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()); + } + +} diff --git a/core/modules/block/tests/lib/Drupal/block/Tests/BlockFormControllerTest.php b/core/modules/block/tests/lib/Drupal/block/Tests/BlockFormControllerTest.php new file mode 100644 index 00000000000..e57e8771cb5 --- /dev/null +++ b/core/modules/block/tests/lib/Drupal/block/Tests/BlockFormControllerTest.php @@ -0,0 +1,102 @@ + '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)); + } + +} diff --git a/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php index 9df4a18d3b6..c749a195af7 100644 --- a/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php +++ b/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; + } + } diff --git a/core/modules/views/lib/Drupal/views/Tests/Plugin/ViewsBlockTest.php b/core/modules/views/lib/Drupal/views/Tests/Plugin/ViewsBlockTest.php index b42726288a1..f1a05635989 100644 --- a/core/modules/views/lib/Drupal/views/Tests/Plugin/ViewsBlockTest.php +++ b/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'); } } diff --git a/core/modules/views/views.module b/core/modules/views/views.module index a70f4e4678f..bb312ffdc8e 100644 --- a/core/modules/views/views.module +++ b/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; - } -} diff --git a/core/tests/Drupal/Tests/UnitTestCase.php b/core/tests/Drupal/Tests/UnitTestCase.php index 727976f79b5..d4bd53f7dcb 100644 --- a/core/tests/Drupal/Tests/UnitTestCase.php +++ b/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; + } + }