Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Routing] Disallow object usage inside Route#27727
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Routing] Disallow object usage inside Route#27727
Uh oh!
There was an error while loading.Please reload this page.
Conversation
faaa501 to59f5f54CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For 4.1 as bug fix? the issue also exists on 2.8, but there we don't have an "export" method as we rely only onvar_export().
| @@ -746,6 +746,10 @@ public static function export($value): string | |||
| return 'null'; | |||
| } | |||
| if (!\is_array($value)) { | |||
| if (\is_object($value)) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if (\is_object($value) && !method_exists($value, '__set_state')) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
even objects with__set_state are broken, as the logic currently replaces new lines by a concatenation with"\n" instead (because when var_export-ing a scalar, new lines appear only inside strings).
If we want to keep support for objects with__set_state, it should be handled as a separate case (and I would put it before theif (!is_array()) case then, rather than nesting in it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
even objects with __set_state are broken
indeed, then let's keep it unsupported :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The purpose of this PR is to disallow object usage, and avoid to export objects (viavar_export). Thus, the fact that__set_state exists or not is not relevant. The dumped file does not work when we export an object (because of\n replacements).
Did not see the comments above :)
| @@ -746,6 +746,10 @@ public static function export($value): string | |||
| return 'null'; | |||
| } | |||
| if (!\is_array($value)) { | |||
| if (\is_object($value)) { | |||
| throw new \InvalidArgumentException('Symfony\Component\Routing\Route cannot contain objects'); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing dot at end of message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed and squashed.
59f5f54 tof1b3ffcCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(for 4.1)
…Bonfante)This PR was submitted for the master branch but it was squashed and merged into the 4.1 branch instead (closessymfony#27754).Discussion----------[HttpFoundation] missing namespace for RedisProxyI think the intention was to check if is instance of \Symfony\Component\Cache\Traits\RedisProxy| Q | A| ------------- | ---| Branch? | 4.1 (becareful when merging)| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |<!--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.-->Commits-------8e0acfc [HttpFoundation] missing namespace for RedisProxy
…regex (nicolas-grekas)This PR was merged into the 4.1 branch.Discussion----------[Routing] fix too much greediness in host-matching regex| Q | A| ------------- | ---| Branch? | 4.1| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony#27721| License | MIT| Doc PR | -Commits-------e16b302 [Routing] fix too much greediness in host-matching regex
f1b3ffc to426fb45CompareThank you@paxal. |
This PR was submitted for the master branch but it was merged into the 4.1 branch instead (closes#27727).Discussion----------[Routing] Disallow object usage inside Route| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | ?| Fixed tickets |#27723| License | MIT| Doc PR | ✘As discussed in#27723 the `Route` object should not support nested objects as attributes (`requirements`, `defaults`, ...).Thus, if detected, an `\InvalidArgumentException` will be thrown.Willfix#27723Commits-------426fb45 [Routing] Disallow object usage inside Route
As discussed in#27723 the
Routeobject should not support nested objects as attributes (requirements,defaults, ...).Thus, if detected, an
\InvalidArgumentExceptionwill be thrown.Willfix#27723