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

Extend symfony DataCollector, do not serialize created_at as \DateTime #562

Merged
merged 7 commits into from
Jul 27, 2019
Merged

Extend symfony DataCollector, do not serialize created_at as \DateTime #562

merged 7 commits into from
Jul 27, 2019

Conversation

toooni
Copy link
Contributor

@toooni toooni commented Mar 19, 2019

Errors with deserialization for the profiling of blocks in debug mode with PHP 7.3

Instead of implementing an own solution for serialize/unserialize which resolves the PHP 7.3 bug https://bugs.php.net/bug.php?id=77302, I modified the BlockDataCollector to extend Symfony\Component\HttpKernel\DataCollector\DateCollector. This way we take advantage of the already solved serialize/unserialize issue there.

I am targeting this branch, because this affects all users with PHP 7.3. This fix keeps BC even for already serialized data.

Closes #556

Changelog

### Changed
- Fix serializing issue for `BlockDataCollector`

@phansys phansys added the bug label Jun 29, 2019
@phansys
Copy link
Member

phansys commented Jun 29, 2019

Is there a way to add tests for this fix in order to avoid regressions?

@core23
Copy link
Member

core23 commented Jul 11, 2019

Ping @toooni

@toooni
Copy link
Contributor Author

toooni commented Jul 12, 2019

@core23 I've added an object in a test trace which should make a problem if we wouldn't use the AbstractDataCollector and PHP7.3. I don't think this doesn't help to prevent anything since - but I wouldn't know what else to do ;)

src/Templating/Helper/BlockHelper.php Outdated Show resolved Hide resolved
@core23 core23 added patch and removed Needs: Tests labels Jul 12, 2019
@greg0ire greg0ire requested a review from phansys July 13, 2019 10:15
@phansys
Copy link
Member

phansys commented Jul 13, 2019

@toooni, could you please explain how to reproduce the issue this PR is fixing?
I'll try to check it locally.

@core23
Copy link
Member

core23 commented Jul 18, 2019

Ping @toooni

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@toooni
Copy link
Contributor Author

toooni commented Jul 26, 2019

@phansys Here's a link to the issue (which is not associated with me) #556
Honestly - I can't reproduce it now. I don't know why and don't have the time right now to dig into it from the beginning (I've reproduced this in March). But relying on the Abstract DataCollector from Symfony reduces complexity (-39 lines of code) in this project and is still a good idea to avoid such issues in the future.

@phansys
Copy link
Member

phansys commented Jul 26, 2019

I agree @toooni, thank you.
Could you please squash your commits?

@toooni
Copy link
Contributor Author

toooni commented Jul 27, 2019

@phansys Can't you just choose "Squash and merge" in the merge drop down menu? Else I can do this next week.

@greg0ire greg0ire merged commit f3d84a5 into sonata-project:3.x Jul 27, 2019
@greg0ire
Copy link
Contributor

Thanks @toooni

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.

None yet

7 participants