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 explaining queries #802

Merged
merged 1 commit into from Apr 20, 2018
Merged

Fix explaining queries #802

merged 1 commit into from Apr 20, 2018

Conversation

stof
Copy link
Member

@stof stof commented Apr 19, 2018

Recent Symfony versions are wrapping the params in a VarDumper Data when collecting the profiler info. This ensures that they get unwrapped properly when trying to explain the query.

Fixes #757

Recent Symfony versions are wrapping the params in a VarDumper Data when
collecting the profiler info. This ensures that they get unwrapped
properly when trying to explain the query.
@stof
Copy link
Member Author

stof commented Apr 19, 2018

Recent means at least 3.4+. I don't remember about older 3.x versions (and I don't have any project using these older unmaintained 3.x versions to test)


$params = $query['params'];

if ($params instanceof Data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create a function with an explicit name and call it in both place ?

@kimhemsoe kimhemsoe added the Bug label Apr 19, 2018
@kimhemsoe kimhemsoe added this to the 1.9.1 milestone Apr 19, 2018
@kimhemsoe
Copy link
Member

@stof Is it possible to add a test for it?

@stof
Copy link
Member Author

stof commented Apr 19, 2018

The proper way to test this in a way making the test strong a regarding changes in the bridge collector would involve building a fake app running some DB query, and having a functional test running a request on this fake app, then opening the profiler, and then trying to explain the query in it (by loading the URL of the partial). This would involve a bunch of work, and I won't have time to work on this during the next few days (and I cannot guarantee it for the next few weeks, even though the French holidays coming in May may give me time for it, but in conflict with other OSS stuff). Is it required to build such test covering the profiler explaining feature for the bug fix to be merged ?

@kimhemsoe
Copy link
Member

I will merge in now then. I very much would a test for this if you or someone else find the time to todo it :)

@kimhemsoe kimhemsoe merged commit fbb8a34 into doctrine:master Apr 20, 2018
@kimhemsoe
Copy link
Member

it is a long running bug, so lets fix it for now and then we can clean up the mess later...

@stof Thanks

@hvt
Copy link

hvt commented Jul 31, 2018

Would be great if a release could be made that includes this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants