* 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#30406).
Discussion
----------
Removed non-existing parameters for LogoutUrlGenerator calls
| Q | A
| ------------- | ---
| Branch? | 3.4 <!-- see below -->
| Bug fix? | no
| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks? | no <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass? | yes <!-- please add some, will be required by reviewers -->
| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->
| License | MIT
| Doc PR | symfony/symfony-docs#... <!-- required for new features -->
Is there a reason these arguments are in place, though they dont actually exist as parameters for `LogoutUrlGenerator::getLogoutPath` and `::getLogoutUrl`?
see https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/Security/Http/Logout/LogoutUrlGenerator.php#L76
and https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/Security/Http/Logout/LogoutUrlGenerator.php#L88
If there is no reason, this PR can be merged, because this parameter makes no sense there. ;-)
Commits
-------
d3ee2b676e Removed non-existing parameters for LogoutUrlGenerator calls
This PR was squashed before being merged into the 4.2 branch (closes#30383).
Discussion
----------
[WebProfilerBundle] toolbar: invisible route name in Firefox
| Q | A
| ------------- | ---
| Branch? | 4.2
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #29858
| License | MIT
| Doc PR | -
This fixes#29858, fixing a bug that prevents Route name displaying in debug toolbar on Firefox
Commits
-------
6b6bd453e1 [WebProfilerBundle] toolbar: invisible route name in Firefox
This PR was merged into the 4.2 branch.
Discussion
----------
Drop spurious execution bit
| Q | A
| ------------- | ---
| Branch? | 4.2 for bug fixes
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | none
| License | MIT
| Doc PR | none
The execution bit seems to have been added by mistake in #28626.
Commits
-------
93dab5c381 Drop spurious execution bit
This PR was merged into the 3.4 branch.
Discussion
----------
[Validator] Add the missing translations for the Latvian ("lv") locale #30174Fixes#30174
| Q | A
| ------------- | ---
| Branch? | 3.4
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #30174
| License | MIT
| Doc PR | -
Commits
-------
86b8c253c7 [Validator] Add the missing translations for the Latvian ("lv") locale
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
This PR was squashed before being merged into the 3.4 branch (closes#30363).
Discussion
----------
Fixed the DebugClassLoader compatibility with eval()'d code on Darwin
| Q | A
| ------------- | ---
| Branch? | 3.4
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #30362
| License | MIT
When a class is defined in an `eval()` block, the reported file name is `file_name.php(123) : eval()'d code`, which prevents `DebugClassLoader::darwinRealpath()` from locating/normalizing the file name, and triggers a notice.
Commits
-------
6c2aa2446d Fixed the DebugClassLoader compatibility with eval()'d code on Darwin
This PR was merged into the 4.2 branch.
Discussion
----------
[FrameworkBundle] Removed eval() from KernelShutdownOnTearDownTrait
| Q | A
| ------------- | ---
| Branch? | 4.2
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| License | MIT
Apart from triggering the #30362 bug, the `eval()` block from #30124 also broke my workflow: static code analyzers don't like this, and the trait even crashes PHPStan.
It may also bring up other compatibility issues to other people (ie: I know companies that completely disable `eval()` on their servers).
As it was only required to keep the trait compatible with PHP 5.x, it is unnecessary on 4.x that requires PHP 7.1+, and this PR removes it on the 4.2 branch.
Commits
-------
324b70afa3 Removed eval() from KernelShutdownOnTearDownTrait
If passing context to getTypes, it checks value of $context['enable_constructor_extraction'] for true/false or the constructor value of enableConstructorExtraction and should then populate fromConstructor if necessary. The missing brackets around the first part of this check mean that fromConstructor is only applied if context is not set.
This fixes the issuse described at [symfony/symfony-docs#10969](https://github.com/symfony/symfony-docs/pull/10969)
This PR was merged into the 3.4 branch.
Discussion
----------
[Validator] Update Serbian translation file
| Q | A
| ------------- | ---
| Branch? | 3.4 <!-- see below -->
| Bug fix? | no
| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks? | no <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass? | yes <!-- please add some, will be required by reviewers -->
| Fixed tickets | #30189 <!-- #-prefixed issue number(s), if any -->
| License | MIT
| Doc PR | / <!-- required for new features -->
<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
- Bug fixes must be submitted against the lowest branch where they apply
(lowest branches are regularly merged to upper ones so they get the fixes too).
- Features and deprecations must be submitted against the master branch.
-->
I am not sure about some translations, if someone could check these, I would appreciate it.
Also, I've found that singular/plural translations have 3 translations. I am not sure if that's a mistake or not. I removed the third translation, but if I was wrong, I'll be happy to put it back.
Commits
-------
9e9a57a544 [Validator] Update Serbian translation file
* 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
...
This PR was squashed before being merged into the 3.4 branch (closes#30347).
Discussion
----------
[Security] Change FormAuthenticator if condition
| Q | A
| ------------- | ---
| Branch? | 3.4
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #30341
| License | MIT
| Doc PR | -
I changed the if condition in `SimpleFormAuthenticationListener` and `UsernamePasswordFormAuthenticationListener` based on the solution provided by @nikic in issue #30341
#OpenSourceFriday
Commits
-------
67ae121b2e [Security] Change FormAuthenticator if condition
This PR was merged into the 3.4 branch.
Discussion
----------
[Console] handles multi-byte characters in autocomplete
fixes#29966
| Q | A
| ------------- | ---
| Branch? | 3.4 <!-- see below -->
| Bug fix? | yes
| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks? | no <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass? | yes <!-- please add some, will be required by reviewers -->
| Fixed tickets | #29966 <!-- #-prefixed issue number(s), if any -->
| License | MIT
| Doc PR | - <!-- required for new features -->
I used the `mb_ord` to detect whether the amount of bytes read is valid before proceeding. I limit the number of bytes read to 4 before giving up because characters can use at most 4 bytes.
The test passes with or without the fix though.
Commits
-------
47320a63e2 handles multi-byte characters in autocomplete
This PR was merged into the 3.4 branch.
Discussion
----------
Fix getItems() performance issue with RedisCluster (php-redis)
| Q | A
| ------------- | ---
| Branch? | 3.4
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | TBD
| License | MIT
On any kind of multi loads, including tags loading where it's always the case, current code leads to an explosion of Redis lookups affecting performance on RedisCluster _(as it does not support pipeline)_.
This backports the code for mget() usage from 4.x in order to fix it.
It's done with one small improvment which would also be relevant for 4.x, only using pipeline on cluster on predis as mget is more efficient (ref redis doc).
Commits
-------
178506e72c Fix getItems() performance issue with RedisCluster (php-redis)
This PR was merged into the 3.4 branch.
Discussion
----------
[VarDumper] Keep a ref to objects to ensure their handle cannot be reused while cloning
| 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 | -
Fixes the root issue that led to https://github.com/symfony/symfony/pull/30311#issuecomment-466457434
Commits
-------
29a0683107 [VarDumper] Keep a ref to objects to ensure their handle cannot be reused while cloning
This PR was merged into the 3.4 branch.
Discussion
----------
[FrameworkBundle][SecurityBundle] speed up tests running them without debug flag
| Q | A
| ------------- | ---
| Branch? | 3.4
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets |
| License | MIT
| Doc PR |
Commits
-------
dfb924f6a9 speed up tests running them without debug flag
This PR was merged into the 3.4 branch.
Discussion
----------
[Translations] added missing Croatian validators
| Q | A
| ------------- | ---
| Branch? | 3.4 up to 4.2 for bug fixes
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #30167
| License | MIT
[Validator] Add the missing translations for the Croatian ("hr") locale #30167
Commits
-------
b9ce861ff6 [Translations] added missing Croatian validators
