Skip to content

[HttpCache] fix: do not cache OPTIONS request #20205

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 14, 2016
Merged

[HttpCache] fix: do not cache OPTIONS request #20205

merged 1 commit into from
Oct 14, 2016

Conversation

dmaicher
Copy link
Contributor

@dmaicher dmaicher commented Oct 11, 2016 •
edited
Loading

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #20202
License MIT
Doc PR -

The HttpCache should not cache any OPTIONS request as they are by spec not cacheable (mentioned here #20202 (comment) by @xabbuh).

@dunglas
Copy link
Member

dunglas commented Oct 13, 2016

Tests must be fixed.

@dmaicher dmaicher force-pushed the http_cache_options branch 2 times, most recently from f3b0cec to 9e3c34d Compare October 13, 2016 17:01
@dmaicher dmaicher changed the base branch from master to 2.7 October 13, 2016 17:02
@dmaicher
Copy link
Contributor Author

How to fix that build fail? 😊

There were 56 errors:

1) Symfony\Component\HttpKernel\Tests\HttpCache\HttpCacheTest::testDoesNotCacheWithAuthorizationRequestHeaderAndNonPublicResponse

Error: Call to undefined method Symfony\Component\HttpFoundation\Request::isMethodCacheable()

I guess I need to update the dependency of symfony/http-foundation somehow?

ogizanagi
ogizanagi reviewed Oct 13, 2016
View reviewed changes
src/Symfony/Component/HttpKernel/composer.json
@@ -18,7 +18,7 @@
"require": {
"php": ">=5.3.9",
"symfony/event-dispatcher": "~2.6,>=2.6.7",
"symfony/http-foundation": "~2.7.15|~2.8.8",
"symfony/http-foundation": "~2.7.20|~2.8.13",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have to change this for 2.7.x-dev

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also fails. I guess it has to use my branch?

Copy link
Member

Choose a reason for hiding this comment

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

~2.7.20|~2.8.13 should be correct

Copy link
Member

Choose a reason for hiding this comment

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

Please note that in that case it is okay that the deps=high tests are failing (because the feature isn't merged to higher branches yet). The deps=low tests must pass though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok good to know 👍 Now I understand. Changed it back.

@dmaicher
Copy link
Contributor Author

tests are passing now (except deps=high) 🎉

@xabbuh
Copy link
Member

xabbuh commented Oct 13, 2016

👍

Status: Reviewed

@dunglas
Copy link
Member

dunglas commented Oct 13, 2016

👍

@fabpot
Copy link
Member

fabpot commented Oct 14, 2016

Thank you @dmaicher.

@fabpot fabpot merged commit c43de7f into symfony:2.7 Oct 14, 2016
fabpot added a commit that referenced this pull request Oct 14, 2016
@fabpot
bug #20205 [HttpCache] fix: do not cache OPTIONS request (dmaicher)
55c3f89 
This PR was merged into the 2.7 branch.

Discussion
----------

[HttpCache] fix: do not cache OPTIONS request

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20202
| License       | MIT
| Doc PR        | -

The HttpCache should not cache any OPTIONS request as they are by spec not cacheable (mentioned here #20202 (comment) by @xabbuh).

Commits
-------

c43de7f [HttpCache] fix: do not cache OPTIONS request
This was referenced Oct 27, 2016
Merged
Merged
Merged
nicolas-grekas referenced this pull request Nov 3, 2016
@nicolas-grekas
Merge branch '2.8' into 3.1
af10027 
* 2.8:
  Fix deps merge
@wimleers wimleers mentioned this pull request Nov 18, 2016
Closed
@dmaicher dmaicher deleted the http_cache_options branch November 21, 2016 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabbuh xabbuh xabbuh left review comments

@ogizanagi ogizanagi ogizanagi left review comments

Assignees
No one assigned
Labels
Bug Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dmaicher @dunglas @xabbuh @fabpot @ogizanagi @carsonbot