Skip to content

[Routing] Generate URLs in compliance with PHP_QUERY_RFC3986 #19637

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3,324 commits into from

Conversation

jameshalsall
Copy link
Contributor

@jameshalsall jameshalsall commented Aug 16, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no?
Deprecations? no
Tests pass? yes
Fixed tickets #19625
License MIT
Doc PR reference to the documentation PR, if any

fabpot and others added 30 commits June 29, 2016 07:48
…tyle in LintCommand (chalasr)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Yaml] Avoid using both Input/Output and SymfonyStyle in LintCommand

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

Fixed some inconsistencies/mistakes from the original YamLintCommand.

Commits
-------

dd84b7f [Yaml] Avoid using both Input/Output and SymfonyStyle in LintCommand
…te shared services. (hhamon)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[DependencyInjection] deprecate access to private shared services.

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | symfony#19117
| License       | MIT
| Doc PR        | ~

Commits
-------

4ed70c4 [DependencyInjection] deprecate access to private shared services. Fixes issue symfony#19117.
…ctors (Ener-Getick)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Serializer] Allow to use easily static constructors

| Q             | A
| ------------- | ---
| Branch?       | "master"
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#19027 (comment)
| License       | MIT
| Doc PR        | -

This PR allows to simply use static constructors to instantiate objects with the serializer by extending the default normalizers.

Commits
-------

9be6484 [Serializer] Allow to use easily static constructors
This PR was merged into the 2.8 branch.

Discussion
----------

fixed test

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Commits
-------

bac531c fixed test
* 2.7:
  [HttpFoundation] Warning when request has both Forwarded and X-Forwarded-For
  [Console] Decouple SymfonyStyle from TableCell
* 2.8:
  [HttpFoundation] Warning when request has both Forwarded and X-Forwarded-For
  fixed test
  [Console] Decouple SymfonyStyle from TableCell
* 3.0:
  [HttpFoundation] Warning when request has both Forwarded and X-Forwarded-For
  fixed test
  [Console] Decouple SymfonyStyle from TableCell
* 3.1:
  [HttpFoundation] Warning when request has both Forwarded and X-Forwarded-For
  fixed test
  [Console] Decouple SymfonyStyle from TableCell
* 2.7:
  [HttpKernel] Inline ValidateRequestListener logic into HttpKernel
  fixed HttpKernel dependencies after symfony#18688

Conflicts:
	src/Symfony/Component/HttpKernel/composer.json
This PR was merged into the 2.8 branch.

Discussion
----------

[FrameworkBundle] Fix fixtures

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#19173 symfony#19223
| License       | MIT
| Doc PR        | -

Commits
-------

d175477 [FrameworkBundle] Fix fixtures
* 2.8:
  [FrameworkBundle] Fix fixtures
  [HttpKernel] Inline ValidateRequestListener logic into HttpKernel
  fixed HttpKernel dependencies after symfony#18688

Conflicts:
	src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/builder_1_services.txt
	src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/definition_1.txt
	src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/definition_2.txt
	src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/event_dispatcher_1_events.txt
	src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/legacy_synchronized_service_definition_1.txt
	src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/legacy_synchronized_service_definition_2.txt
	src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/parameter.txt
	src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/route_collection_1.txt
	src/Symfony/Bundle/FrameworkBundle/composer.json
	src/Symfony/Component/HttpKernel/composer.json
* 3.0:
  [FrameworkBundle] Fix fixtures
  [HttpKernel] Inline ValidateRequestListener logic into HttpKernel
  fixed HttpKernel dependencies after symfony#18688

Conflicts:
	src/Symfony/Component/HttpKernel/HttpKernel.php
	src/Symfony/Component/HttpKernel/Tests/HttpKernelTest.php
* 2.7:
  [ci] Upgrade phpunit wrapper deps
* 2.8:
  [ci] Upgrade phpunit wrapper deps
* 3.0:
  [ci] Upgrade phpunit wrapper deps
…y removing deprecated factory (HeahDude)

This PR was merged into the 3.1 branch.

Discussion
----------

[DoctrineBridge] fixed DoctrineChoiceLoaderTest by removing deprecated factory

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

Commits
-------

3f86eae [DoctrineBridge] fixed DoctrineChoiceLoaderTest by removing deprecated factory
…HeahDude)

This PR was merged into the 3.0 branch.

Discussion
----------

[Form] fixed ChoiceTypeTest after symfony#17822

| Q             | A
| ------------- | ---
| Branch?       | 3.0
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

Commits
-------

777c193 [Form] fixed ChoiceTypeTest after symfony#17822
* 3.0:
  [Form] fixed ChoiceTypeTest after symfony#17822
* 3.1:
  [Form] fixed ChoiceTypeTest after symfony#17822
  [DoctrineBridge] fixed DoctrineChoiceLoaderTest by removing deprecated factory
  [ci] Upgrade phpunit wrapper deps
  [FrameworkBundle] Fix fixtures
  [HttpKernel] Inline ValidateRequestListener logic into HttpKernel
  fixed HttpKernel dependencies after symfony#18688
…iler (javiereguiluz)

This PR was squashed before being merged into the 3.1 branch (closes symfony#18934).

Discussion
----------

Fixed some issues of the AccessDecisionManager profiler

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#19022 https://github.com/symfony/symfony-standard/issues/968 schmittjoh/JMSSecurityExtraBundle#207
| License       | MIT
| Doc PR        | -

Commits
-------

082f1b5 Fixed some issues of the AccessDecisionManager profiler
This PR was merged into the 3.2-dev branch.

Discussion
----------

Fix merge issues

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Commits
-------

7701fea Fix merge issues
This PR was merged into the 3.2-dev branch.

Discussion
----------

[Console] fix input stream related tests

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Commits
-------

1cca740 [Console] fix input stream related tests
nicolas-grekas and others added 18 commits August 7, 2016 17:41
…places TagAwareRedisAdapter) (nicolas-grekas)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Cache] Add generic TagAwareAdapter wrapper (replaces TagAwareRedisAdapter)

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | symfony/symfony-docs#6858

This PR replaces TagAwareRedisAdapter introduced in symfony#19047 by a generic `TagAwareAdapter` that works with any two adapters.

Commits
-------

288308b [Cache] Add generic TagAwareAdapter wrapper (replaces TagAwareRedisAdapter)
…eniedException (Nicofuma)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Security] Expose the required roles in AccessDeniedException

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

Nowadays it is more and more common to protect some sensitive actions and part of a website using 2FA or some re-authentication mechanism (per example, on Github you have to enter your password again when you add an ssh key). But currently, in Symfony, it is really hard to implement without having to duplicate the logic, provide an explicit list of URLs to protect or hack into the security component.

A good way to achieve that would be to add a special role (like IS_AUTHENTICATED_FULLY) and use it in the access map. But it requires us to be able to have a custom logic in an ExceptionListener depending on the roles behind an AccessDeniedException.

With this patch we could write an ExceptionListener of this kind (a similar logic could also be used in an AccessDeniedHandler):

```php
    public function onKernelException(GetResponseForExceptionEvent $event)
    {
        $exception = $event->getException();
        do {
            if ($exception instanceof AccessDeniedException) {
                foreach ($exception->getAttributes() as $role) {
                    if ($role === 'IS_AUTHENTICATED_2FA' && !$this->accessDecisionManager->decide($this->tokenStorage->getToken(), $role, $exception->getObject())) {
                        // Start 2FA
                    }
                }
            }
        } while (null !== $exception = $exception->getPrevious());
    }
```

Replaces symfony#18661

Commits
-------

6618c18 [Security] Expose the required roles in AccessDeniedException
… exceptions on duplicates (Alex Pott)

This PR was squashed before being merged into the 3.2-dev branch (closes symfony#19529).

Discussion
----------

Add Yaml::PARSE_EXCEPTION_ON_DUPLICATE to throw exceptions on duplicates

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#19526
| License       | MIT
| Doc PR        |

Commits
-------

cb362f2 Add Yaml::PARSE_EXCEPTION_ON_DUPLICATE to throw exceptions on duplicates
…leofeyer)

This PR was squashed before being merged into the 3.2-dev branch (closes symfony#19511).

Discussion
----------

Use a dedicated exception in the file locator

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

This PR adds a dedicated `FileLocatorFileNotFoundException` class to the file locator, so it is possible to catch file locator exceptions separately from invalid argument exceptions.

```php
try {
    foreach ($container->get('file_locator')->locate('file.php', null, false) as $file) {
        include $file;
    }
} catch (\InvalidArgumentException $e) {
    // this will catch both file locator exceptions as well as
    // any invalid argument exception thrown in an included file
}
```

With the dedicated exceptions, we could do this:

```php
try {
    foreach ($container->get('file_locator')->locate('file.php', null, false) as $file) {
        include $file;
    }
} catch (FileLocatorFileNotFoundException $e) {
    // this will only ignore file locator exceptions
}
```

Commits
-------

ee4adaa Use a dedicated exception in the file locator
* 2.7:
  [travis] fix after box updates
  Console: Fix indentation of Help: section of txt usage help
  [Intl] Update ICU data to 57.1
  [Config] Improved test
  Added class existence check if is_subclass_of() fails in compiler passes
* 2.8:
  [travis] fix after box updates
  Console: Fix indentation of Help: section of txt usage help
  [Intl] Update ICU data to 57.1
  [Config] Improved test
  Added class existence check if is_subclass_of() fails in compiler passes
* 3.1:
  [travis] fix after box updates
  Console: Fix indentation of Help: section of txt usage help
  [Intl] Update ICU data to 57.1
  [Config] Improved test
  Added class existence check if is_subclass_of() fails in compiler passes
* 2.7:
  [appveyor] Fix cache handling
* 2.8:
  [appveyor] Fix cache handling
* 3.1:
  [appveyor] Fix cache handling
…olm)

This PR was squashed before being merged into the 3.2-dev branch (closes symfony#19602).

Discussion
----------

[Workflow] Updated link to the documentation

| Q             | A
| ------------- | ---
| Branch?       | "master"
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

The link to the docs was broken because of the big documentation restructure a couple of weeks back.

Commits
-------

d96e861 [Workflow] Updated link to the documentation
This PR was merged into the 3.2-dev branch.

Discussion
----------

[Yaml] update changelog and upgrade files

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#19504
| License       | MIT
| Doc PR        |

Commits
-------

3cd2932 [Yaml] update changelog and upgrade files
…grekas)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Cache] Drop TaggedCacheItemInterface

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Introducing this interface was a mistake, we should drop it: the CacheItem class is final, and it can be instantiated only through a cache adapter. Which means there is no way to reuse this interface in any meaningful way as far as extensibility/interoperability is concerned.

Commits
-------

624890b [Cache] Drop TaggedCacheItemInterface
@@ -266,7 +266,13 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
$fragment = isset($extra['_fragment']) ? $extra['_fragment'] : '';
unset($extra['_fragment']);

if ($extra && $query = http_build_query($extra, '', '&')) {
if (defined('PHP_QUERY_RFC3986')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which PHP version was it added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5.4, the reason I've done it this way is to ease merging of upstream changes on 2.x branch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5.4 (the test can be removed in Symfony >= 3.0).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I asked was that you submitted the PR on master :) If this is a bug fix, this should be merged on 2.7 instead first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if this is for master, then the constant is always defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right :)

@fabpot
Copy link
Member

fabpot commented Aug 16, 2016

Isn't it a bug fix instead of a new feature?

@jameshalsall
Copy link
Contributor Author

I'm not so sure, it could have unintended side-effects depending on people's expectations of the generate() call.

@jameshalsall jameshalsall changed the base branch from master to 2.7 August 16, 2016 15:08
@jameshalsall jameshalsall changed the base branch from 2.7 to master August 16, 2016 15:09
@jameshalsall jameshalsall deleted the routing-encoding branch August 16, 2016 15:12
@jameshalsall
Copy link
Contributor Author

Closed in favour of #19639 which is rebased on 2.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.