Skip to content

Only pass the additional arguments if they are not null#3984

Closed
duncan3dc wants to merge 1 commit intodoctrine:2.10.xfrom
duncan3dc:fix-pdo-fetchall-parameters
Closed

Only pass the additional arguments if they are not null#3984
duncan3dc wants to merge 1 commit intodoctrine:2.10.xfrom
duncan3dc:fix-pdo-fetchall-parameters

Conversation

@duncan3dc
Copy link
Contributor

Q A
Type bug
BC Break no
Fixed issues #3975

Summary

Following on from #3893 this PR ensures the additional arguments are only passed when they are non-null, as issues were reported in #3975

@greg0ire greg0ire added the Bug label May 3, 2020
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Can you please add some tests?

@duncan3dc
Copy link
Contributor Author

Any advice on how you'd like that done?

It looks like you're using PHPUnit for mocking here, which doesn't support verifying that no argument was passed, it always substitutes in the default value anyway. So mocking doesn't appear to be an option.

It looks like you set up some databases, would you prefer an integration test with one of these database servers? If so, which one and where should the test case live?

@greg0ire
Copy link
Member

greg0ire commented Jun 4, 2020

Since the issue arises in the form of an SQL error, an integration test makes more sense. You can run it against all DBs, and it should live under tests/Doctrine/Tests/DBAL/Functional I think. I wrote one for the first time a few weeks ago, maybe it will help: #3994

On a somewhat related note, I see that kind of question coming back again and again: #4012 (comment)

We should really work on providing that kind of guide 😅

@duncan3dc
Copy link
Contributor Author

Closing as @BenMorel is tackling the tricky tests in #4173

@duncan3dc duncan3dc closed this Sep 2, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 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.

3 participants