8000 [Bug][DataCollector] Fix the regression caused by the removal of double-stringification by francoispluchino · Pull Request #10361 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Bug][DataCollector] Fix the regression caused by the removal of double-stringification #10361

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 2 commits into from
Closed

[Bug][DataCollector] Fix the regression caused by the removal of double-stringification #10361

wants to merge 2 commits into from

Conversation

francoispluchino
Copy link
Contributor
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ~
License MIT
Doc PR ~

With the commit dce66c9, an exception is thrown when an Object is part of the list of request parameters.

For example, the ParamFetcher object of the FosRestBundle included the Request object, and this object, included the Session object. The serialization fails when we use the PDO handler session.

@@ -52,6 +52,9 @@ public function collect(Request $request, Response $response, \Exception $except
foreach ($request->attributes->all() as $key => $value) {
if ('_route' === $key && is_object($value)) {
$value = $value->getPath();

Copy link
Contributor

Choose a reason for hiding this comment

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

remove line break, also would be a good idea to add a regression test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line break before elseif is not in convention of code?

Copy link
Contributor

Choose a reason for hiding this comment

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

@francoispluchino it is not.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for adding a regression test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I do this.

@fabpot
Copy link
Member
fabpot commented Mar 3, 2014

That's not enough. Actually, we need to serialize everything here (fixed in #10367).

@fabpot fabpot closed this Mar 3, 2014
@francoispluchino francoispluchino deleted the fix-stringification-collector branch March 3, 2014 15:31
fabpot added a commit that referenced this pull request Mar 3, 2014
…ctor (fabpot)

This PR was merged into the 2.5-dev branch.

Discussion
----------

[HttpKernel] fixed serialization of the request data collector

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

This fixes a regression introduced in #10352.

Commits
-------

6102f99 [HttpKernel] fixed serialization of the request data collector
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.

4 participants
0