Opt-in CORS support

I think this is a REST module thing in particular.

Anything that says "Allow: *" automatically makes me nervous from a security perspective. I'm not fully versed in CORS so I don't know if that's warranted, but tagging for the security team to get their opinion.

That makes total sense, but only makes sense from a security perspective if you use for those requests an authentication mechanism that is separate from the one used by the standard browser-based requests. For example, it works well for Facebook / Google / Twitter using the oAuth2 "Implicit grant" authorization flow.

It will not work from a security perspective if we cannot mandate cross-domain requests to use a different authentication mechanism.

This needs to be postponed until:

• We have any other authentication mechanism in core that is useful for cross-domain REST requests (oAuth2 implicit grant is what the big boys use, so we should aim at one);
• We have useful authorization mechanisms for the REST endpoints, we do not need to have full-fledges scopes, but at the minimum the REST endpoints need to respect field-level access control.

Allowing GET while still respecting cookie authentication will open the site to information disclosure. That's just not acceptable.

Access-Control-Allow-Credentials defaults to false, so this just returning Access-Control-Allow-Origin: * to GET requests seems sufficient.

Implementation looks good, suppose we needs a unit test for that

#8 is just a first attempt :) We only want the header for the REST module routes.

The CDN module does this automatically for publicly available static files. (Necessary to e.g. make fonts in Firefox work if they're hosted on a different domain.) My experience is limited to that extremely simple case, so I don't think I can contribute much to this discussion.

@linclark: yes, Access-Control-Allow-Credentials being false is both by design and desirable.

Just an additional note here: we also need to explicitly allow some (response) headers with Access-Control-Expose-Headers. (At the minimum, the Location header.)

Also, the CORS implementation of most if not all of the current browsers is severely broken in many fun and interesting ways.

#13: links for interesting & fun brokenness? :)

This may be related to CORS: Chrome needs to make preflight requests using OPTIONS http verb, but I can't see any support for OPTIONS in the rest module.

More info: https://dvcs.w3.org/hg/cors/raw-file/tip/Overview.html#preflight-request

I've ported the contrib CORS module to D8 over here: https://github.com/previousnext/drupal_cors

I'm building a production site in d8 (I know I'm crazy ;P ) and I need CORS, so this is what I have so far...

Please comment!

+++ b/core/lib/Drupal/Core/Routing/RouteSubscriberBase.php
@@ -43,4 +46,19 @@ public function onAlterRoutes(RouteBuildEvent $event) {
+    watchdog('rest', 'REST CORS ALLOWED!! 2');

deprecated

Oh snap! I was using that for debugging. That shouldn't be in there!

@yanniboi, kim.pepper
Your implementation sure work due to the flexibility of the kernel events.
Using the stack approach would thought have a couple of advantages:

• No code has to be written, https://github.com/asm89/stack-cors/blob/master/src/Asm89/Stack/CorsServ...
  There is quite some non-straightforward code here
• You can replace that with an alternative service not using PHP but for example apache, see http://enable-cors.org/server_apache.html

  Yes, you can do that already, but using the stack is a formal definition to ensure that you just handle on the request/HTTP level and nothing else, so you are sure that you can be replaced by a different HTTP speaking object, like apache.

@dawehner. Looks good. I'd like to know whether there is support for getting this into core before I'd start integrating with the contrib CORS module.

I'm creating a new version of my library https://github.com/enzolutions/backbone.drupal to enable use Headless Drupal using front end in a different frontend domain.

The problem I found with this kind of CORS request explained about is related with how jQuery Ajax execute the first request, because before to do a GET|PUT|DELETE|POST|PATCH request an OPTIONS request is executed to validate request methods accepted for backend server[1].

The current status of Rest module doesn't implement the OPTIONS, so my first approach was implement that method in class Drupal\rest\Plugin\rest\resource\EntityResource using the following code

public function options(EntityInterface $entity) {
   $response = new ResourceResponse();
   $response->setStatusCode(ResourceResponse::HTTP_OK);
   $response->headers->set('Allow', 'GET,POST,PATCH,DELETE,OPTIONS');
   return $response->send();
  }

After that extra function in class EntityResource the method was available and double checked with module RestUI https://www.drupal.org/project/restui, but this solution wasn't enough because I can't enable OPTIONS method without authentication and jQuery trigger the method without CRC or Basic Auth method even if you provide in call ( I will create a issue in RestUI to request the option to enable a method without authentication).

So to resolve I follow the recommendation of @dawehner in comment #24 to use .htacess to enable CORS and intercept the OPTIONS request to avoid Drupal reject the request because doesn't have the proper Basic Auth credentials, the patch is attached but let me list the changes.

1. Rule to avoid Drupal process the options request

# Intercept OPTIONS calls
RewriteCond %{REQUEST_METHOD} OPTIONS
RewriteRule .* / [R=200,L]

2. Rules to enable CORS and send the proper headers even if interception occurs

Header always set Access-Control-Allow-Origin "*"
Header always set Access-Control-Allow-Methods "POST, GET, OPTIONS, PATCH, DELETE"
Header always set Access-Control-Allow-Headers: Authorization

I recommend to change * for your frontend domain i.e frontend-example.com

[1] http://api.jquery.com/jquery.ajax/

We can implement this in stack by just pulling in the stack library and adding a few lines to our core.services.yml:

  http_middleware.cors:
    class: Asm89\Stack\Cors
    arguments: [{ allowedHeaders: [*], allowedMethods: ['DELETE', 'GET', 'POST', 'PUT'], allowedOrigins: [*] }]
    tags:
      - { name: http_middleware }

If we can do this, it's more preferable to an htaccess solution? Although no doubt that works. Also, any of those parameters could be easily overridden.

Part of the question, I think, is whether we want this enabled by default or just easily enabled. And if easily enabled, how?

It would be great if someone who actually deal with headless drupal could test whether this patch works. On top of that some basic integration test would be cool, so that we really know that CORS is running.

+++ b/core/core.services.yml
@@ -394,6 +394,11 @@ services:
+    tags:
+      - { name: http_middleware }

so do we really want to run this after page caching? just thinking loud

If we can do this, it's more preferable to an htaccess solution? Although no doubt that works. Also, any of those parameters could be easily overridden.

Well even this is really nice, having it on .htaccess itself is much faster, if you are honest but well you can still
replace the middleware and use apache, if you know what you do.

Hi, I change this patch to create a wrapper to create an instance of Cors class.

Do you have an interdiff? and why do we need that? It seems the Drupal namespaced Cors wrapper has no benefit at all?

The wrapper is necessary for libraries that are implemented if they change, you can also put a more level configuration, an interface that allows you to manage the domains that can access your content.

I'm sorry but we don't wrap exteral libraries like that atm.
You could easily put this domain configuration into the container as well, no need for another class.

Also, our dependencies would not get just get updated if there was such a big change. If that was the case it would be broken either way.

@dmouse
On top of that, by definition stack implementations have one inderface, that will not change.

Right.

@dmouse, any work that allow this to be configurable should be a follow up issue.

Here is a reroll of #27.

@dmouse, any work that allow this to be configurable should be a follow up issue.

Agreed. If you can come up with good usecase, sure, try to use one.

+++ b/core/core.services.yml
@@ -394,6 +394,11 @@ services:
+    arguments: [{ allowedHeaders: [*], allowedMethods: ['DELETE', 'GET', 'POST', 'PUT'], allowedOrigins: [*] }]

I really don't think that allowing all origins should be the default behaviour. I would expect some discussion of security on this issue and considering the nature of the change and the fact we are enabling it by default I would expect a change record.

I would agree that default-allow-world is a bad idea. So how do we want to enable allowing certain origins, and/or all origins? Ie, what's the API we want to expose for that?

Options I see are:

1) Edit .htaccess
2) Edit services.yml (can we do it there?)
3) Write a compiler pass

If we can make it work option 2 would be my preference.

Plain re-roll of the patch from #39.

Tried to a) add test coverage b) does solution 2)

Updated the patch to work on latest Drupal 8 HEAD, fixed the broken tests.

Before we keep rolling patches, does anyone have an opinion on #45? What patches do we even want to be rolling here?

I would vote for possibility to control these headers per route. For example by adding special tag or something to the route and then this route will get all headers according to configuration of cors.

@Crell I would definitely vote for option 2. It makes total sense to configure generic components with container parameters. This way you can override the settings both manually and modules can do it automatically, by writing their own compiler pass.

@ygerasimov I like that idea. But this is something that contrib could implement with a special compiler pass. I would vote for this initial core implementation to be simple and straight forward, e.g. just using one central container parameter like the patch does now.

I really don't think that allowing all origins should be the default behaviour. I would expect some discussion of security on this issue and considering the nature of the change and the fact we are enabling it by default I would expect a change record.

Just to be clear, the new default is basically the default behaviour without CORS. Maybe we should look inside settings.php.
In case there is a $base_urlset we could allow that by default?

If we use the Stack lib, then the services.yml approach would likely involve setting a parameter that is checked by a compiler pass, which will then register/not register the middleware and configure it. Eg, a parameter that specifies what the legal origins are. If none or not set, then just don't enable the middleware.

Alex, would that be sufficiently tight, security-wise?

re #54 that sounds fine

There we go.

There we go.

1. +++ b/sites/default/default.services.yml
   @@ -51,3 +51,20 @@ parameters:
   +  # Allow to configure Cross-Site HTTP requests (CORS).
   

   Ah, German idioms. :-) Try just "Configure Cross-Site HTTP requests (CORS)"

2. +++ b/sites/default/default.services.yml
   @@ -51,3 +51,20 @@ parameters:
   +  # Note: By default the configuration is done in a way that no behaviour is
   +  # changed.
   

   I don't understand this sentence. Do you mean "by default CORS is disabled"? (That's what it looks like from line 62.)

Other than those doc points I think this is good.

Also assigning to Dries for when it gets RTBCed; it has a new library so he has to commit it. Updating summary for beta considerations.

Rerolled and adapted for the docs suggestions of @Crell (yeah there has been some grammer lacks here)

Didn't fix the #59.1.

+++ b/core/modules/system/src/Tests/HttpKernel/CorsIntegrationTest.php
@@ -0,0 +1,56 @@
+    $this->assertEqual([], $cors_config['allowedHeaders']);
+    $this->assertEqual([], $cors_config['allowedMethods']);
...
+    $this->assertEqual(FALSE, $cors_config['allowedHeaders']);
+    $this->assertEqual(FALSE, $cors_config['allowedMethods']);

This seems to just be asserting the same thing twice.

[] == false is true
false == false is true

Do you want to just remove the second lot and use assertIdentical on the first? Actually maybe assertIdentical on all the options there?

Adressed both points.

Gift for the headless folks...

Looks like security was addressed in #7 and #55. Here's also a -do-not-test patch sans core/vendor and composer.lock for easier review of the changes to Drupal.

Added link to the https://github.com/asm89/stack-cors library in the issue summary.

I think this is valuable, but it is ultimately a feature request. It's not a bug, because REST applications can still be built as native apps or javascript apps on the same domain. We're not adding new features at this point of the release so I'm postponing this to 8.1.x. Would love to see it in 8.1.x though. In the meantime, this can work perfectly well as a contributed module, right?

Updating the summary based on #67.

There is also apparently another method not involving a module... http://enzolutions.com/articles/2014/09/08/how-to-enable-cors-requests-a...

I think this is valuable, but it is ultimately a feature request. It's not a bug, because REST applications can still be built as native apps or javascript apps on the same domain. We're not adding new features at this point of the release so I'm postponing this to 8.1.x. Would love to see it in 8.1.x though. In the meantime, this can work perfectly well as a contributed module, right?

Fair, hopefully someone does it.

Its just sad because that issue would not had any disruption.

I've created a module for that. Any improvments are welcome

https://www.drupal.org/sandbox/chr.fritsch/2460591

@chr.fritsch there is already a module https://www.drupal.org/project/cors with a pending #2328985: Drupal 8 port for CORS

Thanks for the info

I am interested in this because I am writing a book about CORS, including a chapter about using CORS in popular CMSs.

Here are some thoughts.

jQuery Supports Limited CORS

Drupal 8 already supports CORS via jQuery in core/assets/jquery/jquery.js|jquery.min.js
support.cors = !!xhrSupported && ( "withCredentials" in xhrSupported );

However, CORS via jQuery is very limited, it does not support preflight without additional custom code. Preflight is often needed for requests that are not simple, and can improve security.

Adding CORS Headers With the CORS Module

The CORS module is a handy way to map headers required for CORS to Drupal paths. I think its most appropriate use is when integrating a service or API that requires addition of CORS headers, without additional custom coding.

An interesting feature of the CORS module is the <mirror> token that will add the domain from a request that sends the Origin header to the Access-Control-Allow-Origin header. This is like a slightly limited use of the wildcard, because requests without the Origin header will not be allowed.

CORS Should Be Implemented With Custom Code

In most cases, CORS should be implemented very carefully and with additional logic to validate and process the requests, including preflight when needed. This assumes custom code, probably in a custom module. In a custom implementation, the headers should be managed in that code, which makes the CORS module unnecessary for these cases.

Limited Support for Multiple Domains in the Access-Control-Allow-Origin Header

The W3C specification defines a space-separated list of allowed domains. However, multiple allowed domains are not yet supported in all clients.

That is the reason many people suggest using the wildcard "*" but that allows requests from ALL domains. For security additional processing of requests, and perhaps preflight, are needed when using the wildcard.

Recommendations for Including the Access-Control-Allow-Origin Header in Drupal Core

Therefore if the Access-Control-Allow-Origin header is to be added in Drupal core, I recommend:

1. The header is not added by default, it should be a configuration option.
2. As part of the configuration option, the allowed domain(s) could be configured.
3. Rather than adding the CORS header(s) globally, mapping to paths similar to the function in the CORS module should be considered.
4. If the wildcard is used, there should be a warning about the risks, and link(s) to documentation for security best practices in CORS.
5. If the wildcard is used globally, then any additional header added for a specific path will trigger a warning about trying to send additional headers. A message or warning should accompany configuration with the wildcard saying additional headers can not be set. (BTW, This warning can also be triggered by improper configuration of the CORS module.)
6. Consider that Access-Control-Allow-Origin is not the only header needed for any but the most simple uses of CORS. The CORS module can also add additional headers:
   1. Access-Control-Allow-Methods
   2. Access-Control-Allow-Headers
   3. Access-Control-Allow-Credentials

   For a complete implementation of CORS in Drupal core, configuration adding additional headers when needed should be considered.

What About JSON-P?

In some cases cross-origin requests may be implemented with JSON-P. It is much simpler, and less secure, than using CORS.

There is a Drupal 7 CORS custom module that doesn't look like it is being assiduously maintained or updated to Drupal 8 on the module site.

However, someone else on Github has contributed an update for Drupal 8. I would suggest that it be posted over to the DRUPAL archive as the

https://www.drupal.org/project/cors (Drupal 7)

https://github.com/piyuesh23/cors (Drupal 8) I added a fix for D8 Beta 10

Someone should contact piyuesh23 about moving it over.

@decibel.places thanks it is useful, also @pkosenko thanks for a heads-up. I was getting the same error reported
https://github.com/piyuesh23/cors/issues/4
However your patch is good enough, I am working with Drupal 8.0.0-beta14 then I updated implementation of "CorsConfigForm":
Added the following lines:

/**
   * {@inheritdoc}
   */
  protected function getEditableConfigNames() {
    return [
      'cors.config'
    ];
  }

And overwrite the way to retrieve configuration:
$config = $this->configFactory->getEditable('cors.config');

0n Drupal-8.0.0 beta 16 I applied the patch made by @pkosenko and it worked perfectly.

Well, this won't land in 8.1.x, well at lest the next version has a chance for that.

@pkosenko + @keboca + @constrict0r as we have a patch in #63 it would be better to reroll the patch instead of pointing to remote sites. Regarding contrib cors see #2328985: Drupal 8 port for CORS

+++ b/core/lib/Drupal/Core/CoreServiceProvider.php
@@ -51,6 +52,8 @@ public function register(ContainerBuilder $container) {
+    $container->addCompilerPass(new CorsCompilerPass());

I'd better make it into module to easy override from contrib and make configurable

This is a reroll, doesn't contain changes from #80

Let's ask the testbot about it!

We should add the additional library in its issue and integrate it in here.
This way we can iterate quicker.

Fixed patch #81

Let's try it again.
Note: We are using a kernel test now.

Any reason why we need the installed.json file in the patch? That's where it's failing. According to some sources, this file shouldn't be committed to the repository since it's essentially a documentation of what packages are installed on your instance.

Here's the same patch without the installed.json.

One note is that if I set exposedHeaders to true, I get a 403 on just about every request.

I'll try it as well.

Let's try a binary one.

No idea why this doesn't apply to be honest.

Is this not blocked on #2237231: Support OPTIONS request ?

Afaik OPTIONS requests are not always requested, just in one of many cases, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS

Reroll ...

There we go

Oh, hah! Nice fix :)

I think somebody else with deeper CORS knowledge/experience should RTBC this. But the patch looks great to me. Only very minor remarks.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/CorsCompilerPass.php
   @@ -0,0 +1,31 @@
   + * @see services.yml
   

   core.services.yml ?

2. +++ b/sites/default/default.services.yml
   @@ -153,3 +153,19 @@ parameters:
   +    # Specify allowed request methods, specify '*' to allow all possible ones.
   +    allowedMethods: []
   

   This one is a bit confusing. The default value is an array, but we must specify a sole string to allow all methods? Or is it ['*'] that you should specify here?

3. +++ b/sites/default/default.services.yml
   @@ -153,3 +153,19 @@ parameters:
   +    exposedHeaders: false
   +    maxAge: false
   +    supportsCredentials: false
   

   We have documentation for the other settings, shouldn't we also have them for these?

Here are some minor adaptions ...

On second thought, in #105 I said:

I think somebody else with deeper CORS knowledge/experience should RTBC this. But the patch looks great to me. Only very minor remarks.

… but this patch doesn't actually enable CORS by default. This is just to make it possible to enable CORS. That's a big step forward. So let's do this.

This will still need a change record. And ideally, it would be documented. I think https://www.drupal.org/documentation/modules/rest/start would be a good place.

Updated

still needs work for change record

I started a CR myself: https://www.drupal.org/node/2715637

That also made me realize the current issue title is fairly misleading

I had a quick read through https://github.com/asm89/stack-cors/blob/master/src/Asm89/Stack/CorsServ...

    private function checkOrigin(Request $request) {
        if ($this->options['allowedOrigins'] === true) {
            // allow all '*' flag
            return true;
        }
        $origin = $request->headers->get('Origin');
        return in_array($origin, $this->options['allowedOrigins']);
    }
    private function checkMethod(Request $request) {
        if ($this->options['allowedMethods'] === true) {
            // allow all '*' flag
            return true;
        }
        $requestMethod = strtoupper($request->headers->get('Access-Control-Request-Method'));
        return in_array($requestMethod, $this->options['allowedMethods']);

The non-strict in_array() comparisons don't look great, especially given the configuration apparently allows an array of methods/origins as well as boolean true for both - I can see a badly resolved merge conflict or similar leading to someone having 'true' in their options array.

+++ b/core/core.services.yml
@@ -700,6 +708,11 @@ services:
   psr7.http_foundation_factory:

Not sure about adding this to core.services.yml without anything using it. For example why wouldn't REST module add it itself if that's where we're going to use it in core?

If we take the configuration from services.yml, if multiple modules want to use CORS (say one for REST and one for AJAX), how do the different configuration needs between the two work? Or is it entirely up to the site administrator to configure this correctly, in which case we're back to the original question of why put it in core.services.yml and undoing it with a compiler pass vs. just letting them configure in a project/site-specific YAML.

Just in general, having this in a middleware doesn't seem like the most useful place. It allows requests to be rejected very early if they're invalid, but that's by far the least common case - so we'll be running this code on requests that don't care about this at all.

Also moving this out of rest module to base system for now.

For example why wouldn't REST module add it itself if that's where we're going to use it in core?

Because it lives on a different level than REST. REST = routes + controllers. CORS is something you may want to apply to things other than REST too. CORS is something more fundamental than REST.

If we take the configuration from services.yml, if multiple modules want to use CORS (say one for REST and one for AJAX), how do the different configuration needs between the two work?

That is the thing: it needs to be configured. That's why it's opt-in. We can't automate this, because it has security consequences. So we need developers to configure it to suit their needs.

in which case we're back to the original question of why put it in core.services.yml and undoing it with a compiler pass vs. just letting them configure in a project/site-specific YAML.

To my understanding, core.services.yml contains all of the default configuration, site-specific YAML files can then override container parameters, i.e. any container parameter a site-specific YAML file specifies is one they override.

This is why sites/default/default.services.yml duplicates a lot of container parameters that live in core.services.yml, but it adds documentation. And it's why Drupal's default installation doesn't actually copy sites/default/default.services.yml to sites/default/services.yml. It's an opt-in process. It's exactly the same here.

Just in general, having this in a middleware doesn't seem like the most useful place. It allows requests to be rejected very early if they're invalid, but that's by far the least common case - so we'll be running this code on requests that don't care about this at all.

Having this in a middleware may add CORS headers to some responses unnecessarily, but it does make reasoning about it far simpler: it means that we'll be setting the same CORS headers on all responses. Which indeed may be overkill/unnecessary in some cases, but the lack of conditionality, and the declarativeness we get instead make it super simple to reason about: my site controls domains X, Y and Z, and so they're safe to interact with, so I'll whitelist them. That's simple.
If you instead have to whitelist on a per-URL, per-route or per REST-resource basis, that makes the configuration a whole lot more complex, with no apparent gain. Because if you trust a certain domain

Also, quoting #16:

When adding new functionality to the web platform, it can be tempting to grant a privilege to one resource but to withhold that privilege from another resource in the same origin. However, withholding privileges in this way is ineffective because the resource without the privilege can usually obtain the privilege anyway because user agents do not isolate resources within an origin. Instead, privileges should be granted or withheld from origins as a whole (rather than discriminating between individual resources within an origin) [BOFGO].

If you instead have to whitelist on a per-URL, per-route or per REST-resource basis, that makes the configuration a whole lot more complex, with no apparent gain. Because if you trust a certain domain

So what we do on normal requests is the following:

if ( ! $this->cors->isCorsRequest($request)) {
  return $this->app->handle($request, $type, $catch);
}

and

 public function isCorsRequest(Request $request)
    {
        return $request->headers->has('Origin');
    }

so for normal HTML requests we don't use it anyway. Its hard to see how more minimal it could be.

In general I don't want to fight for that external library, I just think that actual configuration via CMI doesn't give us that much.

Per #114, #115 and #116, back to RTBC.

Retesting.

Discussed in irc with dawehner.

I have a general concern that middlewares are becoming the new hook_boot(). The usage here is OK, but I have a list as long as my arm of horrible things done in hook_boot() in 7.x and earlier, and the more middlewares we get in core, the more likely it seems we'll get them used as an antipattern in contrib and custom code. Is there high level documentation of them on d.o? If not a follow-up to add some would be good.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/CorsCompilerPass.php
@@ -0,0 +1,31 @@
+}

Could this not be reversed so it adds the middleware if it's enabled, rather than removing it if it's disabled? dawehner also brought this up in irc.

but I have a list as long as my arm of horrible things done in hook_boot() in 7.x and earlier

:D

and the more middlewares we get in core, the more likely it seems we'll get them used as an antipattern in contrib and custom code. Is there high level documentation of them on d.o? If not a follow-up to add some would be good.

Effectively they indeed are the new hook_boot(). Is this really an anti-pattern? Sometimes there are things you need to do at that stage. What really matters, is that they're as fast as possible, and rely on as few and as cheap services as possible. The devel module's middleware for example is awful, because it starts the entire configuration service… which OTOH seems totally acceptable for that particular module, since it shouldn't ever run on production anyway.
So, my question: what do you want to see happen? Documentation explaining the purpose of middlewares, the caveats, what to pay attention to?

Could this not be reversed so it adds the middleware if it's enabled, rather than removing it if it's disabled? dawehner also brought this up in irc.

That sounds very sensible.

Effectively they indeed are the new hook_boot(). Is this really an anti-pattern? Sometimes there are things you need to do at that stage. What really matters, is that they're as fast as possible, and rely on as few and as cheap services as possible.

Yes it's this last bit that's important. I've seen hook_boot() calling drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL)

So, my question: what do you want to see happen? Documentation explaining the purpose of middlewares, the caveats, what to pay attention to?

Yes something like this. Also the distinction from kernel request events (which I also don't like for a lot of the things they get used for - those are the new hook_init()).

Do you have thoughts on where this documentation should live? I don't see any child page of https://www.drupal.org/developing/api/8 that is relevant (yes, because our documentation is saddeningly incomplete).

Looks #2303673: Implement stackphp; cleanup handlePageCache() and preHandle() missed to add change record and docs
Meta is closed also #2492955: [meta] PSR-7 support

As I see there's only https://www.drupal.org/node/2409067 about middleware

#123 no I had a feeling there was none at all, that's what this issue reminded me of.

Alright, I'll take on working on that documentation and on the reversing mentioned in #120. Then this can go back to RTBC. This is so tantalizingly close.

@dawehner, if you want to work on the reversing, feel free. I'll take on the documentation in any case.

Could this not be reversed so it adds the middleware if it's enabled, rather than removing it if it's disabled? dawehner also brought this up in irc.

I was about to just do it, but then I realized something

a) For better discoverability (use for example ag "- { name: http_middleware }" its worth to have the service inside core.services.yml IMHO
b) I thought about dropping

     tags:
       - { name: http_middleware }

by default, but still the discoverability.
c) The service pass itself used a enabled flag, rather than a disabled flag, so the default behaviour is to not add it to the container, even if the config is dropped entirely.

Given that on runtime we skip the middleware by default, I think the additional discoverability is a good point for keeping it as it is now.

Feel free to disagree.

#127:

a) Agreed.
b) Indeed, that'd result in a partial/incomplete service definition being declarative in a YAML file, and then some logic to make it "complete" in PHP. That's confusing.
c) Right:

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/CorsCompilerPass.php
@@ -0,0 +1,31 @@
+    // Remove the CORS middleware completly in case it was not enabled.
+    if (!$enabled) {
+      $container->removeDefinition('http_middleware.cors');
+    }

@catch referred to this exact code in #120 too:

Could this not be reversed so it adds the middleware if it's enabled, rather than removing it if it's disabled? dawehner also brought this up in irc.

So, catch has a general concern about adding middlewares, and them becoming the new hook_boot(). This patch won't actually make that worse, if anything, it makes it better, because the middleware will only effectively be present in the container if cors.config.enabled === true. But that's easily overlooked, is what catch's concern is. To properly explain this, I think we need documentation anyway. Wrote that just now: https://www.drupal.org/developing/api/8/middleware.

That still leaves a matter of preference. Either:

• declarative service definition in core.services.yml and PHP code to remove it — pro: better discoverability, con: feels a bit strange logically (always add, conditionally remove)
• PHP code to set the service definition — pro: nicer logically (conditionally added), con: worse discoverability

Given that in Drupal we tend to favor discoverability/DX over logical/semantical purity, I think @dawehner is right. So, back to RTBC.

Rerolled

Failed in unrelated Config.Drupal\config\Tests\ConfigImportAllTest. Retesting, back to RTBC.

Now a random fail in SqlContentEntityStorageSchemaIndexFilledTest . Sigh.

Thanks! Published CR: https://www.drupal.org/node/2715637.

+++ b/composer.lock
@@ -8,6 +8,49 @@
@@ -1061,7 +1104,7 @@

@@ -1061,7 +1104,7 @@
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/php-fig/log/zipball/1.0.0",
+                "url": "https://api.github.com/repos/php-fig/log/zipball/fe0936ee26643249e916849d48e3a51d5f5e278b",
                 "reference": "1.0.0",
                 "shasum": ""
             },
@@ -2716,24 +2759,24 @@

@@ -2716,24 +2759,24 @@
     "packages-dev": [
         {
             "name": "behat/mink",
-            "version": "v1.7.1",
+            "version": "v1.7.0",
             "source": {
                 "type": "git",
                 "url": "https://github.com/minkphp/Mink.git",
-                "reference": "e6930b9c74693dff7f4e58577e1b1743399f3ff9"
+                "reference": "6c129030ec2cc029905cf969a56ca8f087b2dfdf"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/minkphp/Mink/zipball/e6930b9c74693dff7f4e58577e1b1743399f3ff9",
-                "reference": "e6930b9c74693dff7f4e58577e1b1743399f3ff9",
+                "url": "https://api.github.com/repos/minkphp/Mink/zipball/6c129030ec2cc029905cf969a56ca8f087b2dfdf",
+                "reference": "6c129030ec2cc029905cf969a56ca8f087b2dfdf",
                 "shasum": ""
             },
             "require": {
                 "php": ">=5.3.1",
-                "symfony/css-selector": "~2.1|~3.0"
+                "symfony/css-selector": "~2.1"
             },
             "require-dev": {
-                "symfony/phpunit-bridge": "~2.7|~3.0"
+                "symfony/phpunit-bridge": "~2.7"
             },
             "suggest": {
                 "behat/mink-browserkit-driver": "extremely fast headless driver for Symfony\\Kernel-based apps (Sf2, Silex)",
@@ -2770,31 +2813,31 @@

@@ -2770,31 +2813,31 @@
                 "testing",
                 "web"
             ],
-            "time": "2016-03-05 08:26:18"
+            "time": "2015-09-20 20:24:03"
         },
         {
             "name": "behat/mink-browserkit-driver",
-            "version": "v1.3.2",
+            "version": "v1.3.0",
             "source": {
                 "type": "git",
                 "url": "https://github.com/minkphp/MinkBrowserKitDriver.git",
-                "reference": "10e67fb4a295efcd62ea0bf16025a85ea19534fb"
+                "reference": "da47df1593dac132f04d24e7277ef40d33d9f201"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/minkphp/MinkBrowserKitDriver/zipball/10e67fb4a295efcd62ea0bf16025a85ea19534fb",
-                "reference": "10e67fb4a295efcd62ea0bf16025a85ea19534fb",
+                "url": "https://api.github.com/repos/minkphp/MinkBrowserKitDriver/zipball/da47df1593dac132f04d24e7277ef40d33d9f201",
+                "reference": "da47df1593dac132f04d24e7277ef40d33d9f201",
                 "shasum": ""
             },
             "require": {
-                "behat/mink": "^1.7.1@dev",
+                "behat/mink": "~1.7@dev",
                 "php": ">=5.3.6",
-                "symfony/browser-kit": "~2.3|~3.0",
-                "symfony/dom-crawler": "~2.3|~3.0"
+                "symfony/browser-kit": "~2.3",
+                "symfony/dom-crawler": "~2.3"
             },
             "require-dev": {
                 "silex/silex": "~1.2",
-                "symfony/phpunit-bridge": "~2.7|~3.0"
+                "symfony/phpunit-bridge": "~2.7"
             },
             "type": "mink-driver",
             "extra": {
@@ -2826,20 +2869,20 @@

@@ -2826,20 +2869,20 @@
                 "browser",
                 "testing"
             ],
-            "time": "2016-03-05 08:59:47"
+            "time": "2015-09-21 20:56:13"
         },
         {
             "name": "behat/mink-goutte-driver",
-            "version": "v1.2.1",
+            "version": "v1.2.0",
             "source": {
                 "type": "git",
                 "url": "https://github.com/minkphp/MinkGoutteDriver.git",
-                "reference": "8b9ad6d2d95bc70b840d15323365f52fcdaea6ca"
+                "reference": "c8e254f127d6f2242b994afd4339fb62d471df3f"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/minkphp/MinkGoutteDriver/zipball/8b9ad6d2d95bc70b840d15323365f52fcdaea6ca",
-                "reference": "8b9ad6d2d95bc70b840d15323365f52fcdaea6ca",
+                "url": "https://api.github.com/repos/minkphp/MinkGoutteDriver/zipball/c8e254f127d6f2242b994afd4339fb62d471df3f",
+                "reference": "c8e254f127d6f2242b994afd4339fb62d471df3f",
                 "shasum": ""
             },
             "require": {
@@ -2849,7 +2892,7 @@

@@ -2849,7 +2892,7 @@
                 "php": ">=5.3.1"
             },
             "require-dev": {
-                "symfony/phpunit-bridge": "~2.7|~3.0"
+                "symfony/phpunit-bridge": "~2.7"
             },
             "type": "mink-driver",
             "extra": {
@@ -2881,7 +2924,7 @@

@@ -2881,7 +2924,7 @@
                 "headless",
                 "testing"
             ],
-            "time": "2016-03-05 09:04:22"
+            "time": "2015-09-21 21:31:11"
         },
         {
             "name": "doctrine/instantiator",
@@ -2939,24 +2982,24 @@

@@ -2939,24 +2982,24 @@
         },
         {
             "name": "fabpot/goutte",
-            "version": "v3.1.2",
+            "version": "v3.1.1",
             "source": {
                 "type": "git",
                 "url": "https://github.com/FriendsOfPHP/Goutte.git",
-                "reference": "3cbc6ed222422a28400e470050f14928a153207e"
+                "reference": "751a3dc5c4d86ec3e97c9f27133ef9694d9243cc"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/FriendsOfPHP/Goutte/zipball/3cbc6ed222422a28400e470050f14928a153207e",
-                "reference": "3cbc6ed222422a28400e470050f14928a153207e",
+                "url": "https://api.github.com/repos/FriendsOfPHP/Goutte/zipball/751a3dc5c4d86ec3e97c9f27133ef9694d9243cc",
+                "reference": "751a3dc5c4d86ec3e97c9f27133ef9694d9243cc",
                 "shasum": ""
             },
             "require": {
                 "guzzlehttp/guzzle": "^6.0",
                 "php": ">=5.5.0",
-                "symfony/browser-kit": "~2.1|~3.0",
-                "symfony/css-selector": "~2.1|~3.0",
-                "symfony/dom-crawler": "~2.1|~3.0"
+                "symfony/browser-kit": "~2.1",
+                "symfony/css-selector": "~2.1",
+                "symfony/dom-crawler": "~2.1"
             },
             "type": "application",
             "extra": {
@@ -2984,7 +3027,7 @@

@@ -2984,7 +3027,7 @@
             "keywords": [
                 "scraper"
             ],
-            "time": "2015-11-05 12:58:44"
+            "time": "2015-08-29 16:16:56"
         },

This composer change is not right - it's a downgrade for no reason. I'm reverting this patch because we shouldn't be doing this here.

Here's the fixed composer patch.

The best method to add a new dependency to core/composer.json is a bit obscure.

1. Add the dependency to core/composer.json
2. Run composer update NOTHING

Doing the weird second step cause composer to sort out the merge and do an install.

My fixes to this patch have little to do the actual functionality - that has been rtbc before :) so... rtbc'ing the issue.

+++ b/core/composer.json
@@ -31,7 +31,8 @@
+        "asm89/stack-cors": "~0.2.1"

This version doesn't support symfony 3.0 also see https://github.com/asm89/stack-cors/issues/26. This will hinder our performance in #2712647: Update Symfony components to ~3.2. We should consider changing it to dev-master here like #2776105: Update asm89/stack-cors for Symfony 3 compatibility and revert changes introduced by #1869548.

@jibran
But we have a way to solve that ... IMHO updating to symfony 3 is not a requirement for 8.2. 2.8 will be supported for longer.

I think we should ask for an official release - see https://github.com/asm89/stack-cors/issues/26 and get that before re-committing. Symfony 3 is a known and big target. Committing dev-master dependencies is not a good idea.

But using a reference is better than just a value copy because exception traces are not always something small.

So #150 bumped the version to 1.0.0 - nice.

+++ b/composer.lock
@@ -1100,12 +1100,12 @@
-                "reference": "1.0.0"
+                "reference": "fe0936ee26643249e916849d48e3a51d5f5e278b"
...
+                "url": "https://api.github.com/repos/php-fig/log/zipball/fe0936ee26643249e916849d48e3a51d5f5e278b",
+                "reference": "fe0936ee26643249e916849d48e3a51d5f5e278b",
                 "shasum": ""

These are completely unrelated changes - not sure why they occur but they don't for me.

Changes occur after composer self-update. Can you confirm it?

@slasher13 not for me. My composer version is Composer version 1.2.0 2016-07-19 01:28:52. The procedure I follow is outlined in #142.

I could reproduce my problem.

• If i run initially composer install with an empty vendor folder and then composer update NOTHING, no package information changes.
• If i run initially composer update NOTHING with an empty vendor folder and then composer update NOTHING again, package information changes.

@slasher13 nice sleuthing. It's one of the tricky things about composer.lock diffs - people have got too used to stuff changing for no reason. I think it's best for changes to be the minimal change - which #152 has. Nice.

It seems to be that @alexpott and @slasher13 agree on #152, so this is ready again, nice!

Thank you @slasher13 for jumping in!

Yay!

This most likely needs to go in before the beta commit freeze (tomorrow) to be in 8.2.x, so tagging for visibility in the RTBC queue.

Committed bc83416 and pushed to 8.2.x. Thanks!

Just noting this was really really hard to override from a config object. See #2794109: Find a way to replace the container parameter in a better way..

Re: andypost, #80, looks like we had the same thought :)