This PR was squashed before being merged into the 3.3-dev branch (closes#22043).
Discussion
----------
Refactor stale-while-revalidate code in HttpCache, add a (first?) test for it
| Q | A
| ------------- | ---
| Branch? | 2.8
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets |
| License | MIT
| Doc PR |
I came up with this while trying to hunt a production bug related to handling of stale cache entries under the condition of a busy backend (also see #22033).
It's just a refactoring to make the code more readable plus a new test.
Commits
-------
b14057c88a Refactor stale-while-revalidate code in HttpCache, add a (first?) test for it
This PR was submitted for the 2.8 branch but it was merged into the 3.3-dev branch instead (closes#22033).
Discussion
----------
Minor cleanups in HttpCache, especially centralize Date header fixup
| Q | A
| ------------- | ---
| Branch? | 2.8
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets |
| License | MIT
| Doc PR |
I am trying to hunt some issues where the HTTP cache does not revalidate responses (or does not use a cached response?) in the edge case of having `s-maxage = 0` and possibly a backend that's slow to respond.
I don't see yet what exactly my problem is there, but it must somehow be related with the `Date` header handling and/or age calculations in the `HTTPCache`.
As a first step, I'd like to suggest this cleanup in `HTTPCache`. Especially, it would be helpful if we could make sure that there is just one place where we set the `Date` header if the backend does not send one (?). This makes sure we always have this header to base later age calculations on it.
Commits
-------
30639c6af6 Minor cleanups in HttpCache, especially centralize Date header fixup
This PR was merged into the 3.3-dev branch.
Discussion
----------
[FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Tests pass? | (no test yet)
| Fixed tickets | -
| License | MIT
| Doc PR | -
Talking with @simensen and @weaverryan, we wondered if we could leverage the `ArgumentResolver` mechanism to make it inject services on demand, using e.g. autowiring.
```php
class PostController
{
public function indexAction(Request $request, PostRepository $postRepository)
{
// PostRepository comes from the container
$postRepository->findAll(); // ...
}
}
```
This PR achieves that, using a new "controller.service_arguments" tag. Typically:
```yaml
services:
AppBundle\Controller\PostController:
autowire: true
tags:
- name: controller.service_arguments
```
It also supports with explicit wiring (thus doesn't necessarily require autowiring if you don't want to use it):
```yaml
services:
AppBundle\Controller\PostController:
tags:
- name: controller.service_arguments
action: fooAction
argument: logger
id: my_logger
```
~~The attached diff is bigger than strictly required for now, until #21770 is merged.~~
Todo:
- [x] rebase on top of #21770 when merged
- [x] add tests
- [x] add cleaning pass to remove empty service locators
Commits
-------
9c6e672780 [FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions
This PR was merged into the 3.3-dev branch.
Discussion
----------
[lock] Rename Quorum into Strategy
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | yes (not consistent naming)
| New feature? | no
| BC breaks? | yes (but version 3.4 not yet released)
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | none
| License | MIT
| Doc PR |
The term `Quorum` in Interface is confusing an not consistent with the Symfony project.
This PR switch to naming `Strategy\StrategyInterface` (like in adapter i `Cache` and `Ldap` component)
Commits
-------
1e9671b993 Rename Quorum into Strategy
This PR was merged into the 3.3-dev branch.
Discussion
----------
[Lock] Don't call blindly the redis client
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets |
| License | MIT
| Doc PR |
Actual code rely on controls on the constructor. This PR add an assertion to avoid futur bugs
Commits
-------
e4db018b6d Don't call blindly the redis client
This PR was merged into the 2.7 branch.
Discussion
----------
[Security] Fixed roles serialization on token from user object
| Q | A |
| --- | --- |
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #14274 |
| License | MIT |
| Doc PR | - |
This PR fixes the serialization of tokens when using `Role` objects provided from the user. Indeed, there were actually a reference issue that can causes fatal errors like the following one:
```
FatalErrorException in RoleHierarchy.php line 43:
Error: Call to a member function getRole() on string
```
Here is a small code example to reproduce and its output:
``` php
$user = new Symfony\Component\Security\Core\User\User('name', 'password', [
new Symfony\Component\Security\Core\Role\Role('name')
]);
$token = new Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken($user, 'password', 'providerKey', $user->getRoles());
$serialized = serialize($token);
$unserialized = unserialize($serialized);
var_dump($unserialized->getRoles());
```
Before:
```
array(1) { [0]=> bool(true) }
```
After:
```
array(1) { [0]=> object(Symfony\Component\Security\Core\Role\Role)#15 (1) {["role":"Symfony\Component\Security\Core\Role\Role":private]=> string(4) "name" } }
```
Thank you
Commits
-------
dfa7f5020e [Security] Fixed roles serialization on token from user object
* 3.2:
Fixed pathinfo calculation for requests starting with a question mark.
[HttpFoundation] Fix missing handling of for/host/proto info from "Forwarded" header
[Validator] Add object handling of invalid constraints in Composite
[WebProfilerBundle] Remove uneeded directive in the form collector styles
removed usage of $that
HttpCache: New test for revalidating responses with an expired TTL
[Serializer] [XML] Ignore Process Instruction
[Security] simplify the SwitchUserListenerTest
Revert "bug #21841 [Console] Do not squash input changes made from console.command event (chalasr)"
[HttpFoundation] Fix Request::getHost() when having several hosts in X_FORWARDED_HOST
This PR was merged into the 3.3-dev branch.
Discussion
----------
[Security][SecurityBundle] Enhance automatic logout url generation
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | yes
| Tests pass? | yes
| Fixed tickets | N/A
| License | MIT
| Doc PR | N/A
This should help whenever:
- [the token does not implement the `getProviderKey` method](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Http/Logout/LogoutUrlGenerator.php#L89-L99)
- you've got multiple firewalls sharing a same context but a logout listener only define on one of them.
##### Behavior:
> When not providing the firewall key:
>
>- Try to find the key from the token (unless it's an anonymous token)
>- If found, try to get the listener from the key. If the listener is found, stop there.
>- Try from the injected firewall key. If the listener is found, stop there.
>- Try from the injected firewall context. If the listener is found, stop there.
>
>The behavior remains unchanged when providing explicitly the firewall key. No fallback.
Commits
-------
5b7fe852aa [Security][SecurityBundle] Enhance automatic logout url generation
This PR was squashed before being merged into the 3.3-dev branch (closes#22112).
Discussion
----------
Minor PR fixes
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | yes-ish
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->
| License | MIT
| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->
cc @fabpot my bad :)
Commits
-------
0728fb91b8 typo
036b0414d6 Minor PR fixes
This PR was merged into the 3.3-dev branch.
Discussion
----------
[WebProfilerBundle] Improved cookie traffic
| Q | A
| ------------- | ---
| Branch? | "master"
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | comma-separated list of tickets fixed by the PR, if any
| License | MIT
| Doc PR | reference to the documentation PR, if any
Relates to #20569 in terms of getting _all_ the cookies.
Commits
-------
171c6d100e [WebProfilerBundle] Improved cookie traffic
This PR was squashed before being merged into the 2.8 branch (closes#22036).
Discussion
----------
Set Date header in Response constructor already
| Q | A
| ------------- | ---
| Branch? | 2.8
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets |
| License | MIT
| Doc PR |
Setting the `Date` header in the `Response` constructor has been removed in #14912 and changed to a more lazy approach in `getDate()`.
That way, methods like `getAge()`, `getTtl()` or `isFresh()` cause side effects as they eventually call `getDate()` and the Request "starts to age" once you call them.
I don't know if this would be a nice test, but current behaviour is
```php
$response = new Response();
$response->setSharedMaxAge(10);
sleep(20);
$this->assertTrue($response->isFresh());
sleep(5);
$this->assertTrue($response->isFresh());
sleep(5);
$this->assertFalse($response->isFresh());
```
A particular weird case is the `isCacheable()` method, because it calls `isFresh()` only under certain conditions, like particular status codes, no `ETag` present etc. This symptom is also described under "Cause of the problem" in #19390, however the problem is worked around there in other ways.
So, this PR suggests to effectively revert #14912.
Additionally, I'd like to suggest to move this special handling of the `Date` header into the `ResponseHeaderBag`. If the `ResponseHeaderBag` guards that we always have the `Date`, we would not need special logic in `sendHeaders()` and could also take care of https://github.com/symfony/symfony/pull/14912#issuecomment-110105215.
Commits
-------
3a7fa7ede2 Set Date header in Response constructor already
This PR was squashed before being merged into the 3.3-dev branch (closes#19887).
Discussion
----------
Sort alternatives alphabetically when a command is not found
| Q | A |
| --- | --- |
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #10893 |
| License | MIT |
| Doc PR | - |
Commits
-------
ba6c9464ea Sort commands like a human would do
f04b1bd72f Sort alternatives alphabetically when a command is not found
This PR was merged into the 3.3-dev branch.
Discussion
----------
[Security] json auth listener should not produce a 500 response on bad request format
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | N/A
| License | MIT
| Doc PR | N/A
To me, it looks wrong to simply throw a `BadCredentialsException` in the wild, which produces a 500 (unless an entrypoint handles it, which you probably don't have on a json login firewall). There isn't any server error, the client request originated the error due to a wrong format.
Instead, the listener should give a chance to the failure handler to resolve it, and return a proper 4XX response. (BTW, the `UsernamePasswordFormAuthenticationListener` also throws a similar `BadCredentialsException` on a too long submitted username, which is caught and forwarded to the failure handler)
Better diff: https://github.com/symfony/symfony/pull/22034/files?w=1
BTW, should we have another exception type like `BadCredentialsFormatException` or whatever in order to distinct a proper `BadCredentialsException` from a format issue in a failure listener?
Commits
-------
cb175a41c3 [Security] json auth listener should not produce a 500 response on bad request format
This PR was submitted for the 3.2 branch but it was merged into the 2.7 branch instead (closes#22022).
Discussion
----------
[Validator] fix URL validator to detect non supported chars according to RFC 3986
| Q | A
| ------------- | ---
| Branch? | 3.2
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #21961
| License | MIT
| Doc PR | none
Commits
-------
3599c476bf [Validator] fix URL validator to detect non supported chars according to RFC 3986
This PR was merged into the 2.7 branch.
Discussion
----------
[Security] simplify the SwitchUserListenerTest
| Q | A
| ------------- | ---
| Branch? | 2.7
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets |
| License | MIT
| Doc PR |
While working on #22048 I noticed that the `SwitchUserListenerTest` was more complicated than necessary by mocking a lot of stuff that didn't need to be mocked.
Commits
-------
923bbdbf9f [Security] simplify the SwitchUserListenerTest
This PR was squashed before being merged into the 3.3-dev branch (closes#20885).
Discussion
----------
[Console] Option to disable stty
| Q | A
| ------------- | ---
| Branch? |
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Tests pass? |
| Fixed tickets |
| License | MIT
| Doc PR | reference to the documentation PR, if any
Shall fix problems in Windows based environments if e.g. git is installed and stty is therefore found but writes only cryptic rubbish into the cmd. In the case of console questions it is also possible that input can't be read properly by console component.
Commits
-------
a189a6c52e [Console] Option to disable stty
This PR was merged into the 3.3-dev branch.
Discussion
----------
[HttpFoundation] Add $trustedHeaderSet arg to Request::setTrustedProxies() - deprecate not setting it
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | yes
| Tests pass? | no tests yet
| Fixed tickets | -
| License | MIT
| Doc PR | -
Follow up of #18688
PR adds a second `$trustedHeaderSet` argument to `Request::setTrustedProxies()`, can be either `Request::HEADER_FORWARDED` or `Request::HEADER_X_FORWARDED_ALL` to set which header to trust from your proxies - the idea being that without this info, one will get some `ConflictingHeadersException`, but those may be lost in the logs.
Commits
-------
d3c960493c [HttpFoundation] Add $trustedHeaderSet arg to Request::setTrustedProxies() - deprecate not setting it
* 2.8:
[HttpFoundation] Fix missing handling of for/host/proto info from "Forwarded" header
[Validator] Add object handling of invalid constraints in Composite
[WebProfilerBundle] Remove uneeded directive in the form collector styles
Revert "bug #21841 [Console] Do not squash input changes made from console.command event (chalasr)"
[HttpFoundation] Fix Request::getHost() when having several hosts in X_FORWARDED_HOST
This PR was merged into the 2.8 branch.
Discussion
----------
[HttpFoundation] Fix missing handling of for/host/proto info from "Forwarded" header
| Q | A
| ------------- | ---
| Branch? | 2.8
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | -
| License | MIT
| Doc PR | -
We're missing handling of for/host/proto info embedded in the `Forwarded` header, as eg in:
`Forwarded: for=1.1.1.1:443, host=foo.example.com:1234, proto=https, for=2.2.2.2, host=real.example.com:8080`
Commits
-------
04caacb757 [HttpFoundation] Fix missing handling of for/host/proto info from "Forwarded" header
This PR was squashed before being merged into the 2.7 branch (closes#21968).
Discussion
----------
Fixed pathinfo calculation for requests starting with a question mark.
| Q | A
| ------------- | ---
| Branch? | 2.7
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #21967
| License | MIT
| Doc PR |
With improper `strpos` result check calculated pathinfo for requests starting with '?' equals to request itself.
Correct pathinfo for those requests should be '/'.
Commits
-------
43297b45de Fixed pathinfo calculation for requests starting with a question mark.
This PR was merged into the 2.8 branch.
Discussion
----------
Revert "bug #21841 [Console] Do not squash input changes made from console.command event (chalasr)"
| Q | A
| ------------- | ---
| Branch? | 2.8
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #21953, https://github.com/symfony/symfony/issues/22050
| License | MIT
| Doc PR | n/a
A bit frustrated to revert this change since the BC break report lacks of information, making us unable to reproduce nor to look at improving the situation.
I'm going to re-propose this on master, covering the BC break that is identified, fixed and tested using the changes made in #21953. That will let the choice for the reporter to upgrade using the 1 required LOC.
Commits
-------
5af47c40dc Revert "bug #21841 [Console] Do not squash input changes made from console.command event (chalasr)"
This PR was merged into the 3.3-dev branch.
Discussion
----------
[FrameworkBundle] Added about command
| Q | A |
| --- | --- |
| Branch? | "master" |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | comma-separated list of tickets fixed by the PR, if any |
| License | MIT |
| Doc PR | reference to the documentation PR, if any |
Commits
-------
2550eab43c [FrameworkBundle] Added about command
* 2.7:
[Validator] Add object handling of invalid constraints in Composite
[HttpFoundation] Fix Request::getHost() when having several hosts in X_FORWARDED_HOST
This PR was merged into the 2.7 branch.
Discussion
----------
[HttpFoundation] Fix Request::getHost() when having several hosts in X_FORWARDED_HOST
| Q | A
| ------------- | ---
| Branch? | 2.7
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | -
| License | MIT
| Doc PR | -
The first "host" in the list provided by `X_FORWARDED_HOST` should be the one, not the last.
Already the case for "port" and "scheme".
Commits
-------
9a2b2de64f [HttpFoundation] Fix Request::getHost() when having several hosts in X_FORWARDED_HOST
This PR was squashed before being merged into the 3.3-dev branch (closes#21926).
Discussion
----------
[Routing] Optimised dumped matcher
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | -
| License | MIT
| Doc PR | -
TL;DR: I've optimised the PhpMatcherDumper output for a <del>60x</del> 4.4x performance improvement on a collection of ~800 routes by inducing cyclomatic complexity.
[EDIT] The 60x performance boost was only visible when profiling with blackfire, which is quite possibly a result of the cost of profiling playing a part. After doing some more profiling the realistic benefit of the optimisation is more likely to be in the ranges is 1.3x to 4.4x.
After the previous optimisation I began looking at how the PrefixCollection was adding its performance boost. I spotted another way to do this, which has the same theory behind it (excluding groups based on prefixes). The current implementation only groups when one prefix resides in the other. In this new implementation I've created a way to detect common prefixes, which allows for much more efficient grouping. Every time a route is added to the group it'll either merge into an existing group, merge into a new group with a route that has a common prefix, or merge into a new group with an existing group that has a common prefix.
However, when a parameter is present grouping must only be done AFTER that route, this case is accounted for. In all other cases, where there's no collision routes can be grouped freely because if a group was matched other groups wouldn't have matched.
After all the groups are created the groups are optimised. Groups with fewer than 3 children are inlined into the parent group. This is because a group with 2 children would potentially result in 3 prefix checks while if they are inlines it's 2 checks.
Like with the previous optimisation I've profiled this using blackfire. But the match function didn't show up anymore. I've added `usleep` calls in the dumped matcher during profiling, which made it show up again. I've verified with @simensen that this is because the wall time of the function was too small for it to be of any interest. When it DID get detected, because of more tasks running, it would show up with around 250 nanoseconds. In comparison, the previous speed improvement brought the wall time down from 7ms to ~2.5ms on a set of ~800 routes.
Because of the altered grouping behaviour I've not modified the PrefixCollection but I've created a new StaticPrefixCollection and updated the PhpMatcherDumper to use that instead.
Commits
-------
449b6912dc [Routing] Optimised dumped matcher
