Problem/Motivation

  1. Enable REST module
  2. Try doing a GET request, that works:
        jQuery.ajax({
          url: Drupal.url('node/' + nodeID) + '?_format=hal_json',
          method: 'GET',
          success: function(data, status, xhr) {
            var json = data;
            console.log(json);
          }
        });
    

    Success!

  3. Now let's try to do a PATCH request:
        jQuery.ajax({
          url: Drupal.url('node/' + nodeID) + '?_format=hal_json',
          method: 'PATCH',
          data: {},
        });
    

    Now you get a 405 (Method Not Allowed) response, with this body: {"message":"No route found for \u0022PATCH \/node\/2\u0022: Method Not Allowed (Allow: GET, POST, DELETE)"}. This makes no sense at all: why is the PATCH method not listed?

  4. Clearly, the previous thing is also wrong because the Content-Type request header is missing. So, let's add that:
        jQuery.ajax({
          url: Drupal.url('node/' + nodeID) + '?_format=hal_json',
          headers: {
            "Content-Type": "application/hal+json"
          },
          method: 'PATCH',
          data: {},
        });
    

    Now you get a 403, and {"message":""} as the response body.

Proposed resolution

  1. Upon debugging this, you end up deep in the routing system, where it will only have matched the "GET" and "DELETE" routes. The "PATCH" and "POST" routes are both missing. So if the "POST" route is missing, why is the "POST" method allowed? Because apparently the "GET" route allows both the "GET" and "POST" methods.
  2. Oh, apparently you need to specify the X-CSRF-Token request header with a valid CSRF token as a value. Upon doing that, routing does work correctly. This will be solved in #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response.

Improve routing + response:

  1. Response should be a 415, not a 405. This points to a deep routing problem.
  2. Response should be a 401, not a 403, and its body should say Required "X-CSRF-Token" request header missing, see https://www.drupal.org/docs/somewhere/useful/info.. Not an empty message, which is just infuriating.This will be solved in #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response.

(Note this seems to belong both in the rest.module component and the routing system component.)

Remaining tasks

  1. Better feedback when missing Content-Type request header (and 415 instead of 405 response).
  2. Better feedback when missing/invalid X-CSRF-Token request header (and 401 instead of 403 response). This will be solved in #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response.
  3. Test coverage.

User interface changes

None.

API changes

TBD

Data model changes

TBD

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Issue summary: View changes

neclimdul and I both struggled with this for hours.

Both of us have lots of Drupal 8 experience.

Hence this is a significant DX problem.

Wim Leers’s picture

Title: Upon incorrect PATCH requests to rest.module, unhelpful responses significantly hinder DX » Upon incorrect PATCH requests to rest.module routes, unhelpful responses significantly hinder DX
Wim Leers’s picture

dawehner’s picture

Adding other random related issue. IMHO we should just have a tag, RE (REST experience ): #2594777: Accessing a non existing format results in a 500

Wim Leers’s picture

Issue tags: +API-First Initiative

+1 for a tag, but let's call it RX then, not RE. That'd be in sync with DX, TX …

swentel’s picture

I really hope one day we can tag something with BMX .. ;)

dawehner’s picture

Now you get a 405 (Method Not Allowed) response, with this body: {"message":"No route found for \u0022PATCH \/node\/2\u0022: Method Not Allowed (Allow: GET, POST, DELETE)"}. This makes no sense at all: why is the PATCH method not listed?

Finally figured out the problem here. Unlike content type, request methods are not filtered by a route filter but after that using \Symfony\Component\Routing\Matcher\UrlMatcher::matchCollection. At that point in time, we don't have the list of all routes and so no longer all routes,
and so no methods.

One way could be to add a route filter for request methods, in which case we could throw some form of helpful exception.

Wim Leers’s picture

Let's all cheerlead for @dawehner!

\o/ \o/ \o/ dawehner \o/ \o/ \o/

Thanks so much for debugging that, Daniel!

dawehner’s picture

No its just up to you to actually solve the problem :) Do we want to take the pill and slow down our routing by introducing another route filter?
Maybe that wouldn't be a problem because we just do things we would do anyway but just simply a bit earlier? Kinda tricky questions to ask I'd say.

Should we move this to the routing component instead?

Crell’s picture

Component: rest.module » routing system
Wim Leers’s picture

+1, #8 explains why this is primarily a routing system problem.

Wim Leers’s picture

Crell’s picture

dawehner and I discussed this a bit in IRC, and I did a bit of quick googling (which is not a definitive answer, of course).

The problem is that we're filtering by content type before we filter by method. That is, technically, incorrect as it results in, er, this exact problem. Routes that are not content-type sensitive (like GET and DELETE) will always survive the content-type filter, so we can never get a content-type exception.

The proper fix is to move method filtering before the content type filter by adding a MethodFilter that runs earlier. That way, by the time we get to Content-Type filter we have only the methods for routes that are relevant (PATCH in this case), and so the content type filter will do its thing, end up with an empty list, and throw the appropriate exception.

That's pretty simple to do, actually. The concern is whether there's a performance implication to that, and if we can swallow it. Unfortunately I don't know of any way to figure that out other than to try and and benchmark it, then decide if we can swallow it. If not, then I'm not sure what plan B is.

Anyone want to volunteer?

Wim Leers’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2.9 KB

1. MethodFilter (when both Content-Type and X-CSRF-Token request headers are missing)

Added MethodFilter. This solves point 3 in the problem description/implements point 1 in the proposed solution.

So, with this, if you repeat point 3 in the problem description, you no longer get a 405 (Method Not Allowed) response, with this body: {"message":"No route found for \u0022PATCH \/node\/2\u0022: Method Not Allowed (Allow: GET, POST, DELETE)"}.

Instead, you get a 415 (Unsupported Media Type), which makes sense, because \Drupal\Core\Routing\ContentTypeHeaderMatcher::filter() now runs after (the newly added) MethodFilter and it does:

throw new UnsupportedMediaTypeHttpException('No route found that matches the Content-Type header.');

Note the message there. Sadly, the body you actually get is {}. The tremendously helpful message is gone!

2. ExceptionJsonSubscriber (when both Content-Type and X-CSRF-Token request headers are missing)

The root cause for that helpful message to have disappeared is that ExceptionHalJsonSubscriber doesn't implement on415, and therefore it says I don't know how to generate a response body for 415 HTTP responses with Content-Type HAL+JSON. Implementing it in ExceptionJsonSubscriber means both JSON and HAL+JSON 415 responses can be generated.

3. X-CSRF-Token (when only the X-CSRF-Token request header is missing)

So now that the system is providing actually sensible errors, let's send that Content-Type request header we were missing. As the IS already says, now you get a 403, and {"message":""} as the response body.

This problem still remains to be solved. The root cause of the problem here is that the system is designed to only allow \Drupal\Core\Routing\AccessAwareRouter::checkAccess() to return a response. \Drupal\rest\Access\CSRFAccessCheck::access() is intended to be only allowed to return an access result object (\Drupal\Core\Access\AccessResultInterface). And such an object can not return a useful message.

i.e. AccessAwareRouter::checkAccess() does this:

    $access_result = $this->accessManager->checkRequest($request, $this->account, TRUE);
    …
    if (!$access_result->isAllowed()) {
      throw new AccessDeniedHttpException();
    }

Instead of this:

    $access_result = $this->accessManager->checkRequest($request, $this->account, TRUE);
    …
    if (!$access_result->isAllowed()) {
      throw new AccessDeniedHttpException((string) $access_result);
    }

i.e. if AccessResult implemented __toString() and would allow a helpful indicator message to be set, we could allow a sensible message to be sent.

Alternatively, we could change CSRFAccessCheck::access() to throw new AccessDeniedHttpException('Missing or invalid X-CSRF-Token'), but I'm not sure if that would break/violate the existing API/architecture. I think this will not be acceptable because it would break in case of checking access to e.g. menu links, because in that context you're checking access not for the current request's route, but just for some route.


Conclusion

  1. Forgetting to send the Content-Type request header is fully solved by this patch. (Tests still needed.)
  2. Forgetting to send the X-CSRF-Token (or sending an invalid one) is not yet solved by this patch. Two possible solutions are given: allow AccessResultInterface objects to contain a message, or allow AccessCheckInterface implementations to not return an AccessResultInterface object but throw an exception (questionable.

Status: Needs review » Needs work

The last submitted patch, 15: 2659070-15.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.21 KB
1.17 KB

Status: Needs review » Needs work

The last submitted patch, 17: 2659070-17.patch, failed testing.

Wim Leers’s picture

Issue summary: View changes

Updating IS to reflect current status/remaining tasks.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.4 KB
7.39 KB
+++ b/core/core.services.yml
@@ -885,6 +885,10 @@ services:
+  method_filter:
+    class: Drupal\Core\Routing\MethodFilter
+    tags:
+      - { name: route_filter }

IMHO we should add a higher priority than the other ones.

Alternatively, we could change CSRFAccessCheck::access() to throw new AccessDeniedHttpException('Missing or invalid X-CSRF-Token'), but I'm not sure if that would break/violate the existing API/architecture. I think this will not be acceptable because it would break in case of checking access to e.g. menu links, because in that context you're checking access not for the current request's route, but just for some route.

Yeah that feels odd. What about adding a message support for access result objects and use that? So like a new interface with a new method?

Status: Needs review » Needs work

The last submitted patch, 20: 2659070-20.patch, failed testing.

Wim Leers’s picture

What about adding a message support for access result objects and use that? So like a new interface with a new method?

That's what I thought too. But then I also realized we have __toString(). So we could literally just add a setMessage() method to the existing AccessResult and that'd be okay too. But if we can think of a better name/semantic for that interface, then +1 for such an interface.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/MethodFilter.php
    @@ -25,7 +25,7 @@ public function filter(RouteCollection $collection, Request $request) {
    -    foreach ($collection as $name => $route) {
    +    foreach ($collection->all() as $name => $route) {
    

    Interesting. Why is this better? \Drupal\Core\Routing\ContentTypeHeaderMatcher doesn't use ->all() either.

  2. +++ b/core/modules/rest/src/Tests/UpdateTest.php
    @@ -55,6 +56,13 @@ public function testPatchUpdate() {
    +    return;
    

    This early return means later tests don't run.

  3. +++ b/core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php
    @@ -35,6 +35,19 @@ protected function setUp() {
    +   * Tests on a route with a non supported HTTP method.
    

    s/non supported/non-supported/

  4. +++ b/core/tests/Drupal/Tests/Core/Routing/MethodFilterTest.php
    @@ -7,6 +7,98 @@
    +    $collection->add('test_route3.get', new Route('/test', [], [], [], '', [], ['PUSH']));
    

    PUSH? :P

dawehner’s picture

Interesting. Why is this better? \Drupal\Core\Routing\ContentTypeHeaderMatcher doesn't use ->all() either.

Well, otherwise my IDE doesn't give my autocompletion.

This early return means later tests don't run.

Ha yeah, but it also prooves I executed the tests locally :)

PUSH? :P

Just had a look at the standard, and it seems to be totally possible to come with your own ones:

The set of common methods for HTTP/1.1 is defined below. Although this set can be expanded

, see https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html

Wim Leers’s picture

Just had a look at the standard, and it seems to be totally possible to come with your own ones

:D Works for me!

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.39 KB
2.1 KB

Thank you for your review!

Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs tests

So, now only this remains to be done:

  1. Better feedback when missing/invalid X-CSRF-Token request header.

IS & tags updated.

dawehner++

Status: Needs review » Needs work

The last submitted patch, 26: 2659070-26.patch, failed testing.

Wim Leers’s picture

Actually, the problem wrt X-CSRF-Token is even worse than I thought: #2573537: Confusing http status code returned if operation is failed due to authentication failed (now marked a duplicate) made me realize that the correct response is not a 403, but a 401. https://en.wikipedia.org/wiki/HTTP_401

dawehner’s picture

Well, do you think we should all those problems in this issue or rather split it up?

Wim Leers’s picture

Yeah I was thinking about that too. Splitting up probably makes more sense. This patch solves the first problem (but still needs to pass all tests). Then we could reopen #257537: file_check_directory is verbose on success. to solve the second problem.

What do you think?

dawehner’s picture

Yeah I was thinking about that too. Splitting up probably makes more sense. This patch solves the first problem (but still needs to pass all tests). Then we could reopen #257537: file_check_directory is verbose on success. to solve the second problem.

Even I guess you mean a different issue, I agree.

Wim Leers’s picture

Wim Leers’s picture

I'll get this done somewhere in the next few days, unless somebody beats me to it.

tic2000’s picture

Status: Needs work » Needs review
FileSize
9.4 KB

This is a reroll of the patch in #26 which didn't apply anymore. NR just so it runs the tests to be sure I got it right.

Status: Needs review » Needs work

The last submitted patch, 35: 2659070-26-reroll.patch, failed testing.

tic2000’s picture

The additional fail is because of the change introduced in #2417917: Include content type format name in error response

-    throw new UnsupportedMediaTypeHttpException('No route found that matches the Content-Type header.');
+    throw new UnsupportedMediaTypeHttpException('No route found that matches "Content-Type: ' . $request->headers->get('Content-Type') . '"');
tic2000’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
9.64 KB

This should fix the failures.

Crell’s picture

Yay, working patch. Now who wants to benchmark it to see what the cost is?

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Per #31, splitting up this issue.

But first, #38 introduced this:

+++ b/core/lib/Drupal/Core/Routing/MethodFilter.php
@@ -27,6 +27,11 @@ public function filter(RouteCollection $collection, Request $request) {
+      // If the GET method is allowed we also need to allow the HEAD method
+      // since HEAD is a GET method that doesn't return the body.
+      if (in_array('GET', $supported_methods)) {
+        $supported_methods[] = 'HEAD';
+      }

and I don't understand why. Removing it to see if the patch still passes. Besides, if we add that, we should also update the test coverage.

Status: Needs review » Needs work

The last submitted patch, 41: 2659070-41.patch, failed testing.

tic2000’s picture

Regarding #40 the reason is in the comment. The test that fails does a drupalHead() request and fails since the HEAD request is not allowed. If you change the test to do a drupalGet() the test passes. But we shouldn't change that. We should allow HEAD when GET is allowed cause again as the comment states, a HEAD request is a GET request that doesn't need the body.

Also the fact that the test fails proves that we already have test coverage. Unless we want another one specific for HEAD requests, not one that happens to do a HEAD request and might be changed to a GET later for whatever reason.

Crell’s picture

#40: As the comment says, the access control of HEAD is/should be the same as GET, so anywhere GET is allowed HEAD should be allowed, too. A HEAD is, essentially, just a truncated GET. We should keep that.

alexpott’s picture

Issue tags: +Triaged D8 major

Discussed with @xjm and @catch. We agreed that this is a major bug because the rest API is a key API for Drupal going forward and we need to make it correct in terms of the response it emits and as easy to use as possible.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
9.64 KB
937 bytes

#44 + #45 answered #40. Thanks for those comments, that totally makes sense, and I should've thought of that too!

This reverts #41, so this is effectively identical to #38 again. Let's see if that still passes.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
20.86 KB
19.92 KB
21.84 KB
20.1 KB
+++ b/core/modules/rest/src/Tests/UpdateTest.php
@@ -52,6 +53,12 @@ public function testPatchUpdate() {
+    // Update the entity over the REST API but forget to specify a Content-Type
+    // header, this should throw the proper exception.
+    $this->httpRequest($entity->toUrl(), 'PATCH', $serialized, 'none');
+    $this->assertResponse(Response::HTTP_UNSUPPORTED_MEDIA_TYPE);
+    $this->assertRaw('No route found that matches "Content-Type: none"');

This is the test that proves the DX is now better. It proves that if you forget to specify the Content-Type request header, that you actually get a helpful response.

I wrote the initial implementation of the solution at #15, but a test was still missing. @dawehner added this in #20.

This is what proves that the issue adequately addresses the problem described in the IS.


In fact, @dawehner wrote full unit test coverage and significantly improved what I originally wrote. The patches I posted in #41 and #48 are not rerolls, they only remove one hunk and then add it back again, to better understand why that hunk was there.

Hence I think it's fair for me to RTBC this. On the condition that Crell mentioned in #14: that this doesn't impact performance too much.

Without further ado: here's performance tests that I did. Tested with PHP 5.5.1.1, as the anonymous user, with Page Cache turned off.

A REST route: http://tres/node/1?_format=hal_json
XHProf (best of 3): from 9864 function calls in HEAD to 9888 function calls with this patch: +24 function calls, or +0.2%
ab -c 1 -n 1000 -g ab-rest.tsv http://tres/node/1?_format=hal_json (best of 3)
  • HEAD:
    Requests per second:    27.99 [#/sec] (mean)
    Time per request:       35.722 [ms] (mean)
    Time per request:       35.722 [ms] (mean, across all concurrent requests)
    Transfer rate:          90.27 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       0
    Processing:    30   36   1.4     36      42
    Waiting:       30   36   1.4     36      42
    Total:         30   36   1.4     36      42
    
  • patch:
    Requests per second:    28.03 [#/sec] (mean)
    Time per request:       35.679 [ms] (mean)
    Time per request:       35.679 [ms] (mean, across all concurrent requests)
    Transfer rate:          90.38 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       1
    Processing:    32   36   1.5     35      53
    Waiting:       32   35   1.5     35      53
    Total:         33   36   1.5     35      53
    
  • Histogram:
The frontpage: http://tres/
XHProf (best of 3): from 57394 function calls in HEAD to 57453 function calls with this patch: +59 function calls, or +0.1%
ab -c 1 -n 1000 -g ab-front.tsv http://tres/ (best of 3)
  • HEAD:
    Requests per second:    21.57 [#/sec] (mean)
    Time per request:       46.353 [ms] (mean)
    Time per request:       46.353 [ms] (mean, across all concurrent requests)
    Transfer rate:          647.39 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       0
    Processing:    40   46   1.6     46      56
    Waiting:       35   40   1.5     40      51
    Total:         40   46   1.6     46      56
    
  • patch:
    Requests per second:    21.77 [#/sec] (mean)
    Time per request:       45.929 [ms] (mean)
    Time per request:       45.929 [ms] (mean, across all concurrent requests)
    Transfer rate:          653.37 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       0
    Processing:    39   46   1.8     46      53
    Waiting:       35   40   1.6     40      47
    Total:         39   46   1.8     46      53
    
  • Histogram:
Conclusion
A very small increase in number of functional calls.
A performance difference that falls within the error margin.
Therefore this is acceptable, so I RTBC'd.
Wim Leers’s picture

Title: Upon incorrect PATCH requests to rest.module routes, unhelpful responses significantly hinder DX » REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response

Better title.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/MethodFilter.php
    @@ -0,0 +1,60 @@
    +      // If the GET method is allowed we also need to allow the HEAD method
    +      // since HEAD is a GET method that doesn't return the body.
    +      if (in_array('GET', $supported_methods)) {
    +        $supported_methods[] = 'HEAD';
    +      }
    

    This could be moved to a compile level operation.

  2. +++ b/core/lib/Drupal/Core/Routing/MethodFilter.php
    @@ -0,0 +1,60 @@
    +
    +      // A route not restricted to specific methods allows any method. If this
    +      // is the case, we'll also have at least one route left in the collection,
    +      // hence we don't need to calculate the set of all supported methods.
    +      if (empty($supported_methods)) {
    +        continue;
    

    Could we for this particular usecase actually use a lazy route filter, so we skip some of the executions on runtime?

Crell’s picture

#51.1: Potentially, but that's a separate matter from this fix so I'd say out of scope.

#51.2: I'm unclear what you mean by lazy route filter?

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

#51:

  1. That sounds great.
  2. I had to look this up: the CR is https://www.drupal.org/node/2405829. Sounds great.
Wim Leers’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
10.22 KB

First, a straight reroll, because this did not apply at all anymore.

Furthermore, because this affects every request/response, this is AFAICT considered too big of a change for 8.1. So, moving this to 8.2.

dawehner’s picture

Agreed, its also not something which blocks people from building stuff in 8.1.

Status: Needs review » Needs work

The last submitted patch, 54: 2659070-54.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review

Gosh darn it update...

Wim Leers’s picture

FileSize
2.18 KB
10.87 KB

Addressed #51.2.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

After looking into it more, I agree with Crell's reply to #51.1:

Potentially, but that's a separate matter from this fix so I'd say out of scope.

Especially because it's the standard in Symfony to not explicitly mention HEAD. See for example \Symfony\Component\Routing\Matcher\Dumper\PhpMatcherDumper::compileRoute():

        // GET and HEAD are equivalent
        if (in_array('GET', $methods) && !in_array('HEAD', $methods)) {
            $methods[] = 'HEAD';
        }

In other words, this is just to match that same behavior. So I'm not entirely sure that #51.1 would be appropriate. But even if it is, it'd require special care, and merits a separate discussion, because it must run after \Drupal\Core\EventSubscriber\RouteMethodSubscriber::onRouteBuilding() (which sets the default methods of both GET and POST). That runs with priority 5000. We'd need to be extremely careful with the priority we choose for a new RoutingEvents::ALTER subscriber, that must run after that subscriber, but before certain others. Hence the need for a separate issue.


So, given that, everything in #51 is addressed, and it required only trivial changes (a one-line change), so moving this back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Routing/MethodFilter.php
    @@ -0,0 +1,55 @@
    +      if (in_array('GET', $supported_methods)) {
    ...
    +      elseif (!in_array($method, $supported_methods)) {
    

    Should we make these strict? It always feels safer.

  2. +++ b/core/lib/Drupal/Core/Routing/MethodFilter.php
    @@ -0,0 +1,55 @@
    +      // A route not restricted to specific methods allows any method. If this
    +      // is the case, we'll also have at least one route left in the collection,
    +      // hence we don't need to calculate the set of all supported methods.
    +      if (empty($supported_methods)) {
    +        continue;
    +      }
    

    This can go first no? No need to do the in_array on an empty $supported_methods right?

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.59 KB
10.88 KB
  1. I keep forgetting in_array() has a "strict" parameter. Thanks, done!
  2. Good point. Done :)

Back to RTBC, because this contains only trivial changes.

xjm’s picture

This issue likely has a beta deadline, so tagging for visibility in the RTBC queue.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed af80cb4 and pushed to 8.2.x. Thanks!

  • alexpott committed af80cb4 on 8.2.x
    Issue #2659070 by Wim Leers, dawehner, tic2000, Crell: REST requests...
Wim Leers’s picture

I think this issue/patch shows an aspect of the Symfony routing system that is well-designed: without having to change any existing routing code and by merely registering another route filter, we are able to efficiently, elegantly, succinctly solve an edge case.

Splendid! :)

  • alexpott committed af80cb4 on 8.3.x
    Issue #2659070 by Wim Leers, dawehner, tic2000, Crell: REST requests...

  • alexpott committed af80cb4 on 8.3.x
    Issue #2659070 by Wim Leers, dawehner, tic2000, Crell: REST requests...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.