Skip to content
This repository has been archived by the owner on Feb 24, 2023. It is now read-only.

Commit

Permalink
bug #714 Restore undetermined state in Cache configuration (jderusse)
Browse files Browse the repository at this point in the history
This PR was merged into the 6.1.x-dev branch.

Discussion
----------

Restore undetermined state in Cache configuration

This PR fix a BC break introduced by #707.

Given the following code:
```
    /**
     * @Cache(smaxage="+1min")
     */
    public function fooAction(){}
```

The headers generated by `HttpCacheListener` changed as following:
```diff
- public, s-maxage=60
+ private, s-maxage=60
```

Why? Because previously the Cache's properties were null until the user provide a value. In such state, `isPublic` returns false (although `isPrivate`). And sometimes, null is expected (like the `vary` array => https://github.com/sensiolabs/SensioFrameworkExtraBundle/blob/b3596907c3d35bee5a9a1096baaeb7d465a04f30/src/EventListener/HttpCacheListener.php#L135)

The #707 PR introduce 2 changes:
- a value for each property is now forced by the constructor's default value.
- the constructor calls the setter for each property (even if the user did not provide a value for it), some setter (ie. `setPublic`) does not expect a null value and convert null into something.

It looks like, only the Cache configuration is affected. Other Configuration has already a default value defined for each property.

This PR restore the default `null` value for all properties and prevent the constructor to call the setter when the value is null.

Commits
-------

99e749b Restore undetermined state in Cache configuration
  • Loading branch information
fabpot committed Feb 23, 2021
2 parents b359690 + 99e749b commit 8ef75e9
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 7 deletions.
13 changes: 8 additions & 5 deletions src/Configuration/Cache.php
Expand Up @@ -114,11 +114,11 @@ public function __construct(
string $expires = null,
$maxage = null,
$smaxage = null,
bool $public = false,
bool $mustRevalidate = false,
array $vary = [],
?string $lastModified = null,
?string $etag = null,
bool $public = null,
bool $mustRevalidate = null,
array $vary = null,
string $lastModified = null,
string $etag = null,
$maxstale = null,
$staleWhileRevalidate = null,
$staleIfError = null
Expand All @@ -135,6 +135,9 @@ public function __construct(
$values['staleWhileRevalidate'] = $values['staleWhileRevalidate'] ?? $staleWhileRevalidate;
$values['staleIfError'] = $values['staleIfError'] ?? $staleIfError;

$values = array_filter($values, function ($v) {
return $v !== null;
});
parent::__construct($values);
}

Expand Down
16 changes: 14 additions & 2 deletions tests/EventListener/HttpCacheListenerTest.php
Expand Up @@ -50,6 +50,18 @@ public function testWontReassignResponseWhenNoConfigurationIsPresent()
$this->assertSame($response, $this->event->getResponse());
}

public function testResponseIsPublicIfSharedMaxAgeSetAndPublicNotOverridden()
{
$request = $this->createRequest(new Cache([
'smaxage' => 1,
]));

$this->listener->onKernelResponse($this->createEventMock($request, $this->response));

$this->assertTrue($this->response->headers->hasCacheControlDirective('public'));
$this->assertFalse($this->response->headers->hasCacheControlDirective('private'));
}

public function testResponseIsPublicIfConfigurationIsPublicTrue()
{
$request = $this->createRequest(new Cache([
Expand All @@ -65,8 +77,8 @@ public function testResponseIsPublicIfConfigurationIsPublicTrue()
public function testResponseIsPrivateIfConfigurationIsPublicFalse()
{
$request = $this->createRequest(new Cache([
'public' => false,
]));
'public' => false,
]));

$this->listener->onKernelResponse($this->createEventMock($request, $this->response));

Expand Down

0 comments on commit 8ef75e9

Please sign in to comment.