Skip to content

[HttpFoundation] Adds support for the immutable directive in the cache-control header #22932

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
fabpot merged 1 commit into from
Jun 21, 2017
Merged

[HttpFoundation] Adds support for the immutable directive in the cache-control header #22932

fabpot merged 1 commit into from
Jun 21, 2017

Conversation

@twoleds
Copy link
Contributor

@twoleds twoleds commented May 28, 2017 •
edited
Loading

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

Added support for the immutable directive in the cache-control header, tries to resolve #21425.

@twoleds twoleds changed the title Added support for the immutable directive in the cache-control header [HttpFoundation] Adds support for the immutable directive in the cache-control header May 28, 2017
@twoleds twoleds force-pushed the twoleds/cache-control-immutable branch from cba133a to eb37144 Compare May 28, 2017 19:39
@xabbuh xabbuh added this to the 3.4 milestone May 29, 2017
@twoleds twoleds force-pushed the twoleds/cache-control-immutable branch from eb37144 to 4b03f1b Compare May 30, 2017 18:10
@twoleds twoleds changed the base branch from master to 3.4 May 30, 2017 18:25
@twoleds twoleds changed the base branch from 3.4 to master May 30, 2017 18:25
@greg0ire
Copy link
Contributor

greg0ire commented May 30, 2017

  1. Fetch all new commits: git fetch --all. ⬇️
  2. Rebase on what you fetched interactively: ✂️
    1. git rebase -i origin/3.4, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin repository.
    2. A window will show up with many lines, remove every line but the last one, which should correspond to your commit.
    3. If you run into a conflict: 💥
      1. fix it with git mergetool. 😷
      2. continue on your merry way with git rebase --continue. ⏩
  3. Force push to overwrite all this : git push --force. ⬆️

@twoleds twoleds force-pushed the twoleds/cache-control-immutable branch from 4b03f1b to cbf1dcc Compare May 30, 2017 19:06
@twoleds twoleds changed the base branch from master to 3.4 May 30, 2017 19:06
@twoleds twoleds force-pushed the twoleds/cache-control-immutable branch 5 times, most recently from abcdae4 to f9bbae7 Compare June 6, 2017 20:07
@twoleds twoleds force-pushed the twoleds/cache-control-immutable branch 2 times, most recently from cf6bc1f to fba0d33 Compare June 9, 2017 19:45
xabbuh
xabbuh reviewed Jun 12, 2017
View reviewed changes
src/Symfony/Component/HttpFoundation/Response.php
* @param bool $immutable Enables or disabled the immutable directive.
*
* @return $this
*/
Copy link
Member

@xabbuh xabbuh Jun 12, 2017

Choose a reason for hiding this comment

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

a default value doesn't make much sense here IMO

Copy link
Contributor Author

@twoleds twoleds Jun 13, 2017

Choose a reason for hiding this comment

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

OK I understand. My idea was calling the setImmutable method without argument like setPublic/setPrivate method.

return (new Response('SOME RESPONSE DATA'))
  ->setPublic()
  ->setImmutable();
return (new Response('SOME RESPONSE DATA'))
  ->setPublic()
  ->setImmutable(true);

I like the first example of code (calling setImmutable without boolean value). Maybe it will be better to implement opposite method setMutable (which remove the flag immutable from the Cache-Control header) and to remove the argument from setImmutable method completely. What is your opinion about it?

greg0ire
greg0ire suggested changes Jun 12, 2017
View reviewed changes
src/Symfony/Component/HttpFoundation/Response.php

/**
* Marks the response as "immutable".
*
Copy link
Contributor

@greg0ire greg0ire Jun 12, 2017

Choose a reason for hiding this comment

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

"disables"

src/Symfony/Component/HttpFoundation/Response.php Outdated
}

if (isset($options['immutable'])) {
$this->setImmutable($options['immutable'] ? true : false);
Copy link
Contributor

@greg0ire greg0ire Jun 12, 2017

Choose a reason for hiding this comment

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

How about a cast to book?

Copy link
Contributor

@greg0ire greg0ire Jun 13, 2017

Choose a reason for hiding this comment

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

*bool lol

@twoleds twoleds force-pushed the twoleds/cache-control-immutable branch from fba0d33 to 2143eff Compare June 13, 2017 17:48
@twoleds twoleds force-pushed the twoleds/cache-control-immutable branch from 60ca02e to 7f603b7 Compare June 13, 2017 18:49
stof
stof requested changes Jun 14, 2017
View reviewed changes
src/Symfony/Component/HttpFoundation/Response.php
* @return bool Returns true if the response is marked as "immutable"; otherwise false.
*/
public function isImmutable()
{
Copy link
Member

@stof stof Jun 14, 2017

Choose a reason for hiding this comment

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

just return $this->headers->hasCacheControlDirective('immutable');

Copy link
Contributor Author

@twoleds twoleds Jun 14, 2017

Choose a reason for hiding this comment

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

Fixed, thanks for hint

@twoleds twoleds force-pushed the twoleds/cache-control-immutable branch 2 times, most recently from 695fd9a to 4cf01a1 Compare June 14, 2017 21:10
@twoleds twoleds force-pushed the twoleds/cache-control-immutable branch from 4cf01a1 to 33573c6 Compare June 17, 2017 07:49
@fabpot
Copy link
Member

fabpot commented Jun 21, 2017

Thank you @twoleds.

@fabpot fabpot merged commit 33573c6 into symfony:3.4 Jun 21, 2017
fabpot added a commit that referenced this pull request Jun 21, 2017
@fabpot
feature #22932 [HttpFoundation] Adds support for the immutable direct…
16fbe3a 
…ive in the cache-control header (twoleds)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpFoundation] Adds support for the immutable directive in the cache-control header

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

Added support for the immutable directive in the cache-control header, tries to resolve #21425.

Commits
-------

33573c6 Added support for the immutable directive in the cache-control header
This was referenced Oct 18, 2017
Merged
Merged
@twoleds twoleds deleted the twoleds/cache-control-immutable branch May 4, 2022 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@javiereguiluz javiereguiluz javiereguiluz approved these changes

@stof stof stof approved these changes

@xabbuh xabbuh xabbuh approved these changes

+1 more reviewer

@greg0ire greg0ire greg0ire approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature HttpFoundation Status: Reviewed

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

6 participants

@twoleds @greg0ire @fabpot @javiereguiluz @stof @xabbuh