* 3.4:
Removed non-existing parameters for LogoutUrlGenerator calls
[HttpKernel] Correctly merging cache directives in HttpCache/ResponseCacheStrategy
[Validator] Add the missing translations for the Latvian ("lv") locale
Fixed the DebugClassLoader compatibility with eval()'d code on Darwin
[Validator] Update Serbian translation file
This PR was squashed before being merged into the 3.4 branch (closes#26532).
Discussion
----------
[HttpKernel] Correctly merging cache directives in HttpCache/ResponseCacheStrategy
| Q | A
| ------------- | ---
| Branch? | 3.4
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #26245, #26352, #28872
| License | MIT
| Doc PR | -
This PR is a first draft to fix the incorrect merging of private and other cache-related headers that are not meant for the shared cache but the browser (see mentioned issues).
The existing implementation of `HttpFoundation\Response` is very much tailored to the `HttpCache`, for example `isCacheable` returns `false` if the response is `private`, which is not true for a browser cache. That is why my implementation does not longer use much of the response methods. They are however still used by the `HttpCache` and we should keep them as-is. FYI, the `ResponseCacheStrategy` does **not** affect the stored data of `HttpCache` but is only applied to the result of multiple merged subrequests/ESI responses.
I did read up a lot on RFC2616 as a reference. [Section 13.4](https://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.4) gives an overall view of when a response MAY be cached. [Section 14.9.1](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.1) has more insight into the `Cache-Control` directives.
Here's a summary of the relevant information I applied to the implementation:
- > Unless specifically constrained by a cache-control (section 14.9) directive, a caching system MAY always store a successful response (see section 13.8) as a cache entry, MAY return it without validation if it is fresh, and MAY return it after successful validation.
A response without cache control headers is totally fine, and it's up to the cache (shared or private) to decide what to do with it. That is why the implementation does not longer set `no-cache` if no `Cache-Control` headers are present.
- > A response received with a status code of 200, 203, 206, 300, 301 or 410 MAY be stored […] unless a cache-control directive prohibits caching.
> A response received with any other status code (e.g. status codes 302 and 307) MUST NOT be returned […] unless there are cache-control directives or another header(s) that explicitly allow it.
This is what `ResponseCacheStrategy::isUncacheable` implements to decide whether a response is not cacheable at all. It differs from `Response::isCacheable` which only returns true if there are actual `Cache-Control` headers.
- > [Section 13.2.3](https://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.2.3): When a response is generated from a cache entry, the cache MUST include a single Age header field in the response with a value equal to the cache entry's current_age.
That's why the implementation **always** adds the `Age` header. It takes the oldest age of any of the responses as common denominator for the content.
- > [Section 14.9.3](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.3): If a response includes an s-maxage directive, then for a shared cache (but not for a private cache), the maximum age specified by this directive overrides the maximum age specified by either the max-age directive or the Expires header.
This effectively means that `max-age`, `s-maxage` and `Expires` must all be kept on the response. My implementation assumes that we can only do that if they exist in **all** of the responses, and then takes the lowest value of any of them. Be aware the implementation might look confusing at first. Due to the fact that the `Age` header might come from another subresponse than the lowest expiration value, the values are stored relative to the current response date and then re-calculated based on the age header.
The Symfony implementation did not and still does not implement the full RFC. As an example, some of the `Cache-Control` headers (like `private` and `no-cache`) MAY actually have a string value, but the implementation only supports boolean. Also, [Custom `Cache-Control` headers](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.6) are currently not merged into the final response.
**ToDo/Questions:**
1. [Section 13.5.2](https://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.5.2) specifies that we must add a [`Warning 214 Transformation applied`](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.46) if we modify the response headers.
2. Should we add an `Expires` headers based on `max-age` if none is explicitly set in the responses? This would essentially provide the same information as `max-age` but with support for HTTP/1.0 proxies/clients.
3. I'm not sure about the implemented handling of the `private` directive. The directive is currently only added to the final response if it is present in all of the subresponses. This can effectively result in no cache-control directive, which does not tell a shared cache that the response must not be cached. However, adding a `private` might also tell a browser to actually cache it, even though non of the other responses asked for that.
4. > [Section 14.9.2](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.2): The purpose of the `no-store` directive is to prevent the inadvertent release or retention of sensitive information […]. The `no-store` directive applies to the entire message, and MAY be sent either in a response or in a request. If sent in a request, a cache MUST NOT store any part of either this request or any response to it. If sent in a response, a cache MUST NOT store any part of either this response or the request that elicited it.
I have not (yet) validated whether the `HttpCache` implementation respects any of this.
5. As far as I understand, the current implementation of [`ResponseHeaderBag::computeCacheControlValue`](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/ResponseHeaderBag.php#L313) is incorrect. `no-cache` means a response [must not be cached by a shared or private cache](https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.1), which overrides `private` automatically.
5. The unit tests are still very limited and I want to add plenty more to test and sort-of describe the implementation or assumptions on the RFC.
/cc @nicolas-grekas
#SymfonyConHackday2018
Commits
-------
893118f978 [HttpKernel] Correctly merging cache directives in HttpCache/ResponseCacheStrategy
* 3.4: (24 commits)
Apply php-cs-fixer rule for array_key_exists()
[Security] Change FormAuthenticator if condition
handles multi-byte characters in autocomplete
speed up tests running them without debug flag
[Translations] added missing Croatian validators
Fix getItems() performance issue with RedisCluster (php-redis)
[VarDumper] Keep a ref to objects to ensure their handle cannot be reused while cloning
IntegerType: reject submitted non-integer numbers
be keen to newcomers
[HttpKernel] Fix possible infinite loop of exceptions
fixed CS
[Validator] Added missing translations for Afrikaans
do not validate non-submitted form fields in PATCH requests
Update usage example in ArrayInput doc block.
[Console] Prevent ArgvInput::getFirstArgument() from returning an option value
[Validator] Fixed duplicate UUID
fixed CS
[EventDispatcher] Fix unknown priority
Avoid mutating the Finder when building the iterator
[Validator] Add the missing translations for the Greek (el) locale
...
* 3.4:
Remove "internal" annotation from datacollector serialization methods
replace mocks with real objects in tests
Fix phpunit 8 compatibility
render integer types with grouping as text input
ignore _method forms in NativeRequestHandler
don't lose int precision with not needed type casts
* 3.4:
Add missing `@internal` annotations
Disable Twig in the profiler menu when Twig is not used
Mark some/most implementations of Serializable as `@internal`
[Config] ensure moving away from Serializable wont break cache:clear
[VarDumper] dont implement Serializable in Stub
[Config] fix compat with wrapping autoloaders
* 4.1:
Bump phpunit bridge cache id
[appveyor] fix create-project phpunit
Fix HttpKernel Debug requirement
Fix heredoc
use final annotation to allow mocking the class
synchronise the form builder docblock
Grammar fix in exception message
fix tests
forward the parse error to the calling code
[Debug][DebugClassLoader] Match more cases for final, deprecated and internal classes / methods extends
ensure compatibility with older PHPUnit mocks
[Security] Do not mix usage of password_*() functions and sodium_*() ones
* 3.4:
Bump phpunit bridge cache id
[appveyor] fix create-project phpunit
Fix HttpKernel Debug requirement
Fix heredoc
use final annotation to allow mocking the class
synchronise the form builder docblock
Grammar fix in exception message
fix tests
forward the parse error to the calling code
[Debug][DebugClassLoader] Match more cases for final, deprecated and internal classes / methods extends
ensure compatibility with older PHPUnit mocks
[Security] Do not mix usage of password_*() functions and sodium_*() ones
* 3.4:
fixed CS
fixed short array CS in comments
fixed CS in ExpressionLanguage fixtures
fixed CS in generated files
fixed CS on generated container files
fixed CS on Form PHP templates
fixed CS on YAML fixtures
fixed fixtures
switched array() to []
* 4.1:
properly fix tests on PHP 5
fix tests on PHP 5
remove doubled dot from exception message
bug #29697 [DI] Fixed wrong factory method in exception (Wojciech Gorczyca)
[Intl] make type-hinted arguments nullable
[DI] Fixed wrong factory method in exception
Changed gender choice types to color
remove no longer needed PHP version checks
remove no longer needed PHP version checks
Fixed groupBy argument value in DefaultChoiceListFactoryTest
[HttpKernel] Correctly Render Signed URIs Containing Fragments
[HttpFoundation] Fix request uri when it starts with double slashes
* 3.4:
properly fix tests on PHP 5
fix tests on PHP 5
bug #29697 [DI] Fixed wrong factory method in exception (Wojciech Gorczyca)
Changed gender choice types to color
remove no longer needed PHP version checks
Fixed groupBy argument value in DefaultChoiceListFactoryTest
[HttpKernel] Correctly Render Signed URIs Containing Fragments
[HttpFoundation] Fix request uri when it starts with double slashes
This PR was merged into the 3.4 branch.
Discussion
----------
[HttpKernel] Correctly Render Signed URIs Containing Fragments
| Q | A
| ------------- | ---
| Branch? | `3.4`
| Bug fix? | yes
| New feature? | no
| BC breaks? | no?
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | n/a
| License | MIT
| Doc PR | n/a
- Rebuild the URL with the computed hash instead of appending it onto the end of the fragment.
- Update unit tests, and add new unit test to cover URIs that include fragments.
Commits
-------
b9ece6bde7 [HttpKernel] Correctly Render Signed URIs Containing Fragments
* 4.1:
Fix: Adjust DocBlock
\"ParserTest->getParserTestData()\" -> only some more tests
access the container getting it from the kernel
[Lock] Pedantic improvements for lock
[EventDispatcher] Fixed phpdoc on interface
update year in license files
[Console] Fix help text for single command applications
Fix random test failure on lock
improve error message when using test client without the BrowserKit component
[Event Dispatcher] fixed 29703: TraceableEventDispatcher reset now sets callStack to null with test to dispatch after reset.
Fixed minor typos
Fix: Method can also return null
[Stopwatch] Fixed phpdoc for category name
* 3.4:
Fix: Adjust DocBlock
\"ParserTest->getParserTestData()\" -> only some more tests
[Lock] Pedantic improvements for lock
[EventDispatcher] Fixed phpdoc on interface
update year in license files
[Console] Fix help text for single command applications
Fix random test failure on lock
improve error message when using test client without the BrowserKit component
[Event Dispatcher] fixed 29703: TraceableEventDispatcher reset now sets callStack to null with test to dispatch after reset.
Fixed minor typos
Fix: Method can also return null
[Stopwatch] Fixed phpdoc for category name
Rebuild the URL with the computed hash instead of appending it onto the end of the URI, preventing incorrect formatting when dealing with URIs containing fragments.
Since https://github.com/symfony/symfony/pull/25733 the Kernel attempts to unlink the legacy container while being built.
This throws an error if the file did not exist, for example on a clean install, on the build, which is then silenced.
That's fine on production systems, but on our build we have enabled "xdebug.scream" in order to visualise every errors, which basically un-silences the errors. I believe there should not be a need to silence anything on a usual, clean usage of the system.
Making this `unlink` conditional fixes it.
Could you please approve and merge this PR?
Thanks
* 4.1:
[Routing] fix trailing slash redirections involving a trailing var
[EventDispatcher] Revers event tracing order
[Security] Prefer clone over unserialize(serialize()) for user refreshment
[Console] OutputFormatter: move strtolower to createStyleFromString
Adjust tests to work in the armhf architecture. Fixes#29281.
Vietnamese translations improvement
[Form] Fixed FormErrorIterator class phpdoc
Renamed test controller from Controller to TestController so it doesn't show up in the IDE autocomplete.
Don't use he in docs when its not needed
EventSubscriberInterface isn't a man
fixed public directory of web server and assets install when configured in composer.json
* 3.4:
[EventDispatcher] Revers event tracing order
[Security] Prefer clone over unserialize(serialize()) for user refreshment
[Console] OutputFormatter: move strtolower to createStyleFromString
Adjust tests to work in the armhf architecture. Fixes#29281.
Vietnamese translations improvement
[Form] Fixed FormErrorIterator class phpdoc
Renamed test controller from Controller to TestController so it doesn't show up in the IDE autocomplete.
Don't use he in docs when its not needed
EventSubscriberInterface isn't a man
fixed public directory of web server and assets install when configured in composer.json
* 4.1:
Fix typos in doc blocks
[Debug] ignore underscore vs backslash namespaces in DebugClassLoader
[TwigBridge][Form] Prevent multiple rendering of form collection prototypes
[FrameworkBundle] fix describing routes with no controllers
[DI] move RegisterServiceSubscribersPass before DecoratorServicePass
Update ValidationListener.php
[Yaml] ensures that the mb_internal_encoding is reset to its initial value
[WebLink] Fixed documentation link
[Security] getTargetPath of TargetPathTrait must return string or null
[Hackday][Serializer] Deserialization ignores argument type hint from phpdoc for array in constructor argument
Optimize perf by replacing call_user_func with dynamic vars
[Routing] fix dumping same-path routes with placeholders
[Security] defer log message in guard authenticator
[Validator] Added IBAN format for Vatican City State
merge conflicts
filter out invalid Intl values
filter out invalid language values
[Validator] Fixed grouped composite constraints
[Form] Filter arrays out of scalar form types
Fix HeaderBag::get phpdoc
This PR was merged into the 4.1 branch.
Discussion
----------
Optimize perf by replacing call_user_func with dynamic variables
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets |
| License | MIT
| Doc PR |
This provides similar boost as in https://github.com/symfony/symfony/pull/29245, but on more places and without complexity increase. Check eg. https://github.com/fab2s/call_user_func for proof
Fabpot failure unrelated
Commits
-------
0c6ef01713 Optimize perf by replacing call_user_func with dynamic vars
* 4.1:
fix cs
[Validator] Allow `ConstraintViolation::__toString()` to expose codes that are not null or emtpy strings
fix type for $value in DocBlock
[WebProfilerBundle] Fix title case
Fix wrapped loop of event listener
[DI] fix edge case in InlineServiceDefinitionsPass
undeprecate the single-colon notation for controllers
Update HttpKernel.php
* 3.4:
fix cs
[Validator] Allow `ConstraintViolation::__toString()` to expose codes that are not null or emtpy strings
fix type for $value in DocBlock
Fix wrapped loop of event listener
Update HttpKernel.php
* 4.1:
fix cs
fix cs
fix cs
SCA: consolidate non empty array checks across codebase
[cs] correct invalid @param types
[Bridge/PhpUnit] Use composer to download phpunit
[DI] fix taking lazy services into account when dumping the container
[Form] Fixed empty data for compound date interval
[Cache] fix optimizing Psr6Cache for AdapterInterface pools
deal with explicitly enabled workflow nodes
* 3.4:
fix cs
SCA: consolidate non empty array checks across codebase
[cs] correct invalid @param types
[Bridge/PhpUnit] Use composer to download phpunit
[DI] fix taking lazy services into account when dumping the container
[Form] Fixed empty data for compound date interval
[Cache] fix optimizing Psr6Cache for AdapterInterface pools
deal with explicitly enabled workflow nodes
* 4.1:
Undeprecate the single-colon notation for controllers
Command::addOption should allow int in $default
Update symfony links to https
[Form] Fixed keeping hash of equal \DateTimeInterface on submit
[PhpUnitBridge] Fix typo
[Routing] generate(null) should throw an exception
[Form] Minor fixes in docs and cs
[Workflow] Made code simpler
[Config] Unset key during normalization
[Form] Fixed empty data for compound date types
invalidate forms on transformation failures
[FrameworkBundle] fixed guard event names for transitions
method buildTransitionBlockerList returns TransitionBlockerList of expected transition
[FrameworkBundle] fixed guard event names for transitions
[PropertyAccessor] Fix unable to write to singular property using setter while plural adder/remover exist
This notation is the only way to support controllers as services in the 3.4
LTS version.
This deprecation has only a very small benefit for the Symfony codebase (the
amount of code involved is very small), but has a huge cost for the community
which cannot avoid this deprecation without dropping support for the LTS or
making crazy logic to switch routing files (as they cannot switch things
inline in YAML or XML files).
This deprecation will be delayed until a future 5.x version, when the current
LTS will be 4.4 (which supports the new notation).
* 4.1:
[Form] Hardened test suite for empty data
Bump phpunit XSD version to 5.2
[Fwb][EventDispatcher][HttpKernel] Fix getClosureScopeClass usage to describe callables
Add required key attribute
* 3.4:
[Form] Hardened test suite for empty data
Bump phpunit XSD version to 5.2
[Fwb][EventDispatcher][HttpKernel] Fix getClosureScopeClass usage to describe callables
Add required key attribute
Some attributes being used in the phpunit configuration files, namely
failOnRisky and failOnWarning were introduced in phpunit 5.2.0. The
Composer configuration shows that tests should run with old versions of
phpunit, but phpunit only validates the configuration against the XSD
since phpunit 7.2.0.
These changes can be tested as follows:
wget http://schema.phpunit.de/5.2/phpunit.xsd
xargs xmllint --schema phpunit.xsd 1>/dev/null
find src -name phpunit.xml.dist| xargs xmllint --schema phpunit.xsd 1>/dev/null
See 7e06a82806
See 46e3745a03/composer.json (L98)
* 4.1:
[VarDumper] fix dump of closures created from callables
[DI] fix dumping inlined services
Add framework asset changes to upgrade 3.0 guide
[Travis] Bump ext-mongodb to 1.5.2 on Travis
[DI] dont track classes/interfaces used to compute autowiring error messages
[DI] fix GraphvizDumper ignoring inline definitions
bumped Symfony version to 4.1.8
updated VERSION for 4.1.7
updated CHANGELOG for 4.1.7
bumped Symfony version to 3.4.19
updated VERSION for 3.4.18
updated CHANGELOG for 3.4.18
bumped Symfony version to 2.8.48
updated VERSION for 2.8.47
update CONTRIBUTORS for 2.8.47
updated CHANGELOG for 2.8.47
Fix ini_get() for boolean values
* 3.4:
[VarDumper] fix dump of closures created from callables
[DI] fix dumping inlined services
Add framework asset changes to upgrade 3.0 guide
[Travis] Bump ext-mongodb to 1.5.2 on Travis
[DI] dont track classes/interfaces used to compute autowiring error messages
[DI] fix GraphvizDumper ignoring inline definitions
bumped Symfony version to 3.4.19
updated VERSION for 3.4.18
updated CHANGELOG for 3.4.18
bumped Symfony version to 2.8.48
updated VERSION for 2.8.47
update CONTRIBUTORS for 2.8.47
updated CHANGELOG for 2.8.47
Fix ini_get() for boolean values
This PR was merged into the 3.4 branch.
Discussion
----------
[VarDumper] fix dump of closures created from callables
| Q | A
| ------------- | ---
| Branch? | 3.4
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | -
| License | MIT
| Doc PR | -
We are missing displaying full information about closures created using `ReflectionMethod::getClosure()` or `Closure::fromCallable()`.
This PR fixes it. For VarDumper but also other places where we have logic to display them.
Commits
-------
1c1818b876 [VarDumper] fix dump of closures created from callables