Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Routing] #23109

Closed
Gribnif opened this issue Jun 8, 2017 · 5 comments
Closed

[Routing] #23109

Gribnif opened this issue Jun 8, 2017 · 5 comments
Labels
BC Break Bug Routing Status: Needs Review
Milestone
3.3

Comments

@Gribnif
Copy link

Gribnif commented Jun 8, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no
Symfony version >=3.3

#1c5a24 introduced a strict check on unserialize() within Route.php, which is good for security. However, it causes a failure if the compiled route's class is anything other than CompiledRoute. This is the case in Drupal, where the serialized object of a route requires \Drupal\Core\Routing\CompiledRoute.

Might I suggest adding the ability to set the list of allowed classes somehow?
@xabbuh xabbuh added the Routing label Jun 8, 2017
@Tobion
Copy link
Contributor

Tobion commented Jun 9, 2017

#21090 should be reverted for the Route class as it breaks BC. Being able to change the compilation by overwriting the compiler class is a supported use-case. But the new check prevents that.

@fabpot
Copy link
Member

fabpot commented Jun 9, 2017

@Gribnif Can you submit a pull request?

@stof
Copy link
Member

stof commented Jun 9, 2017

There is a second place where it was added but should not. I commented on the PR

@xabbuh xabbuh added this to the 3.3 milestone Jun 9, 2017
Gribnif pushed a commit to Gribnif/symfony that referenced this issue Jun 9, 2017
Revert the change in b420181 with respect to Routing/Route.php becaus…
399ec44 
…e it breaks BC. Refers to symfony#21090 and symfony#23109
@Gribnif Gribnif mentioned this issue Jun 9, 2017
Closed
@Gribnif
Copy link
Author

Gribnif commented Jun 9, 2017

I have created a PR for the Routing portion of the issue. I feel that the FormError.php portion should be done by someone else in a separate PR, since reverting it can have different effects.

@stof
Copy link
Member

stof commented Jun 13, 2017

which different effect ? Removing the allowed_class will give us back the 3.2 behavior: allowing any object there, which is something we need

fabpot added a commit that referenced this issue Jun 13, 2017
@fabpot
bug #23121 [Routing] Revert the change in [#b42018] with respect to R…
9649b9e 
…outing/Route.php (Dan Wilga)

This PR was submitted for the master branch but it was merged into the 3.3 branch instead (closes #23121).

Discussion
----------

[Routing] Revert the change in [#b42018] with respect to Routing/Route.php

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

...because it breaks BC with third-party code which, for instance, might use a subclass of CompiledRoute within the options portion of the Route. Refers to #21090 and #23109

Commits
-------

f09893b [Routing] Revert the change in [#b42018] with respect to Routing/Route.php
@fabpot fabpot closed this as completed Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BC Break Bug Routing Status: Needs Review
Projects
None yet
Milestone
3.3
Development

No branches or pull requests
6 participants
@fabpot @stof @Tobion @xabbuh @Gribnif @carsonbot