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

DDC-2748 DQL expression "in" not working with Collection #822

Closed
wants to merge 2 commits into from

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Oct 18, 2013

Fix + tests for DDC-2748

DQL expression "in" not working with Collection
Using $qb->expr()->in() with a Collection, will result in an empty array sent.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2750

We use Jira to track the state of pull requests and the versions they got
included in.

@@ -442,6 +444,9 @@ public function sqrt($x)
*/
public function in($x, $y)
{
if ($y instanceof Collection) {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if somebody implements the collection interface on an entity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I agree on your remark, I don't see why today in() accepts objects or entities... IMO, in() means "in an array" (or Collection of course) period.

Take any array function ever in the whole history (up to the dinosaurs), it is normal behavior to accept only arrays as input and not have a weird "fallback" mechanism when the input is "invalid".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I've never heard of a dinosaur calling a function expecting an array and giving it an object, yet I'm not an archaeologist)

Copy link
Member

Choose a reason for hiding this comment

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

Fair point :-)

@stof
Copy link
Member

stof commented Oct 18, 2013

In any case, you should not pass a collection to IN because it will put the values directly in the DQL, thus allowing DQL injection (and having a different DQL for each combination of values, thus bypassing the query cache). You should be using a DQL parameter for the collection

@mnapoli
Copy link
Contributor Author

mnapoli commented Oct 18, 2013

@stof Right, I don't know why I thought the expression builder would automatically add the values as parameters to the query :/

This PR is wrong, I'm closing it

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

Successfully merging this pull request may close these issues.

4 participants