[regression] REST in Drupal 8.2.x does not allow HTTP methods other than GET/PATCH/POST/DELETE: OPTIONS, PUT, et cetera all fail

So the recommended action is to completely ignore unknown methods? That sounds like the option which at least doesn't break the update path.

So per #3, this is really about updating REST resource configuration that includes OPTIONS. #2803179-23: [regression] rest_update_8201() can fail and break things describes a similar problem with PUT.

Relevant quote:

The problem is that my custom rest resource implemented a PUT method, which is not valid in Drupal rest

This is a problem, and it makes no sense.

Since this code lives in \Drupal\rest\Entity\RestResourceConfig::normalizeRestMethod() and is called by both \Drupal\rest\Entity\RestResourceConfig::getAuthenticationProvidersForMethodGranularity() and \Drupal\rest\Entity\RestResourceConfig::getFormatsForMethodGranularity(), this is actually applying to not the upgrade path, but simply the behavior. It's possible to create a RestResourceConfig entity in a fresh Drupal 8.2.x site and also run into this problem.

The reason you're also running into it during the upgrade process, is because rest_post_update_resource_granularity() calls $resource_config_entity->save();, which is fine for a post-update function (post-update functions can rely on the entity system).

I think simply removing the validation makes most sense.

Note that this also matches the scope of what \Drupal\rest\Plugin\ResourceBase::requestMethods() allows:

 /**
   * Provides predefined HTTP request methods.
   *
   * Plugins can override this method to provide additional custom request
   * methods.
   *
   * @return array
   *   The list of allowed HTTP request method strings.
   */
  protected function requestMethods() {
    return array(
      'HEAD',
      'GET',
      'POST',
      'PUT',
      'DELETE',
      'TRACE',
      'OPTIONS',
      'CONNECT',
      'PATCH',
    );
  }

That lists all official HTTP methods. But allows a particular REST resource to define additional ones.

We didn't even seem to assert the validation :-D

Indeed!

I think we should have a test for this. We might decide to limit this again in the future and be wrong.

Discussed with @effulgentsia, @catch, @xjm and @cilefen. We agreed this was critical because if this update fails your rest settings and not upgraded to work with 8.2.x and your site is then broken.

Here is a start to an unit test. I added as a pure unit test for speed. It invokes the getMethods method on the entity, which seems to be the easiest way to get at the normalizeRestMethod protected method.

I thought about adding a data provider, but that really could be done in a follow-up for testing getMethods, getFormats, etc... respectively.

+++ b/core/modules/rest/src/Entity/RestResourceConfig.php
@@ -243,24 +243,16 @@ protected function getRestResourceDependencies() {
-    if (!in_array($normalised_method, $valid_methods)) {
-      throw new \InvalidArgumentException('The method is not supported.');
-    }

Ideally our entity plugin could somehow validate this

#15: that's already happening. See \Drupal\rest\Plugin\rest\resource\EntityResource::availableMethods(), which calls \Drupal\rest\Plugin\ResourceBase::availableMethods(), which calls \Drupal\rest\Plugin\ResourceBase::requestMethods().

So, it's being validated not at configuration time, but at the run time.

Ideally, the configuration itself could indeed be validated. But then we need a configuration validation API, which doesn't exist … (#2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs …)

+++ b/core/modules/rest/tests/src/Unit/Entity/RestResourceConfigTest.php
@@ -0,0 +1,42 @@
+   * This also tests that no exceptions are thrown during that method so that
+   * alternate methods such as OPTIONS and PUT are supported.
+   */
+  public function testNormalizeRestMethod() {
+    $expected = ['GET', 'PUT', 'POST', 'PATCH', 'DELETE', 'OPTIONS'];
+    $methods = ['get', 'put', 'post', 'patch', 'delete', 'options'];

I think this should also do a "foo" method, just to show that it literally doesn't care about which HTTP methods are being used.

Once that's done, this is ready IMO.

Adds change to test for adding an unsupported method to assert that normalizeRestMethod should not be doing validation from #17.

I thought about adding a comment, but I wasn't sure about the wording so I left it without.

Thanks!

Re-running test due to #2157927: Intermittent test fails in LocaleUpdateTest::testUpdateImportSourceRemote().

Retesting against 8.2.x, and adding a test against 8.3.x. Because #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method just landed.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!