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.

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

Fix ChannelCollector related serialization issue in Symfony profiler #10680

Merged
merged 1 commit into from Oct 4, 2019
Merged

Fix ChannelCollector related serialization issue in Symfony profiler #10680

merged 1 commit into from Oct 4, 2019

Conversation

ostrolucky
Copy link
Contributor

Q A
Branch? 1.4
Bug fix? yes
New feature? no
BC breaks? unlikely, but possible
Deprecations? no
Related tickets fixes #10223
License MIT

This is very annoying long standing issue.

I'm pretty sure this will fix the issue (it does in our case). As a side effect, this is more efficient, as there is less data needed to be serialized.

This issue is related to circular references inside Channel model. In our case we can reproduce with

$n = new Channel();
$locale = new Locale();

$n->addLocale($locale);
$n->setDefaultLocale($locale);

$this->data = [
    'channel' => null,
    'channels' => [$n],
    'channel_change_support' => $channelChangeSupport,
];

inside ChannelCollector

I'm aware changing return type is technically BC break, but it's unlikely somebody uses this class. If this is an issue, please advise how to continue.

@ostrolucky ostrolucky requested a review from a team as a code owner September 18, 2019 19:06
@Zales0123 Zales0123 added the Potential Bug Potential bugs or bugfixes, that needs to be reproduced. label Sep 19, 2019
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

Hey Gabriel!

Thanks a lot for your contribution!

@pamil are we ready to make a BC Break here? This class could be consider as internal, but we didn't mention it anywhere.

Copy link
Contributor

@pamil pamil left a comment

Choose a reason for hiding this comment

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

I think we can proceed with this BC break, it's our internal code and only runs in development environments.

@pamil pamil merged commit f8ac6c9 into Sylius:1.4 Oct 4, 2019
@pamil
Copy link
Contributor

pamil commented Oct 4, 2019

Thanks, Gabriel! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Potential Bug Potential bugs or bugfixes, that needs to be reproduced.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants