Fix class constants as type in sql queries output

[mikeSimonson]

No description provided.

[lcobucci]

Isn't this already fixed in master?

[lcobucci]

(handled by #608)

[stof]

it would be great to rebase the branch to clean the diff here

[lcobucci]

What do you think about this?

[lcobucci]

I think that we can simplify some things, what do you think about this:

private function formatParamsForOutput(array $params, array $types): string
{
    if (empty($params)) {
        return '';
    }

    $out = [];

    foreach ($params as $key => $value) {
        $formatted = $this->formatParameter($value, $types[$key] ?? 'string');

        $out[] = is_string($key) ? sprintf(':%s => %s', $key, $formatted) : $formatted;
    }

    return sprintf('with parameters (%s)', implode(', ', $out));
}

private function formatParameter($value, string $type) : string
{
    if (Type::hasType($type)) {
        return Type::getType($type)->convertToDatabaseValue(
            $value,
            $this->connection->getDatabasePlatform()
        );
    }

    // This is quite important since `$value` and `$type` are things provided
    // by the user, so we can't really ensure that `$value` will always be an array.
    if (is_array($value)) { 
        return implode(', ', array_map([$this, 'parameterToString'], $value));
    }

    return $this->parameterToString($value);
}

private function parameterToString($value) : string
{
    if (is_int($value) || is_string($value)) {
        return (string) $value;
    }

    if (is_bool($value)) {
        return $value === true ? 'true' : 'false';
    }

    return '?';
}

[mikeSimonson]

I will take it

[lcobucci]

We could also make parameterToString() recursive, if needed... like:

private function formatParameter($value, string $type) : string
{
    if (Type::hasType($type)) {
        return Type::getType($type)->convertToDatabaseValue(
            $value,
            $this->connection->getDatabasePlatform()
        );
    }

    return $this->parameterToString($value);
}

private function parameterToString($value) : string
{
    if (is_array($value)) { 
        return implode(', ', array_map([$this, 'parameterToString'], $value));
    }

    if (is_int($value) || is_string($value)) {
        return (string) $value;
    }

    if (is_bool($value)) {
        return $value === true ? 'true' : 'false';
    }

    return '?';
}

[stof]

Can you show what the new output looks like ?

[mikeSimonson]

@stof I did not run that on an actual migrations but it basically turns some

INSERT INTO test VALUES (?) with parameters (?)

into

INSERT INTO test VALUES (?) with parameters (1, 2, 3)

[stof]

Looks fine to me

[lcobucci]

@mikeSimonson I think that imploding the array might be confusing when you have more parameters, like:

INSERT INTO test VALUES (?, ?) with parameters (1, 2, 3, 4)

Which values are related to the first parameter and which are to the second? Maybe this helps:

- INSERT INTO test VALUES (?, ?) with parameters (1, 2, 3, 4)
+ INSERT INTO test VALUES (?, ?) with parameters ([1, 2], [3, 4])

[mikeSimonson]

I will try to add a test for this one too.

[lcobucci]

As @stof mentioned, could you rebase the branch to sync it with master?

[lcobucci]

Couldn't we solve this when generating the value (in line 458) instead of re-iterating $out? Like:

- $outval = $this->formatParameter($value, $type);
+ $outval = '[' . $this->formatParameter($value, $type) . ']';

Or only when dealing with arrays, in line 497:

- return implode(', ', array_map([$this, 'parameterToString'], $value));
+ return '[' . implode(', ', array_map([$this, 'parameterToString'], $value)) . ']';

The output would be a bit more understandable.