8000 [Serializer] Improve CircularReference detection message by ScullWM · Pull Request #23322 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Serializer] Improve CircularReference detection message #23322

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

Merged
merged 1 commit into from
Jul 3, 2017

Conversation

ScullWM
Copy link
Contributor
@ScullWM ScullWM commented Jun 28, 2017
Q A
Branch? 2.7
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Hi,
I've get a CircularReferenceException error while serializing object with nested levels. Detect a CircularReference is great, but having more information about the object that generate this exception is better.

I simply suggest to add a get_class of the current object in the error message.

Before:
A circular reference has been detected (configured limit: 1).

After (edit)
A circular reference has been detected when serializing the object of class "App\Domain\User" (configured limit: 1).

@ScullWM ScullWM changed the title [Serializer] Improve CircularReference detection [Serializer] Improve CircularReference detection message Jun 28, 2017
@ScullWM ScullWM changed the base branch from master to 3.4 June 28, 2017 15:48
@ScullWM ScullWM changed the base branch from 3.4 to master June 28, 2017 15:48
@@ -195,7 +195,7 @@ protected function handleCircularReference($object)
return call_user_func($this->circularReferenceHandler, $object);
}

throw new CircularReferenceException(sprintf('A circular reference has been detected (configured limit: %d).', $this->circularReferenceLimit));
throw new CircularReferenceException(sprintf('A circular reference has been detected (configured limit: %d) on %s.', $this->circularReferenceLimit, get_class($object)));
Copy link
Member
@dunglas dunglas Jun 28, 2017

Choose a reason for hiding this comment

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

May I suggest A circular reference has been detected when serializing the object of class "App\Domain\User" identified by "00000000462ff471000000005e39f75b" (configured limit: 1).?

(Use spl_object_hash to get the id).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 👍

@ScullWM ScullWM force-pushed the circular_reference_msg branch from fe4161a to dab69c3 Compare June 29, 2017 06:27
@@ -195,7 +195,7 @@ protected function handleCircularReference($object)
return call_user_func($this->circularReferenceHandler, $object);
}

throw new CircularReferenceException(sprintf('A circular reference has been detected (configured limit: %d).', $this->circularReferenceLimit));
throw new CircularReferenceException(sprintf('A circular reference has been detected when serializing the object of class "%s" identified by "%s" (configured limit: %d)', get_class($object), spl_object_hash($object), $this->circularReferenceLimit));
Copy link
Member

Choose a reason for hiding this comment

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

does the hash provide any value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may help if we catch this exception and show spl_hash of each normalized objects to compare. Honestly I'm not convinced too. I will remove it.

@nicolas-grekas
Copy link
Member

We could merge this as minor on the lowest branch where it applies, no need to consider it a new feature IMHO.

@ScullWM ScullWM force-pushed the circular_reference_msg branch from dab69c3 to 3a529e3 Compare July 3, 2017 13:10
@ScullWM ScullWM changed the base branch from master to 2.7 July 3, 2017 13:11
@ScullWM
Copy link
Contributor Author
ScullWM commented Jul 3, 2017

Updated to lowest branch possible (2.7)

@fabpot
Copy link
Member
fabpot commented Jul 3, 2017

Thank you @ScullWM.

@fabpot fabpot merged commit 3a529e3 into symfony:2.7 Jul 3, 2017
fabpot added a commit that referenced this pull request Jul 3, 2017
… (ScullWM)

This PR was merged into the 2.7 branch.

Discussion
----------

[Serializer] Improve CircularReference detection message

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

Hi,
I've get a CircularReferenceException error while serializing object with nested levels. Detect a CircularReference is great, but having more information about the object that generate this exception is better.

I simply suggest to add a `get_class` of the current object in the error message.

Before:
`A circular reference has been detected (configured limit: 1).`

After (edit)
`A circular reference has been detected when serializing the object of class "App\Domain\User" (configured limit: 1).`

Commits
-------

3a529e3 Improve CircularReferenceException message
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.

6 participants
0