REST export view add requirement to all paths, leading to "Method Not Allowed" on most forms

Interesting, this totally makes sense, thanks for catching this. Of course we should not modify all routes, but only our own.

Patch looks good, but this needs a test case. I think we can just unit-test collectRoutes() and check that it only modifies its own routes that are passed in a collection.

1. +++ b/core/modules/rest/lib/Drupal/rest/Tests/Views/CollectRoutesTest.php
   @@ -0,0 +1,50 @@
   +/**
   + * Defines tests for collectRoutes.
   + */
   

   Should be "... for the collectRoutes() method."

2. +++ b/core/modules/rest/lib/Drupal/rest/Tests/Views/CollectRoutesTest.php
   @@ -0,0 +1,50 @@
   +
   +  function setUp() {
   

   Scope modifier missing, I think it is protected.

3. +++ b/core/modules/rest/lib/Drupal/rest/Tests/Views/CollectRoutesTest.php
   @@ -0,0 +1,50 @@
   +    $this->rest_export = RestExport::create($container, array(), "test_routes", array());
   +    $this->routes = new RouteCollection();
   +    $this->routes->add('test_1', '/test/1');
   +    $this->routes->add('test_2', '/test/2');
   

   All class properties should be defined explicitly.

   And shouldn't you test the requirements of one valid route that belongs to the plugin and should be modified?

kicked this along some more, but stuck on

There was 1 error:

1) Drupal\rest\Tests\CollectRoutesTest::testRoutesRequirements
Argument 1 passed to Mock_Serializer_b424d9d2::init() must be an instance of Drupal\views\ViewExecutable, instance of Mock_ViewExecutable_038f176a given, called in /var/www/d8/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php on line 849 and defined

/var/www/d8/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php:849
/var/www/d8/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php:257
/var/www/d8/core/modules/rest/tests/Drupal/rest/Tests/CollectRoutesTest.php:159
/usr/share/php/PHPUnit/TextUI/Command.php:176
/usr/share/php/PHPUnit/TextUI/Command.php:129

Sorry, that was the interdiff.
Full test and interdiff

this is at least major, if not critical

view != views - fixed

So we should have red/green here.

Given that once any view of this type is added, any non GET request returns a Method Not Allowed

This makes a lot more sense when you view the patch without whitespace changes:

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 33a7e6b..4f7a77f 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
@@ -253,7 +253,10 @@ public function optionsSummary(&$categories, &$options) {
    */
   public function collectRoutes(RouteCollection $collection) {
     parent::collectRoutes($collection);
+    $view_id = $this->view->storage->id();
+    $display_id = $this->display['id'];
 
+    if ($route = $collection->get("view.$view_id.$display_id")) {
     $style_plugin = $this->getPlugin('style');
     // REST exports should only respond to get methods.
     $requirements = array('_method' => 'GET');
@@ -261,8 +264,7 @@ public function collectRoutes(RouteCollection $collection) {
     // Format as a string using pipes as a delimeter.
     $requirements['_format'] = implode('|', $style_plugin->getFormats());
 
-    // Add the new requirements to each route.
-    foreach ($collection as $route) {
+      // Add the new requirements to the route.
       $route->addRequirements($requirements);
     }
   }

Running the test without the fix, it has 1 error:

1) Drupal\rest\Tests\CollectRoutesTest::testRoutesRequirements
First route has no requirement.
Failed asserting that 0 matches expected 2.

Looks good to me, nice work.

Looks good to me. Committed/pushed to 8.x, thanks!