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

add sql type parameter to connection project function #3703

Closed
wants to merge 1 commit into from
Closed

add sql type parameter to connection project function #3703

wants to merge 1 commit into from

Conversation

leonhusmann
Copy link

@leonhusmann leonhusmann commented Oct 18, 2019

Q A
Type improvement
BC Break yes
Fixed issues none

Summary

I've added the missing sql-type parameter to the project wrapper function.

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

Thank you for your PR. What use case do you try to handle with this PR? It would be nice if you can add this to your description of the PR.

lib/Doctrine/DBAL/Connection.php Show resolved Hide resolved
@leonhusmann
Copy link
Author

Thank you for your PR. What use case do you try to handle with this PR? It would be nice if you can add this to your description of the PR.

I use the types parameter to specify the types of the sql-parameters. That's needed because only then mysql can choose the right index for me. Here is a small example, using the executeQuery function.

$this->connection->executeQuery(
    <<<'SQL'
    SELECT * FROM dummy WHERE number IN (:numberArray)
    SQL,
    [
        'numberArray' => [123, 456],
    ],
    [
        'numberArray' => Connection::PARAM_INT_ARRAY
    ]
);

This is working fine. Now I want to hydrate the rows to structs after the execution. Therefore, I could use the project function which does this for me with my custom callback.
But because of the missing type parameter I then can't specify the sql-parameter as int array and the wrong index will be used.

@SenseException SenseException added this to the 3.0.0 milestone Oct 24, 2019

$conn->expects($this->once())
->method('executeQuery')
->with($statement, $params, $types)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than mocking 4 classes just in order to make sure that the argument is passed down the line, I'd like to test the real value that passing this parameter introduces for DBAL.

Could you instead come up with a realistic functional test that would show the need for this change?

@morozov
Copy link
Member

morozov commented Nov 15, 2019

Looking at this method, I have really no idea why we need it in DBAL in the first place. It contains more limitations and caveats than real use:

  1. It fetches all data in memory which is often inefficient (see Add iterators for non pdo drivers. #2718).
  2. It fetches rows one by one instead of using fetchAll().
  3. Adding $types at the end of the signature makes it look weird since $params and $types logically belong together but they end up separated by $function.
  4. It doesn't allow to specify the statement fetch mode since it's instantiated internally.
  5. It can be easily replaced with:
    foreach ($conn->executeQuery($query, $params, $types) as $value) {
       yield $function($value);
    }

Why don't we just get rid of it instead of fixing?

@morozov morozov removed this from the 4.0.0 milestone Nov 17, 2020
@morozov morozov closed this Nov 17, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants