Fixes fatal error if query params is a data dumper object instead of array

[ssaki]

refs #757

[Majkl578]

What is Data? There is no import for this class and Doctrine\Bundle\DoctrineBundle\Controller\Data doesn't exist either - probably missing import?

[ssaki]

I've missed to add the import. It's done now

[piotrantosik]

👍

[Ocramius]

This patch fixes a symptom, not the root cause. Replacing a value with a Symfony\Component\VarDumper\Cloner\Data is the root cause of the issue. This patch is therefore invalid, and the root cause is to be fixed instead.

[florentdestremau]

@Ocramius meanwhile, this is still broken. Where should we look for the root cause ?

[bartko-s]

+1

[Ocramius]

If you can reproduce it, send a test case. This simply needs someone with time and a debugger, looking fir the location where the Data instance is replacing the actual parameters.

[florentdestremau]

I have it in my project, I see there is a conversion supposed to be done in the doctrine-bundle/Twig/DoctrineExtension.php::replaceQueryParameters. I'm running my debugger, will keep you updated

[florentdestremau]

Ok replaceQueryParameters is NOT called, probably nothing to do with it.
What I see is that the query is loaded from the profile with $params as a Data instance. As far as I go, right from the unserialization the query parameters are in a Data instance. So it looks like the profiler stores them like this in the first place

[ssaki]

@florentdestremau When I was looking around at this I somehow got the impression that this is intentional, probably to provide the pretty collapsible dump of query's parameters. That's why I initially opted for this fix (BC-safe, no side effects). Needless to say this impression is not based on evidence so it may be completely wrong :) When you look at the issue have this in mind and check if the fix won't break the dump as well.

[florentdestremau]

Well I am now wondering if the good design is to actually use Data or not. If the implementation was made but not applied in this case then we would need something like the replaceQueryParameters function I mentioned above. If the array is the way to go now to store the query parameters then the profiler is wrong and the fix should be made at the creation of the profile. Any thoughts @Ocramius ?

[florentdestremau]

Ok I found the place where query params are converted to Data. This happens at the end of the collect cycle for the DoctrineDataCollector

doctrine/doctrine-bundle/DataCollector/DoctrineDataCollector.php:144

public function collect(Request $request, Response $response, \Exception $exception = null)
    (...)
    // HttpKernel < 3.2 compatibility layer
    if (method_exists($this, 'cloneVar')) {
        // Might be good idea to replicate this block in doctrine bridge so we can drop this from here after some time.
        // This code is compatible with such change, because cloneVar is supposed to check if input is already cloned.
        foreach ($this->data['queries'] as &$queries) {
            foreach ($queries as &$query) {
                $query['params'] = $this->cloneVar($query['params']);
            }
        }
    }

The cloneVar method returns a Data instance. Now the question is: who is right between the collector or the profiler controller ?

• Should the $query[params] be stored as Data and read as such ? In this case this PR is relevant
• If not, then either the (method_exists($this, 'cloneVar') test is not sufficient or simply this whole bunch of code has nothing to do here.

Waiting for your thoughts @Ocramius and I'm ready to make a PR

[stof]

Wrapping collected data into a VarDumper Data is the expected behavior in the profiler collector though (it avoids issue with non-serializable data and so on)

[florentdestremau]

In this case then we are missing a "convert to array" and this PR is relevant, although it might be done straight in the controller rather than in last-minute before the executeQuery call

[florentdestremau]

https://github.com/doctrine/DoctrineBundle/compare/master...florentdestremau:convert-query-params-to-array-in-explain?expand=1

I can make this PR if needed, what do you think @stof @Ocramius ?

[Ocramius]

Looks like a lot of wrong stuff is happening in there anyway:

• DoctrineBundle/Controller/ProfilerController.php

  Line 51 in 9e79906

  $query = $queries[$connectionName][$query];

  is already wrong (why are we overwriting an int with a different value?)

• DoctrineBundle/Controller/ProfilerController.php

  Line 52 in 9e79906

  if (! $query['explainable']) {

  assumes that the value is an array without checking it at all (should add an assertion)
• I can't find a getQueries() definition in this repo. That's the actual API that is either to be fixed or documented for BC breaks.

[florentdestremau]

@Ocramius the getQueries method comes from the Symfony\Bridge\Doctrine\DataCollector\DoctrineDataCollector.

I am willing to make the changes if needed, I just need the list, so far:

• changing the first $query parameter to something more explicit like $queryId
• add tests like isset($query['explainable']

The getQueries come with a Data structure, again, if this is supposed to be a Data element I don't see why wouldn't we simply convert to array if needed ? There is obviously a BC break in the middle, but again I don't have the official design pattern in mind.

[Ocramius]

I don't see why wouldn't we simply convert to array if needed ?
There is obviously a BC break in the middle

Need to figure out when the BC break was introduced first, so we can decide if it needs reverting or adapting.

[stof]

Well, storing the collected data in a Data object was an intended change in Symfony 3. And some parts of the bundle have already been updated to follow the latest profiler enhancements. It is just that the controller explaining queries was forgotten.

[florentdestremau]

@Ocramius @stof I would like to help here as I'm directly concerned (and use my own clone in local now) but I don't know what to do or check for this issue to go forward (or #757 for the reference).

If Data object is now the norm, then this PR (or mine suggested above with the enhancements invoked) is BC compatible and in the right direction.

One might consider moving this conversation back to the original issue rather than in a PR ?

[ArrowStrike]

I have the same problem in our project when we try attempting to EXPLAIN a query from Web Profiler(#707). And @ssaki fix it perfectly. It needs to be in the production :)