Issue #2241827 by killua99, dawehner, djevans | Crell: Using a numeric placeholder in paths in Views UI causes fatal error.

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 @@ class RestExport extends PathPluginBase {
       $plugin_definition,
       $container->get('router.route_provider'),
       $container->get('state'),
+      $container->get('form_builder'),
       $container->get('content_negotiation'),
       $container->get('request')
     );

core/modules/rest/tests/Drupal/rest/Tests/CollectRoutesTest.php

@@ -90,6 +90,9 @@ class CollectRoutesTest extends UnitTestCase {
       ->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());

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 @@ abstract class PathPluginBase extends DisplayPluginBase implements DisplayRouter
       $plugin_id,
       $plugin_definition,
       $container->get('router.route_provider'),
-      $container->get('state')
+      $container->get('state'),
+      $container->get('form_builder')
     );
   }
 
@@ -395,8 +407,9 @@ abstract class PathPluginBase extends DisplayPluginBase implements DisplayRouter
     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 @@ abstract class PathPluginBase extends DisplayPluginBase implements DisplayRouter
     }
   }
 
+  /**
+   * 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;
+  }
+
+
 }

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 @@ class PathPluginBaseTest extends UnitTestCase {
 
     $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();

core/modules/views_ui/lib/Drupal/views_ui/Tests/DisplayPath.php

@@ -30,6 +30,14 @@ class DisplayPath extends UITestBase {
   }
 
   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 @@ class DisplayPath extends UITestBase {
     $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.
    */

core/modules/views_ui/tests/Drupal/views_ui/Tests/ViewListBuilderTest.php

@@ -79,9 +79,10 @@ class ViewListBuilderTest extends UnitTestCase {
     );
     $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')