Problem/Motivation

Over in #1869548: Opt-in CORS support we are trying to get support in for CORS requests. As a part of this, preflight OPTIONS requests are used. The current REST module doesn't support OPTIONS requests.

This is going to impact the ability of Drupal to be used with cross-domain ajax requests.

Proposed resolution

Add support for OPTIONS requests.

How to test

Run commands like the one below on your rest endpoints.

curl --request OPTIONS "http://drupal.d8/node/1?_format=json"

Remaining tasks

Implement #29.

User interface changes

None.

(Except an additional permission for >OPTIONS per entity type.)

API changes

None.

http://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-21#section-5.3.7
http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.2

CommentFileSizeAuthor
#47 interdiff.txt617 bytesdawehner
#47 2237231-47.patch7.95 KBdawehner
#42 2237231-41.patch7.89 KBclemens.tolboom
#36 2237231-36.patch7.85 KBdawehner
#26 interdiff-2237231-23-26.txt1.44 KBclemens.tolboom
#26 support_options_request-2237231-26.patch5.81 KBclemens.tolboom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,197 pass(es). View
#23 support_options_request-2237231-23.patch5.58 KBclemens.tolboom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,037 pass(es). View
#23 interdiff-2237231-21-23.txt1.18 KBclemens.tolboom
#21 support_options_request-2237231-21.patch5.3 KBclemens.tolboom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,029 pass(es). View
#18 support_options_request-2237231-18.patch4.83 KBclemens.tolboom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,029 pass(es), 2 fail(s), and 0 exception(s). View
#14 support_options_request-2237231-14.patch4.82 KBclemens.tolboom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,996 pass(es), 2 fail(s), and 0 exception(s). View
#12 interdiff.txt2.88 KBkim.pepper
#12 2237231-options-request-12.patch4.85 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,756 pass(es), 7 fail(s), and 2 exception(s). View
#8 interdiff.txt3.39 KBkim.pepper
#8 2237231-options-request-8.patch4.16 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#6 2237231-options-request-6.patch2.8 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#1 2237231-options-request-1.patch2.28 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,702 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

kim.pepper’s picture

Status: Active » Needs review
FileSize
2.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,702 pass(es). View

This is an initial stab at adding OPTIONS support. No tests yet, and it assumes that if you can GET you can OPTIONS.

Reviews welcome!

Crell’s picture

This approach would only work for routes generated by REST module. And currently only for entities. That seems too narrow. We have all of the route data available. We should be able to just grab all the available routes at a path, check their legal methods, and return that. It should not be entity coupled.

That said, big +1 on supporting OPTIONS. It's just polite REST behavior.

dawehner’s picture

Issue tags: +Needs tests

Interesting! Do you have some paper/reference you can link to?

This approach would only work for routes generated by REST module. And currently only for entities. That seems too narrow. We have all of the route data available. We should be able to just grab all the available routes at a path, check their legal methods, and return that. It should not be entity coupled.

At least this example in code clearly shows that we need some logic, regarding access for example. Do you suggest to put that directly on top of the routing layer?

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.php
@@ -185,6 +185,32 @@ public function delete(EntityInterface $entity) {
+  public function options(EntityInterface $entity) {
+    if (!$entity->access('view')) {
+      throw new AccessDeniedHttpException();
+    }

I wonder whether we should also somehow expose that in views and just return GET ?

Crell’s picture

I don't think OPTIONS is user-access-sensitive. It's only whether or not there is code to respond to a given method. My understanding is that it's a shorthand for "so what's going to give me a non-405". Not "what's going to give me a 200". So relying on entity access would actually be wrong anyway.

Grayside’s picture

I tried to find some supporting documentation for #4 but it's tricky to google. An argument in it's favor: CORS support brings in the "Access-Control-Max-Age" header as an option, which is a caching directive for OPTIONS responses (http://www.w3.org/TR/cors/#preflight-result-cache). Dynamic access control would probably mess with this cache.

kim.pepper’s picture

FileSize
2.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

After talking with Moshe and Crell at DrupalCon we decided the best approach is a request subscriber. Here's an initial patch which does all the route listening bit. I have no idea where to look for how to determine what methods we can access?? Appreciate any feedback.

Status: Needs review » Needs work

The last submitted patch, 6: 2237231-options-request-6.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
4.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
3.39 KB

I've added calls to the AccessManager, and works with manual tests locally. Need to add tests now.

Status: Needs review » Needs work

The last submitted patch, 8: 2237231-options-request-8.patch, failed testing.

kim.pepper’s picture

Hmmm. Turns out this is not working after all. Running through xdebug on a simple node/1 route shows that a method not allowed exception gets thrown. This is because there are no known routes that support an OPTIONS request.

jibran’s picture

Issue summary: View changes

Added links OPTIONS related docs.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
4.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,756 pass(es), 7 fail(s), and 2 exception(s). View
2.88 KB

I've added support for the OPTIONS response to the entity resource, and now I'm getting a 200 response for options requests. Not sure whether accessmanager is returning the right results, because all methods are being returned in the Allow header.

Status: Needs review » Needs work

The last submitted patch, 12: 2237231-options-request-12.patch, failed testing.

clemens.tolboom’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
4.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,996 pass(es), 2 fail(s), and 0 exception(s). View

Patch needed a reroll.

Enabling the OPTIONS for content then testing

$ curl --user admin:admin --header 'Accept: application/hal+json' --request OPTIONS http://drupal.d8/node/1 gives HTML output with an 'access denied'. Hal module was disabled.

curl --user admin:admin --header 'Accept: application/json' --request OPTIONS http://drupal.d8/node/1
{}

gives an empty json.

$ curl --user admin:admin --request OPTION http://drupal.d8/node/1
Method Not Allowed

I expected list of methods GET and authentication methods plus formats.

Status: Needs review » Needs work

The last submitted patch, 14: support_options_request-2237231-14.patch, failed testing.

clemens.tolboom’s picture

Hmmm ... my result comes from an exception.

Event dispatching takes core/lib/Drupal/Core/Routing/AccessAwareRouter.php before this core/modules/rest/src/Routing/OptionsRequestSubscriber.php (rest.options_subscriber)

There is iirc an issue about event ordering.

clemens.tolboom’s picture

clemens.tolboom’s picture

Issue summary: View changes
Status: Needs work » Needs review
Related issues: -#2344645: The order of event subscribers should not be affected by definition order
FileSize
4.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,029 pass(es), 2 fail(s), and 0 exception(s). View

Reading #2344645: The order of event subscribers should not be affected by definition order is about priority and this patch has a priority of -10000 which is in weight terms first but on priority last.

Changed that number to 1000 which gives at least a debug step into onKernelRequest with
curl --verbose --header 'Accept: application/json' --request OPTIONS http://drupal.d8/node/1?XDEBUG_SESSION_START=19350 but the request is not complete.

+++ b/core/modules/rest/src/Routing/OptionsRequestSubscriber.php
@@ -0,0 +1,102 @@
+  protected function getAllowedMethods(Request $request) {
+    $allow = array();
+    $route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);
+    if (isset($route)) {
+      foreach ($this->availableMethods as $method) {
+        $request->setMethod($method);
+        if ($this->accessManager->check($route, $request, $this->account)) {
+          $allow[] = $method;
+        }
+      }
+    }
+    return $allow;
+  }

This result keeps empty so we need some tunig for the priority number

Status: Needs review » Needs work

The last submitted patch, 18: support_options_request-2237231-18.patch, failed testing.

clemens.tolboom’s picture

array_keys($this->listeners['kernel.request'])

gives 9 values out of 12 listeners. The choosen '1000' in patch above is '200' and then '50' '35' '32' ... so what should it be ?!?

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
5.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,029 pass(es). View

Fix missing schema. How could I miss that :-/

dawehner’s picture

  1. +++ b/core/modules/rest/src/Routing/OptionsRequestSubscriber.php
    @@ -0,0 +1,103 @@
    +/**
    + * Handles OPTIONS requests.
    + */
    +class OptionsRequestSubscriber implements EventSubscriberInterface {
    +
    

    It would be great to explain why we cannot implement this as ordinary route but need a request subscriber.

  2. +++ b/core/modules/rest/src/Routing/OptionsRequestSubscriber.php
    @@ -0,0 +1,103 @@
    +    if ($request->getMethod() == 'OPTIONS') {
    

    you can use isMethod() here.

  3. +++ b/core/modules/rest/src/Routing/OptionsRequestSubscriber.php
    @@ -0,0 +1,103 @@
    +      foreach ($this->availableMethods as $method) {
    

    I am confused, this property is not set anywhere.

clemens.tolboom’s picture

Issue summary: View changes
FileSize
1.18 KB
5.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,037 pass(es). View

#1 Added some documentation. Not sure whether that's enough. There is AFAIK no generic route as the OPTIONS methods matches any URI. See links to w3c in the summary.

#2 Done

#3 It's a class protected $availableMethods = array(

New issue:

When debugging with ie curl --verbose --header 'Accept: application/json' --request OPTIONS http://drupal.d8/node/1?XDEBUG_SESSION_START=16522

on a configured rest there still is no route found in

    $route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);

This needs a route expert.

dawehner’s picture

+++ b/core/modules/rest/src/Routing/OptionsRequestSubscriber.php
@@ -18,6 +18,12 @@
  * Handles OPTIONS requests.
+ *
+ * Option request are allowed regardless of access permissions on
+ * the requested resource and can be requested on any path.
+ *
+ * For example requesting OPTIONS on /node/1 could result into list of methods
+ * depending permissions on other methods like GET, POST, PATCH, DELETE.
  */
 class OptionsRequestSubscriber implements EventSubscriberInterface {

Yeah this is somehow important information, maybe also link to the w3c page.

+ $events[KernelEvents::REQUEST][] = array('onKernelRequest', 1000);

In this case use priority < 32

clemens.tolboom’s picture

@dawehner why that magic < 32 ? It should be one of the highest values as OPTIONS must run before access check.

I failed to find the Access :: onKernelRequest implementation. Guess I'm searching wrong :-/

clemens.tolboom’s picture

FileSize
5.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,197 pass(es). View
1.44 KB

Based on the list below I guess we need to be @ 32+/- 0.5 . Note OPTIONS does not have to do an access / permission check.

kernel.request
1000 rest.options_subscriber::onKernelRequest
200 path_subscriber::onKernelRequestConvertPath
50 ajax_subscriber::onRequest
50 ajax.subscriber::onKernelRequest
40 hal.subscriber::onKernelRequest
35 user_maintenance_mode_subscriber::onKernelRequestMaintenance
32 router_listener::onKernelRequest
31 route_content_controller_subscriber::onRequestDeriveFormat
30 route_content_controller_subscriber::onRequestDeriveContentWrapper
30 maintenance_mode_subscriber::onKernelRequestMaintenance
29 route_content_form_controller_subscriber::onRequestDeriveFormWrapper
0 router.route_preloader::onRequest
0 replica_database_ignore__subscriber::checkReplicaServer

Running

drush @drupal.d8 cache-rebuild
curl --verbose --header 'Accept: application/json' --request OPTIONS http://drupal.d8/node/1

gives either an empty Allow header field or {"error":"A fatal error occurred: No authentication credentials provided."}
depending on values above 33 (empty Allow) or 32 and below (error).

Adding authentication

curl --verbose --user admin:admin --header 'Accept: application/json' --request OPTIONS http://drupal.d8/node/1

gives an HTML exceptions response.

Recoverable fatal error: Argument 1 passed to Drupal\Core\Access\AccessManager::check() must implement interface Drupal\Core\Routing\RouteMatchInterface, instance of Symfony\Component\Routing\Route given, called in /Users/clemens/Sites/drupal/d8/www/core/modules/rest/src/Routing/OptionsRequestSubscriber.php on line 100 and defined in Drupal\Core\Access\AccessManager->check() (line 219 of core/lib/Drupal/Core/Access/AccessManager.php).

Drupal\Core\Access\AccessManager->check(Object, Object, Object)
Drupal\rest\Routing\OptionsRequestSubscriber->getAllowedMethods(Object)
Drupal\rest\Routing\OptionsRequestSubscriber->onKernelRequest(Object, 'kernel.request', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.request', Object)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

New patch
- adds puzzle to use the accept header but lacks to use this information. Mimetype is hardcoded to json. But that should use ie xml when requested. Not sure whether that is correct.
- lowers the priority from 1000 to 32 to see whether there are testbot failures too apart from mentioned exception.

clemens.tolboom’s picture

Still learning about priorities. $ grep -r -A 2 " tags:" . | grep priority helps to find some but still no luck with level 35, 34 ... guess those are done through altering the prio?

Open puzzle is how to tweak the priority so it runs before access check. See Remaining tasks

dawehner’s picture

Open puzzle is how to tweak the priority so it runs before access check. See Remaining tasks

The answer is, you cannot anymore. Access checking is hardcoded inside AccessAwareRouter.

Crell’s picture

Status: Needs review » Needs work
Issue tags: +rest-critical

Daniel, Kalpana, and I discussed this at BADCamp. We need to support all routes, not just rest.module's routes.

The way to do that is to have a request listener that fires before routing (probably right before). If the method is not OPTIONS, do nothing. If it is, then it looks up available routes in the RouteProvider, does a union of all of their legal methods, and returns a response listing those. Routing then never happens, controllers never happen, etc.

The RouteProvider will still only be called once, EITHER by this new listener or by normal routing. Never both. So the performance impact should be negligible. (It's just one more request listener that is usually a no-op.)

AlxVallejo’s picture

Can we get a status update for this? My understanding is that CORS requires preflight negotiation with the OPTIONS method and that this is not natively a part of core or the CORS bundle. I'm surprised there's no Issues tab on the github repo for the CORS module either. https://github.com/previousnext/drupal_cors

CORS seems like mere necessity for decoupled applications and I've yet to resolve this in Drupal 8.

Wim Leers’s picture

Issue summary: View changes
Priority: Normal » Major
Status: Needs work » Active

Updated IS based on #29. This seems at least major, for the reasons outlined in #30.

Wim Leers’s picture

Issue tags: +API-First Initiative
endorn’s picture

Am I reading this right that drupal 8 doesn't support CORS requests? Thats... wow... That's a critical issue for the rest module isn't it?

Wim Leers’s picture

Critical == "data loss, completely not working". CORS is not like that. The next priority level is "major". This issue is marked "major".

But I completely agree this is critical for REST. And that's also why this has the "rest-critical" issue tag. It's sad/unfortunate that D8's REST support doesn't yet have CORS. But this is open source, with people contributing in their free time.

If you'd like to help out with getting Drupal 8's REST to support CORS, that'd be great :) I'd be happy to provide reviews.

dawehner’s picture

Looking into #29 now, yeah!

dawehner’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Active » Needs review
FileSize
7.85 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, 36: 2237231-36.patch, failed testing.

The last submitted patch, 36: 2237231-36.patch, failed testing.

Wim Leers’s picture

Looks great :)

  1. +++ b/core/core.services.yml
    @@ -1022,6 +1022,11 @@ services:
    +    class: Drupal\Core\EventSubscriber\OptionsRequestListener
    

    Why "listener" and not subscriber"?

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/OptionsRequestListener.php
    @@ -0,0 +1,71 @@
    +      // In case we don't have any routes, a 403 should be thrown by the normal
    +      // request handling.
    +      if (count($routes) > 0) {
    +        $methods = array_map(function (Route $route) {
    +          return $route->getMethods();
    +        }, $routes->all());
    +        // Flatten and unique the available methods.
    +        $methods = array_unique(call_user_func_array('array_merge', $methods));
    +        $response = new Response('', 200, ['Allow' => implode(', ', $methods)]);
    +        $event->setResponse($response);
    +      }
    

    Should this also do access checking on all those routes? That probably doesn't make sense because additional metadata may need to be sent with the request for access to be granted.

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/OptionsRequestListener.php
    @@ -0,0 +1,71 @@
    +    $events[KernelEvents::REQUEST][] = ['onRequest', 1000] ;
    

    Why 1000?

  4. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/OptionsRequestListenerTest.php
    @@ -0,0 +1,114 @@
    +  public function providerTestOnRequestWithOptionsRequest() {
    

    Nice :)

clemens.tolboom’s picture

dawehner pointed out we allow GET + POST on almost any page which is done by #2019123: Use the same canonical URI paths as for HTML routes in \Drupal\Core\EventSubscriber\RouteMethodSubscriber::onRouteBuilding

[edit]WimLeers foundRelated[/edit] #2505339: Stop using getMasterRequest() to build $form['#action'] as the offender for always GET + POST

It is weird to POST to a view until one realizes it could have filters etc. requiring a post.

curl --request OPTIONS --verbose "http://drupal.d8/node" 2>&1 | grep Allow 
< Allow: GET, POST

I've tested a few router.pattern_outlines of which node/%, user/% fail on node/add, user/login aka string argument variants of nid or uid.

curl --request OPTIONS --verbose "http://drupal.d8/node/add" 2>&1 | grep Allow
< Allow: GET, POST, DELETE, PATCH

curl --request OPTIONS --verbose "http://drupal.d8/user/login" 2>&1| grep Allow
< Allow: GET, POST, DELETE, PATCH

gives false results as one cannot DELETE, PATCH node/add or user/login. Similar goes for

curl --request OPTIONS --verbose "http://drupal.d8/rss.xml" 2>&1| grep Allow
< Allow: GET, POST
curl --request OPTIONS --verbose "http://drupal.d8/user/%/cancel/confirm" 2>&1| grep Allow
curl --request OPTIONS --verbose "http://drupal.d8/user/x/cancel/confirm" 2>&1| grep Allow 
< Allow: GET, POST
curl --request OPTIONS --verbose "http://drupal.d8/quickedit/entity/%/%" 2>&1| grep Allow
curl --request OPTIONS --verbose "http://drupal.d8/quickedit/entity/p/q" 2>&1| grep Allow 
< Allow: GET, POST

I guess this patch is good to go but building an API scan based on OPTIONS result is futile.

dawehner’s picture

WimLeers found #2505339: Stop using getMasterRequest() to build $form['#action'] as the offender for always GET + POST

MH, I think we still need POST on most routes, otherwise people cannot just embed random forms in sidebars and what not.

I guess the reason for this strange behaviour is that /node/add is matched by /node/{node}, as long you don't apply the regex.

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
7.89 KB

@WimLeers agreed : in #26 we had OptionsRequestListener (in a complete different namespace)

Should we anticipate in our tests for mentioned default POST result?

Attached patch contains the renaming.

Probably of topic aka ReST API related: I'm puzzled regarding _format. What if I configure POST only for hal+json? https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.2 is unclear to me as they require a body when using 'Content-Type'

Wim Leers’s picture

I didn't say #2505339: Stop using getMasterRequest() to build $form['#action'] was the offender for GET+POST. That's a completely separate thing.

clemens.tolboom’s picture

@Wim-leers: I've corrected in #40 ... sorry about that.

@dawehner I rather had the argument validated for a node/% rather then a match on node/add. But I guess we could RTBC this now right?

dawehner’s picture

Regarding the naming, I couldn't care less. A listener is for me more about the concrete function which gets called, while a subscriber also specifies what it subscribes to. The RouterListener just came next in the core.services.yml file.

One remaining thing we could implement: Some form of configuration to enable options requests, but I guess given how fast they are responded there is no real a reason to make it toggleable.

@clemens.tolboom
I'm fine with some RTBC

Wim Leers’s picture

RTBC as soon as Why 1000? is answered by a code comment.

dawehner’s picture

Sure

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
+++ b/core/lib/Drupal/Core/EventSubscriber/OptionsRequestSubscriber.php
@@ -64,6 +64,7 @@ public function onRequest(GetResponseEvent $event) {
+    // Set a high priority so its executed before routing.

Ah, so that's the reason. Okay :)

Nit: s/its/it's/, can be fixed on commit.

clemens.tolboom’s picture

Issue summary: View changes

Is this for 8.1.x or should it go for 8.2.x?

(+ a IS fix on curl command)

dawehner’s picture

Well, ideally it is for both, but I fear at least CORS is certainly a feature so we missed the 8.1.x deadline.

kim.pepper’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/OptionsRequestSubscriber.php
@@ -0,0 +1,72 @@
+   *   THe route provider.

Very minor nitpick. (THe -> The)

mikl’s picture

This looks pretty much perfect, hope it gets in soon.

Crell’s picture

I'm +1 here as well, with the two on-commit-able tweaks. Thanks all!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This seems like a good candidate for 8.1.x-beta - it is new functionality this is isolated and should not break anything. Committed a4e5372 and pushed to 8.1.x and 8.2.x. Thanks!

diff --git a/core/lib/Drupal/Core/EventSubscriber/OptionsRequestSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/OptionsRequestSubscriber.php
index 622fe23..17ef946 100644
--- a/core/lib/Drupal/Core/EventSubscriber/OptionsRequestSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/OptionsRequestSubscriber.php
@@ -23,6 +23,8 @@
 class OptionsRequestSubscriber implements EventSubscriberInterface {
 
   /**
+   * The route provider.
+   *
    * @var \Symfony\Cmf\Component\Routing\RouteProviderInterface
    */
   protected $routeProvider;
@@ -30,11 +32,11 @@ class OptionsRequestSubscriber implements EventSubscriberInterface {
   /**
    * Creates a new OptionsRequestSubscriber instance.
    *
-   * @param \Symfony\Cmf\Component\Routing\RouteProviderInterface $routeProvider
-   *   THe route provider.
+   * @param \Symfony\Cmf\Component\Routing\RouteProviderInterface $route_provider
+   *   The route provider.
    */
-  public function __construct(RouteProviderInterface $routeProvider) {
-    $this->routeProvider = $routeProvider;
+  public function __construct(RouteProviderInterface $route_provider) {
+    $this->routeProvider = $route_provider;
   }
 
   /**
@@ -64,8 +66,8 @@ public function onRequest(GetResponseEvent $event) {
    * {@inheritdoc}
    */
   public static function getSubscribedEvents() {
-    // Set a high priority so its executed before routing.
-    $events[KernelEvents::REQUEST][] = ['onRequest', 1000] ;
+    // Set a high priority so it is executed before routing.
+    $events[KernelEvents::REQUEST][] = ['onRequest', 1000];
     return $events;
   }
 
diff --git a/core/tests/Drupal/Tests/Core/EventSubscriber/OptionsRequestSubscriberTest.php b/core/tests/Drupal/Tests/Core/EventSubscriber/OptionsRequestSubscriberTest.php
index f1c6296..7c0e454 100644
--- a/core/tests/Drupal/Tests/Core/EventSubscriber/OptionsRequestSubscriberTest.php
+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/OptionsRequestSubscriberTest.php
@@ -103,7 +103,7 @@ public function providerTestOnRequestWithOptionsRequest() {
           $collection->add('example.2', new Route('/example', [], [], [], '', [], [$method_a, $method_b]));
           $collection->add('example.3', new Route('/example', [], [], [], '', [], [$method_b, $method_c]));
           $methods = array_unique([$method_a, $method_b, $method_c]);
-          $data['multiple_routes_' . $method_a . '_' . $method_b . '_' . $method_c] = [$collection,  implode(', ', $methods)];
+          $data['multiple_routes_' . $method_a . '_' . $method_b . '_' . $method_c] = [$collection, implode(', ', $methods)];
         }
       }
     }

Nits fixed on commit.

  • alexpott committed 84879a8 on 8.2.x
    Issue #2237231 by clemens.tolboom, kim.pepper, dawehner, Wim Leers,...

  • alexpott committed a4e5372 on 8.1.x
    Issue #2237231 by clemens.tolboom, kim.pepper, dawehner, Wim Leers,...

Status: Fixed » Closed (fixed)

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