From c056f6dfe6b8544c25964eaee9cb4a087591e4ee Mon Sep 17 00:00:00 2001 From: webchick Date: Tue, 6 May 2014 13:33:23 -0700 Subject: [PATCH] Issue #2241827 by killua99, dawehner, djevans | Crell: Using a numeric placeholder in paths in Views UI causes fatal error. --- .../rest/Plugin/views/display/RestExport.php | 9 ++- .../Drupal/rest/Tests/CollectRoutesTest.php | 3 + .../Plugin/views/display/PathPluginBase.php | 61 +++++++++++++++++-- .../Plugin/display/PathPluginBaseTest.php | 10 ++- .../lib/Drupal/views_ui/Tests/DisplayPath.php | 23 +++++++ .../views_ui/Tests/ViewListBuilderTest.php | 3 +- 6 files changed, 100 insertions(+), 9 deletions(-) diff --git a/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php index b28b477085..c6055e2c14 100644 --- a/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php +++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php @@ -7,7 +7,7 @@ namespace Drupal\rest\Plugin\views\display; - +use Drupal\Core\Form\FormBuilderInterface; use Drupal\Core\State\StateInterface; use Drupal\Core\Routing\RouteProviderInterface; use Drupal\Core\ContentNegotiation; @@ -99,13 +99,15 @@ class RestExport extends PathPluginBase { * The route provider * @param \Drupal\Core\State\StateInterface $state * The state key value store. + * @param \Drupal\Core\Form\FormBuilderInterface $form_error + * The form builder. * @param \Drupal\Core\ContentNegotiation $content_negotiation * The content negotiation library. * @param \Symfony\Component\HttpFoundation\Request $request * The request object. */ - public function __construct(array $configuration, $plugin_id, $plugin_definition, RouteProviderInterface $route_provider, StateInterface $state, ContentNegotiation $content_negotiation, Request $request) { - parent::__construct($configuration, $plugin_id, $plugin_definition, $route_provider, $state); + public function __construct(array $configuration, $plugin_id, $plugin_definition, RouteProviderInterface $route_provider, StateInterface $state, FormBuilderInterface $form_error, ContentNegotiation $content_negotiation, Request $request) { + parent::__construct($configuration, $plugin_id, $plugin_definition, $route_provider, $state, $form_error); $this->contentNegotiation = $content_negotiation; $this->request = $request; } @@ -120,6 +122,7 @@ public static function create(ContainerInterface $container, array $configuratio $plugin_definition, $container->get('router.route_provider'), $container->get('state'), + $container->get('form_builder'), $container->get('content_negotiation'), $container->get('request') ); diff --git a/core/modules/rest/tests/Drupal/rest/Tests/CollectRoutesTest.php b/core/modules/rest/tests/Drupal/rest/Tests/CollectRoutesTest.php index 5b78e769e3..db0e724fdc 100644 --- a/core/modules/rest/tests/Drupal/rest/Tests/CollectRoutesTest.php +++ b/core/modules/rest/tests/Drupal/rest/Tests/CollectRoutesTest.php @@ -90,6 +90,9 @@ protected function setUp() { ->getMock(); $container->set('plugin.manager.views.style', $style_manager); + $form_builder = $this->getMock('Drupal\Core\Form\FormBuilderInterface'); + $container->set('form_builder', $form_builder); + \Drupal::setContainer($container); $this->restExport = RestExport::create($container, array(), "test_routes", array()); diff --git a/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php index bf4e008c39..baa777307d 100644 --- a/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php @@ -7,6 +7,7 @@ namespace Drupal\views\Plugin\views\display; +use Drupal\Core\Form\FormErrorInterface; use Drupal\Core\State\StateInterface; use Drupal\Core\Routing\RouteCompiler; use Drupal\Core\Routing\RouteProviderInterface; @@ -39,6 +40,13 @@ abstract class PathPluginBase extends DisplayPluginBase implements DisplayRouter */ protected $state; + /** + * The form error helper. + * + * @var \Drupal\Core\Form\FormErrorInterface + */ + protected $formError; + /** * Constructs a PathPluginBase object. * @@ -52,12 +60,15 @@ abstract class PathPluginBase extends DisplayPluginBase implements DisplayRouter * The route provider. * @param \Drupal\Core\State\StateInterface $state * The state key value store. + * @param \Drupal\Core\Form\FormErrorInterface $form_error + * The form error helper. */ - public function __construct(array $configuration, $plugin_id, $plugin_definition, RouteProviderInterface $route_provider, StateInterface $state) { + public function __construct(array $configuration, $plugin_id, $plugin_definition, RouteProviderInterface $route_provider, StateInterface $state, FormErrorInterface $form_error) { parent::__construct($configuration, $plugin_id, $plugin_definition); $this->routeProvider = $route_provider; $this->state = $state; + $this->formError = $form_error; } /** @@ -69,7 +80,8 @@ public static function create(ContainerInterface $container, array $configuratio $plugin_id, $plugin_definition, $container->get('router.route_provider'), - $container->get('state') + $container->get('state'), + $container->get('form_builder') ); } @@ -395,8 +407,9 @@ public function validateOptionsForm(&$form, &$form_state) { parent::validateOptionsForm($form, $form_state); if ($form_state['section'] == 'path') { - if (strpos($form_state['values']['path'], '%') === 0) { - form_error($form['path'], $form_state, t('"%" may not be used for the first segment of a path.')); + $errors = $this->validatePath($form_state['values']['path']); + foreach ($errors as $error) { + $this->formError->setError($form['path'], $form_state, $error); } // Automatically remove '/' and trailing whitespace from path. @@ -415,4 +428,44 @@ public function submitOptionsForm(&$form, &$form_state) { } } + /** + * Validates the path of the display. + * + * @param string $path + * The path to validate. + * + * @return array + * A list of error strings. + */ + protected function validatePath($path) { + $errors = array(); + if (strpos($path, '%') === 0) { + $errors[] = $this->t('"%" may not be used for the first segment of a path.'); + } + + $path_sections = explode('/', $path); + // Symfony routing does not allow to use numeric placeholders. + // @see \Symfony\Component\Routing\RouteCompiler + $numeric_placeholders = array_filter($path_sections, function ($section) { + return (preg_match('/^%(.*)/', $section, $matches) + && is_numeric($matches[1])); + }); + if (!empty($numeric_placeholders)) { + $errors[] = $this->t("Numeric placeholders may not be used. Please use plain placeholders (%)."); + } + return $errors; + } + + /** + * {@inheritdoc} + */ + public function validate() { + $errors = parent::validate(); + + $errors += $this->validatePath($this->getOption('path')); + + return $errors; + } + + } diff --git a/core/modules/views/tests/Drupal/views/Tests/Plugin/display/PathPluginBaseTest.php b/core/modules/views/tests/Drupal/views/Tests/Plugin/display/PathPluginBaseTest.php index 95f140ebfe..596b80545c 100644 --- a/core/modules/views/tests/Drupal/views/Tests/Plugin/display/PathPluginBaseTest.php +++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/display/PathPluginBaseTest.php @@ -47,6 +47,13 @@ class PathPluginBaseTest extends UnitTestCase { */ protected $state; + /** + * The mocked form error. + * + * @var \Drupal\Core\Form\FormErrorInterface|\PHPUnit_Framework_MockObject_MockObject + */ + protected $formError; + public static function getInfo() { return array( 'name' => 'Display: Path plugin base.', @@ -63,8 +70,9 @@ protected function setUp() { $this->routeProvider = $this->getMock('Drupal\Core\Routing\RouteProviderInterface'); $this->state = $this->getMock('\Drupal\Core\State\StateInterface'); + $this->formError = $this->getMock('Drupal\Core\Form\FormErrorInterface'); $this->pathPlugin = $this->getMockBuilder('Drupal\views\Plugin\views\display\PathPluginBase') - ->setConstructorArgs(array(array(), 'path_base', array(), $this->routeProvider, $this->state)) + ->setConstructorArgs(array(array(), 'path_base', array(), $this->routeProvider, $this->state, $this->formError)) ->setMethods(NULL) ->getMock(); $this->setupContainer(); diff --git a/core/modules/views_ui/lib/Drupal/views_ui/Tests/DisplayPath.php b/core/modules/views_ui/lib/Drupal/views_ui/Tests/DisplayPath.php index 8435589cfe..8eb3e6da2a 100644 --- a/core/modules/views_ui/lib/Drupal/views_ui/Tests/DisplayPath.php +++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/DisplayPath.php @@ -30,6 +30,14 @@ public static function getInfo() { } public function testPathUI() { + $this->doBasicPathUITest(); + $this->doAdvancedPathsValidationTest(); + } + + /** + * Tests basic functionality in configuring a view. + */ + protected function doBasicPathUITest() { $this->drupalGet('admin/structure/views/view/test_view'); // Add a new page display and check the appearing text. @@ -44,6 +52,21 @@ public function testPathUI() { $this->assertLink(t('View @display', array('@display' => 'Page')), 0, 'view page link found on the page.'); } + /** + * Tests a couple of invalid path patterns. + */ + protected function doAdvancedPathsValidationTest() { + $url = 'admin/structure/views/nojs/display/test_view/page_1/path'; + + $this->drupalPostForm($url, array('path' => '%/magrathea'), t('Apply')); + $this->assertUrl($url); + $this->assertText('"%" may not be used for the first segment of a path.'); + + $this->drupalPostForm($url, array('path' => 'user/%1/example'), t('Apply')); + $this->assertUrl($url); + $this->assertText("Numeric placeholders may not be used. Please use plain placeholders (%)."); + } + /** * Tests deleting a page display that has no path. */ diff --git a/core/modules/views_ui/tests/Drupal/views_ui/Tests/ViewListBuilderTest.php b/core/modules/views_ui/tests/Drupal/views_ui/Tests/ViewListBuilderTest.php index 902b615ad7..73f5013e7c 100644 --- a/core/modules/views_ui/tests/Drupal/views_ui/Tests/ViewListBuilderTest.php +++ b/core/modules/views_ui/tests/Drupal/views_ui/Tests/ViewListBuilderTest.php @@ -79,9 +79,10 @@ public function testBuildRowEntityList() { ); $route_provider = $this->getMock('Drupal\Core\Routing\RouteProviderInterface'); $state = $this->getMock('\Drupal\Core\State\StateInterface'); + $form_builder = $this->getMock('Drupal\Core\Form\FormBuilderInterface'); $page_display = $this->getMock('Drupal\views\Plugin\views\display\Page', array('initDisplay', 'getPath'), - array(array(), 'default', $display_manager->getDefinition('page'), $route_provider, $state) + array(array(), 'default', $display_manager->getDefinition('page'), $route_provider, $state, $form_builder) ); $page_display->expects($this->any()) ->method('getPath') -- GitLab