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
This PR was squashed before being merged into the 3.3-dev branch (closes#21708).
Discussion
----------
[DI] Add and wire ServiceSubscriberInterface - aka explicit service locators
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Tests pass? | no test yet
| Fixed tickets | #20658
| License | MIT
| Doc PR | -
This PR implements the second and missing part of #20658: it enables objects to declare their service dependencies in a similar way than we do for EventSubscribers: via a static method. Here is the interface and its current description:
```php
namespace Symfony\Component\DependencyInjection;
/**
* A ServiceSubscriber exposes its dependencies via the static {@link getSubscribedServices} method.
*
* The getSubscribedServices method returns an array of service types required by such instances,
* optionally keyed by the service names used internally. Service types that start with an interrogation
* mark "?" are optional, while the other ones are mandatory service dependencies.
*
* The injected service locators SHOULD NOT allow access to any other services not specified by the method.
*
* It is expected that ServiceSubscriber instances consume PSR-11-based service locators internally.
* This interface does not dictate any injection method for these service locators, although constructor
* injection is recommended.
*
* @author Nicolas Grekas <p@tchwork.com>
*/
interface ServiceSubscriberInterface
{
/**
* Returns an array of service types required by such instances, optionally keyed by the service names used internally.
*
* For mandatory dependencies:
*
* * array('logger' => 'Psr\Log\LoggerInterface') means the objects use the "logger" name
* internally to fetch a service which must implement Psr\Log\LoggerInterface.
* * array('Psr\Log\LoggerInterface') is a shortcut for
* * array('Psr\Log\LoggerInterface' => 'Psr\Log\LoggerInterface')
*
* otherwise:
*
* * array('logger' => '?Psr\Log\LoggerInterface') denotes an optional dependency
* * array('?Psr\Log\LoggerInterface') is a shortcut for
* * array('Psr\Log\LoggerInterface' => '?Psr\Log\LoggerInterface')
*
* @return array The required service types, optionally keyed by service names
*/
public static function getSubscribedServices();
}
```
We could then have eg a controller-as-a-service implement this interface, and be auto or manually wired according to the return value of this method - using the "kernel.service_subscriber" tag to do so.
eg:
```yaml
services:
App\Controller\FooController:
arguments: [service_container]
tags:
- name: kernel.service_subscriber
key: logger
service: monolog.logger.foo_channel
```
The benefits are:
- it keeps the lazy-behavior gained by service locators / container injection
- it allows the referenced services to be made private from the pov of the main Symfony DIC - thus enables some compiler optimizations
- it makes dependencies autowirable (while keeping manual wiring possible)
- it does not add any strong coupling at the architecture level
- and most importantly and contrary to regular container injection, *it makes dependencies explicit* - each classes declaring which services it consumes.
Some might argue that:
- it requires to be explicit - thus more verbose. Yet many others think it's a good thing - ie it's worth it.
- some coupling happens at the dependency level, since you need to get the DI component to get the interface definition. This is something that the PHP-FIG could address at some point.
Commits
-------
c5e80a2b09 implement ServiceSubscriberInterface where applicable
9b7df39865 [DI] Add and wire ServiceSubscriberInterface
This PR was merged into the 3.3-dev branch.
Discussion
----------
[Yaml] ParseException: pcre.backtrack_limit reached
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | no
| Fixed tickets | -
| License | MIT
| Doc PR | -
while merging 3.2 into master, I noticed that `testCanParseVeryLongValue` is triggering this error on master, due to this regexp that we added for handling yaml tags. This regexp needs to be fixed so that we can merge the test case.
ping @GuilhemN
Commits
-------
f0256f1aa5 [Yaml] Fix pcre.backtrack_limit reached
This PR was merged into the 3.3-dev branch.
Discussion
----------
[FrameworkBundle][Serializer] Add option to register a circular_reference_handler
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | -
| License | MIT
| Doc PR | -
---
Right now, it's quite hard (especially for new comers) to register a CircularReferenceHandler:
---
This PR introduce an option to wire a service to the internal Object Normalizer.
Commits
-------
0a638f5352 [FrameworkBundle][Serializer] Add option to register a "circular_reference_handler"
This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes#21208).
Discussion
----------
[Validator] Add object handling of invalid constraints in Composite
| Q | A
| ------------- | ---
| Branch? | 2.7
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #21206
| License | MIT
| Doc PR | n/a
This PR fixes a minor bug described in #21206. The constraint `Symfony\Component\Validator\Constraints\Composite` doesn't check in it's exception handling if the wrongly created instance of a nested constraint is an object, which is the expected type for a constraint.
Commits
-------
4bd2c22871 [Validator] Add object handling of invalid constraints in Composite
This PR was merged into the 3.3-dev branch.
Discussion
----------
[DI] Deprecate Container::isFrozen and introduce isCompiled
| Q | A |
| --- | --- |
| Branch? | "master" |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | yes |
| 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 |
This deprecates the concept of freezing a container, implied by `Container::isFrozen`. However, freezing happens due compilation (`Container::compile`). So having just `isCompiled` instead seems more intuitive, and plays along well with `ContainerBuilder`.
Before/After;
- `Container::isFrozen`
- Checks if the parameter bag is frozen, but is deprecated in 3.2
- In 4.0 this methods does not exists and can be replaced with `getParameterBag() instanceof FrozenParameterBag` _or_ `isCompiled()`. Depending on what you want (to clarify; the behavior is different when passing a frozen bag to the constructor)
- `Container::isCompiled`
- Truly checks if `compile()` has ran, and is a new feature
- `ContainerBuilder::merge` etc.
- Now uses `isCompiled` instead of `isFrozen`, ie. we allow for it till compilation regarding the state of the paramater bag
Commits
-------
6abd312800 [DI] Deprecate Container::isFrozen and introduce isCompiled
This PR was merged into the 3.3-dev branch.
Discussion
----------
[Console] Exclude empty namespaces in text descriptor
| Q | A |
| --- | --- |
| Branch? | "master" |
| Bug fix? | yes |
| New feature? | no |
| 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 |
Before:
```
$ bin/console
doctrine
doctrine:mapping:convert [orm:convert:mapping] Convert mapping information between supported formats.
orm <----
router
router:match Helps debug routes by simulating a path info match
$ bin/console list orm
[Symfony\Component\Debug\Exception\ContextErrorException]
Warning: max(): Array must contain at least one element
$ bin/console list generate
Available commands for the "generate" namespace:
generate:bundle Generates a bundle
generate:command Generates a console command
generate:controller Generates a controller
```
After:
```
$ bin/console
doctrine
doctrine:mapping:convert [orm:convert:mapping] Convert mapping information between supported formats.
router
router:match Helps debug routes by simulating a path info match
$ bin/console list orm
Available commands for the "orm" namespace:
orm:convert:mapping Convert mapping information between supported formats.
$ bin/console list generate
Available commands for the "generate" namespace:
generate:bundle Generates a bundle
generate:command Generates a console command
generate:controller Generates a controller
generate:doctrine:crud Generates a CRUD based on a Doctrine entity
generate:doctrine:entities Generates entity classes and method stubs from your mapping information
generate:doctrine:entity Generates a new Doctrine entity inside a bundle
generate:doctrine:form Generates a form type class based on a Doctrine entity
```
Overrules #19776 but also includes other fixes related to aliases that popped up when writing tests 👍
Commits
-------
d5a7608036 [Console] Exclude empty namespaces in text descriptor
This PR was squashed before being merged into the 3.3-dev branch (closes#21093).
Discussion
----------
[Lock] Create a lock component
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Tests pass? | they will
| Fixed tickets | #20382
| License | MIT
| Doc PR | symfony/symfony-docs#7364
This PR aim to add a new component Lock going further than the FileSystem\LockHandler by allowing remote backend (like Redis, memcache, etc)
Inspired by
## Usage
The simplest way to use lock is to inject an instance of a Lock in your service
```php
class MyService
{
private $lock;
public function __construct(LockInterface $lock)
{
$this->lock = $lock;
}
public function run()
{
$this->lock->acquire(true);
// If I'm here, no exception had been raised. Lock is acquired
try {
// do my job
} finally {
$this->lock->release();
}
}
}
```
Configured with something like
```yaml
services:
app.my_service:
class: AppBundle\MyService
arguments:
- app.lock.my_service
app.lock.my_service:
class: Symfony\Component\Lock\Lock
factory: ['@locker', createLock]
arguments: ['my_service']
```
If you need to lock serveral resource on runtime, wou'll nneed to inject the LockFactory.
```php
class MyService
{
private $lockFactory;
public function __construct(LockFactoryInterface $lockFactory)
{
$this->lockFactory = $lockFactory;
}
public function run()
{
foreach ($this->items as $item) {
$lock = $this->lockFactory->createLock((string) $item);
try {
$lock->acquire();
} catch (LockConflictedException $e) {
continue;
}
// When I'm here, no exception had been, raised. Lock is acquired
try {
// do my job
} finally {
$lock->release();
}
}
}
}
```
Configured with something like
```yaml
services:
app.my_service:
class: AppBundle\MyService
arguments:
- '@locker'
```
This component allow you to refresh an expirable lock.
This is usefull, if you run a long operation split in several small parts.
If you lock with a ttl for the overall operatoin time and your process crash, the lock will block everybody for the defined TTL.
But thank to the refresh method, you're able to lock for a small TTL, and refresh it between each parts.
```php
class MyService
{
private $lock;
public function __construct(LockInterface $lock)
{
$this->lock = $lock;
}
public function run()
{
$this->lock->acquire(true);
try {
do {
$finished = $this->performLongTask();
// Increase the expire date by 300 more seconds
$this->lock->refresh();
} while (!$finished)
// do my job
} finally {
$this->lock->release();
}
}
}
```
## Naming anc implementation choise
```
$lock->acquire()
vs
$lock->lock()
```
Choose to use acquire, because this component is full of `lock` Symfony\Component\Lock\Lock::Lock` raised a E_TOO_MANY_LOCK in my head.
```
$lock->acquire(false);
$lock->acquire(true);
vs
$lock->aquire()
$lock->waitAndAquire()
```
Not a big fan of flag feature and 2. But I choose to use the blocking flag to offer a simple (and common usecase) implementation
```
$lock = $factory->createLock($key);
$lock->acquire();
vs
$lock->aquire($key)
```
I choose to a the pool of locks implementation. It allow the user to create 2 instances and use cross lock even in the same process.
```
interface LockInterface
final class Lock implements LockInterface
vs
final class Lock
```
I choose to use a Interface even if there is only one implementaiton to offer an extension point here
# TODO
## In this PR
* [x] tests
* [x] add logs
* [x] offer several redis connectors
* [x] try other store implementation to validate the architecture/interface
## In other PR
* documentation
* add configuration in framework bundle
* add stop watch in the debug bar
* improve the combined store (takes the drift into account and elapsed time between each store)
* implement other stores (memcache, ...)
* use this component in session manipulation (fixes#4976)
Commits
-------
018e0fc330 [Lock] Create a lock component
This PR was merged into the 3.3-dev branch.
Discussion
----------
[FrameworkBundle] Add project directory default for installing assets
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #20337
| License | MIT
| Doc PR |
This allows the `assets:install` console command to have a fallback default of the project root directory.
Current behavior is to install assets only in the `target` of the current working directory.
Commits
-------
7a11f3ecf3 [FrameworkBundle] Add project directory default for installing assets
This PR was squashed before being merged into the 3.3-dev branch (closes#20365).
Discussion
----------
[TwigBridge] Handle form label attributes like others
| Q | A |
| --- | --- |
| Branch? | master
| Bug fix? | no
| New feature? | yes
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes
| Fixed tickets | -
| License | MIT |
| Doc PR | -
The HTML for rendering attributes is duplicated in multiple blocks, making it error prone/hard to maintain.
Next, the label attributes followed a different approach. Imo. all should follow the same base rendering, showing the above is actually an issue.
Commits
-------
e317e0aeab [TwigBridge] Handle form label attributes like others
This PR was merged into the 3.3-dev branch.
Discussion
----------
[FrameworkBundle][Translator] Make the Translator works with any PSR-11 container
| 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
Uses a service-locator for collected translation loaders and replace the single call of `getParameter()` by an optional constructor argument.
Commits
-------
85177a649e [FrameworkBundle] Make Translator works with any PSR-11 container
