Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle OPTIONS cacheable #74

Merged
merged 3 commits into from
Apr 18, 2020
Merged

Handle OPTIONS cacheable #74

merged 3 commits into from
Apr 18, 2020

Conversation

barryvdh
Copy link
Collaborator

@barryvdh
Copy link
Collaborator Author

So this would make sure that Preflight requests and OPTIONS requests are handled same as before, only the Vary header is added for the Methods, so preflight / non-preflight will be cached separately. Origin check is removed so it it doesn't create cache entries per origin.

@davidbarratt
Copy link
Contributor

This looks perfect to me.

@@ -166,7 +164,7 @@ private function configureAllowedMethods(Response $response, Request $request)
if ($this->options['allowedMethods'] === true) {
if ($this->options['supportsCredentials']) {
$allowMethods = strtoupper($request->headers->get('Access-Control-Request-Method'));
$this->varyHeader($response, 'Access-Control-Request-Method');
// Vary is always done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe more specific?

// Vary by Access-Control-Request-Method is added to every preflight request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tweaked it a bit by just adding it and detect duplicates, in case the CorsService is used stand-alone or with a custom middleware.

@barryvdh barryvdh merged commit cfa0af5 into master Apr 18, 2020
@barryvdh
Copy link
Collaborator Author

barryvdh commented Apr 18, 2020

FYI, I've tweaked it a bit in 03157d5
This will add CORS headers to plain OPTIONS requests also. (and tagged beta2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants