Improve documentation for cors configuration.

Same here. +1

I just encountered this today.

If you look at line 94 in CorsService.php (Drupal 8.4.4), it's expecting an array and not a Boolean.

$response->headers->set('Access-Control-Expose-Headers', implode(', ', $this->options['exposedHeaders']));

Defining 'exposedHeaders' as 'true' rightfully produces an error in the log.

So what's the purpose of this property? Should it contain the same header array as the allowedHeaders property?

It seems like a good short term solution to this issue would be to provide some better help text in sites/default/default.services.yml.

Thank you for improving the documentation by giving an example too.

+++ b/sites/default/default.services.yml
@@ -166,7 +166,9 @@ parameters:
+    # Sets the Access-Control-Expose-Headers header. The default is false.
+    # To modify the exposed headers use a comma delimited list.
+    # i.e. ['Content-Type', 'Expires'] or ['*'] for all headers.
     exposedHeaders: false

+1 for improving the docs but I think it can be a little bit more specific. The exposedHeaders value expects a sequence - ie it can be:

    exposedHeaders:
      - Content-Type
      - Expires

or

    exposedHeaders: ['Content-Type', 'Expires']

I was thinking of just recommend changing To modify the exposed headers use a comma delimited list. to To modify the exposed headers use a comma delimited list within square brackets because the within square brackets and then I just ended up reading all the CORS docs and thought if we improve this one why not improve them all and require less cognitive overload on the person doing the configuring.

@alexpott
There seems to be an error in your patch? Code for exposed headers only handles the imploding of an array of values. There is no wildcard support for that property.

@littletiger I was probably going on the patch in #5. But looking at the docs the situation is complex :) https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control...

+++ b/sites/default/default.services.yml
@@ -160,15 +160,32 @@ parameters:
+    # Specify allowed headers and sets the Access-Control-Allow-Headers header.

Statement is switching between singular and plural (specify/sets). I think this should be either singular (specify/set) or plural (specifies/sets). Same goes for the allowedMethods comment.

There seems to be an error in your patch? Code for exposed headers only handles the imploding of an array of values. There is no wildcard support for that property.

This looks ok to me. Setting exposedHeaders: ['*'] will result in a Access-Control-Expose-Headers: * response header, which is valid according to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control....

+++ b/sites/default/default.services.yml
@@ -160,15 +160,32 @@ parameters:
+    # Sets the Access-Control-Max-Age header if set to an integer. This is the
+    # maximum number of seconds the results can be cached. For example, 600 to
+    # cache results of a preflight request for 10 minutes. See
+    # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Max-Age

Setting this value to '0' or false will omit the Access-Control-Max-Age header from the response. However, setting it to '-1' will explicitly disable caching, we might want to add that to the comment.

Simple changes to the existing patch, so tagging novice.

I have created a patch with the mentioned suggestions in #15. Please review if more changes are required.

Thanks @shimpy, changes look good. Before we can do a final review, we need to address the test failure: this is caused by #3067645: Add core scaffold assets to drupal/core's composer.json extra field, which landed a month ago. From now until 9.x, changes to default.services.yml will need to be made both in /sites/default/ and /core/assets/scaffold/files/ (the linked issue explains the reasoning for this).

Still a novice issue, since the only required change is to copy /sites/default/default.services.yml to /core/assets/scaffold/files/default.services.yml and re-roll the patch with both files changed.

Hii @mr.baileys

Thanks for the suggestion. I have recreated a patch as per #19. Please review.

Failed patch is a random test failure caused by #3041318: Various random fails due to mis-triggered Mink deprecation error, re-test requested.

@mr.baileys

I have restested it.. It pass all tests.

+++ b/core/assets/scaffold/files/default.services.yml
@@ -160,15 +160,33 @@ parameters:
+    # Setting Access-Control-Max-Age headers value to '0' or false will omit the this from the response.
+    # However, setting it to '-1' will explicitly disable caching. For example, 600 to
+    # cache results of a preflight request for 10 minutes. See

The wording here still needs a bit of work. "headers" when there is only one header, "the this" doesn't make sense either.

Fixed the wordings

As per the previous patches we need to ensure that /sites/default/default.services.yml and /core/assets/scaffold/files/default.services.yml are changed in exactly the same way.

I have updated both the files. Please review.

1. +++ b/core/assets/scaffold/files/default.services.yml
   @@ -160,15 +160,33 @@ parameters:
   +    # list within square brackets. For example, ['Content-Type', 'Expires'] or
   +    # Setting exposedHeaders: ['*'] will result in a Access-Control-Expose-Headers:
   

   I think we should word this as per the above

   For example, ['Content-Type', 'Expires'] or ['*'] to expose all headers.

2. +++ b/core/assets/scaffold/files/default.services.yml
   @@ -160,15 +160,33 @@ parameters:
   +    # However, setting it to '-1' will explicitly disable caching. For example, 600 to
   +    # cache results of a preflight request for 10 minutes. See
   

   I this this is missing some words before 600

   suggest. For example, setting the value to 600 will cache results of a preflight for 10 minutes.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Patch for Drupal version 10.1.x

Points in #33 were not addressed.

Not ready for review yet.

Now the strings are going over 80 characters per line.

Fixed docs' lines having characters above 80 in a single line.

Thanks!

+++ b/core/assets/scaffold/files/default.services.yml
@@ -200,18 +200,38 @@ parameters:
+    # ['https://www.drupal.org'] or ['*'] to allow any resource to access your
+    # resource. See
+    # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin

I think this should be 'allow any origin to access your resource' rather than 'resource to access your resource'

There's still a few lines that don't wrap at 80 chars too (At least according to dreditor)

Hi @larowlan,

As per your suggestion, I have made changes with respect to patch #44.

Corrected the sentence. Please verify.

Setting the issue status to Needs Review.

Also, I am uploading a patch along with an interdiff file.

Thanks.

Resolved errors from #48

Shoot.
I guess some tech stack issue with my system probably.
Kindly, Ignore #44.

Also, @Rishabh, thanks for the patch and interdiff, but I believe @larowlan asked to change the sentence in "core/assets/scaffold/files/default.services.yml" but your interdiff showcases the changes in "sites/default/default.services.yml".

Please guide me, if I am getting this wrong anywhere.

Re-attaching the interdiff file with patch.

Thank You !

Fixed 'resource' to origin in both the files as suggested by larowlan in #46.

does this need issue summary update?

Hi,

I tested the patch of #55 in d11, it works fine, applies cleanly. Since, the issue has changed to 11.x, providing the patch for 11.x.

Hiding #60. Thank you but #55 applied cleanly to 11.x so #60 is just a re-upload from what I can tell, since there was no interdiff.

Can run the 11.x tests by clicking the "Add test/retest" link. Queued #55 for 11.x tests.

But #57 I believe is correct in that IS should be updated about what is being accomplished. Me marking RTBC in #56 was incorrect on my part.

1. +++ b/core/assets/scaffold/files/default.services.yml
   @@ -160,15 +160,33 @@ parameters:
   +    # Setting exposedHeaders: ['*'] will result in a Access-Control-Expose-Headers:
   +    # * response header. See
   

   This line should wrap at 80 characters. This will also fix the fact that header name and value are currently split over 2 lines.

2. +++ b/core/assets/scaffold/files/default.services.yml
   @@ -160,15 +160,33 @@ parameters:
   +    # Setting Access-Control-Max-Age header value to '0' or false will omit this from the response.
   +    # However, setting it to '-1' will explicitly disable caching. For example, 600 to
   

   Line wrapping should be fixed.

It might make sense to merge #2905848: Improve documentation for cors configuration. and this issue, since that issue is about adding and documenting the new allowedOriginsPatterns-option to cors.config in default.services.yml.

Made changes as per comment #62.
Please review.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

This still needs a rebase which was not addressed in the creation of the MR.

Remember to hide all patch files when switching to a MR.

Hi @xjm @smustgrave
I intentionally deferred work on these issues. All the updates were made on November 14, 2023, and I've been addressing them sequentially in the Drupal issue queue without initially considering the tags assigned to each. I acknowledge this oversight and apologize for any confusion. Moving forward, I will ensure to prioritize and address issues based on their respective tags.