Handle Empty Array for IN and NOT IN queries. #8035

VincentLanglet · 2020-02-21T21:20:22Z

beberlei · 2020-02-22T22:36:04Z

lib/Doctrine/ORM/Query/Expr/Func.php

@@ -73,6 +75,10 @@ public function getArguments()
     */
    public function __toString()
    {
+        if (count($this->arguments) === 0) {
+            return $this->name . '(NULL)';


wouldn't this return all elements where the value is null for nullable columns? This seems wrong.

@beberlei No,

Select foo from bar where foo IN (NULL)

Return always 0 row. I tried it multiple times.

foo IN (A, B) mean foo = A or foo = B
foo IN (NULL) mean foo = NULL
foo = NULL always returns no result, instead of foo IS NULL.

And as explained here: #8033 (comment), this is already the query created when using

$query->andWhere("$alias.foo IN (:bar)"); $query->setParameter('bar', []);

AFAIK NULL is handled that way in all commonly known databases like e.g. PostgreSQL, MySQL, SQLite...

https://dbfiddle.uk/?rdbms=sqlite_3.8&fiddle=ac349cc3ca597516e307d690e7f19d90

I saw this nice comic on Twitter about this the other day :)

use x IS NULL, not x = NULL

— 🔎Julia Evans🔍 (@b0rk) October 17, 2019

SenseException · 2020-02-24T11:04:30Z

tests/Doctrine/Tests/ORM/Query/ExprTest.php

@@ -299,7 +299,7 @@ public function testNotInLiteralExpr()

    public function testNotInExprForEmptyArray()
    {
-        self::assertEquals('u.id NOT IN(FALSE)', (string) $this->_expr->notIn('u.id', []));
+        self::assertEquals('u.id NOT IN(NULL)', (string) $this->_expr->notIn('u.id', []));


u.id NOT IN(NULL) would result in an empty result too.

https://dbfiddle.uk/?rdbms=sqlite_3.8&fiddle=0097a9450b4e56c142e64a480b745614

this contradicts with your expectations described in your issue #8033 (comment)

Yeah, I know.

It's still an improvement since

IN (NULL) works correclty

NOT IN (NULL) wont return an error, when NOT IN () does.

And in the next comment #8033 (comment), I explained that

$query->andWhere("foo.bar NOT IN (:baz)")->setParameter('baz', []);

Is actually translated as foo.bar NOT IN (NULL).

So my PR make the behaviour of $query->expr()->notIn() more consistent with $query->andWhere("foo.bar NOT IN (:baz)")->setParameter('baz', []);.

We can improve the behaviour of the NOT IN query with an empty array, but It'll need a lot a more code changes and some discussions, so I recommend to split this from this PR, which was only looking for more consistency.

If you want to split it, then you'll have to remove the NOT IN changes. What do you want to change for supporting this case?

If you want to split it, then you'll have to remove the NOT IN changes.

I disagree. Actually if you write

$query->andWhere($queryBuilder->expr()->notIn('foo.bar', ':baz')) $query->setParameter('baz', []);

There is no exception and this is translated as foo.bar NOT IN (NULL).

But if you write directly

$query->andWhere($queryBuilder->expr()->notIn('contractOperation.type', []))

You get an exception. But there is absolutely no reason to behave differently from the first case.

So it's still an improvement to have a consistent behaviour for notIn('foo.bar', ':baz')) and notIn('foo.bar', [])).

I improve both the IN case and the NOT IN case.
So I don't think I should remove the NOT IN changes.

I don't know what I have to change to completely supports the NOT IN case and if it's possible. It should handle correctly the case

$query->andWhere('foo.bar NOT IN :baz') $query->setParameter('baz', []);

$query->andWhere($queryBuilder->expr()->notIn('foo.bar', ':baz')) $query->setParameter('baz', []);

$query->andWhere($queryBuilder->expr()->notIn('foo.bar', []))

And this seems for me like a lot of work/rework.

Is this PR perfect and fixing everything ? No
Is this PR better than the actual situation ? Yes

VincentLanglet · 2020-03-08T20:41:14Z

@beberlei @SenseException Is this PR ok for you ? :)

greg0ire · 2020-03-20T17:44:14Z

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

git rebase -i origin/2.7, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
A window will show up with many lines, replace pick with fixup on every line but the first one
Close your editor, git should do its magic, and you should end up with one commit
Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

VincentLanglet · 2020-03-20T18:08:48Z

Done @greg0ire :)

kimhemsoe · 2020-03-21T00:43:48Z

Is this a good idea?

It set off my footgun sense and
personal i would rather just have my app blow up into my face and force me to take of the case then orm guess what i want.

VincentLanglet · 2020-03-21T03:43:38Z

Is this a good idea?

It set off my footgun sense and
personal i would rather just have my app blow up into my face and force me to take of the case then orm guess what i want.

I'm just calling for consistency.

With this PR
$query->andWhere($queryBuilder->expr()->in('contractOperation.type', [])) will have the same behaviour than $query->andWhere($queryBuilder->expr()->in('contractOperation.type', ':baz'))->setParameter('baz', []); or $query->andWhere("foo.bar IN (:baz)")->setParameter('baz', []); and returning nothing.

There is no reason for having the first syntax crashing when the others don't.

I prefer to have my app blowing up for a real good reason, and this is not a valid one.

VincentLanglet · 2020-03-29T23:08:12Z

Hi @beberlei @SenseException @greg0ire ;

What is needed to make a decision about this PR ? :)

I had in mind to add consistency, in order to have the same behaviour for

$query->andWhere($queryBuilder->expr()->in('contractOperation.type', ':baz'))->setParameter('baz', []);

which was working, and

$query->andWhere($queryBuilder->expr()->in('contractOperation.type', []))

which was previously returning an exception.

You can prefer the opposite behaviour (returning an exception) but either way I think there is something to do about this non-consistency.

VincentLanglet · 2020-04-08T22:29:06Z

Hi @SenseException.
You review position is still "Requesting change".
What is needed to make a decision about this PR ? :)

My main goal was to add consistency, in order to have the same behaviour for

$query->andWhere($queryBuilder->expr()->in('contractOperation.type', ':baz'))->setParameter('baz', []); // No exception
$query->andWhere($queryBuilder->expr()->notIn('contractOperation.type', ':baz'))->setParameter('baz', []); // No exception

and

$query->andWhere($queryBuilder->expr()->in('contractOperation.type', [])) // Throw exception
$query->andWhere($queryBuilder->expr()->notIn('contractOperation.type', [])) // Throw exception

SenseException · 2020-04-09T19:43:55Z

I'm not satisfied with leaving u.id NOT IN(NULL) just because of consistency as mentioned above. But that's just my opinion and not a blocker if others decide to merge this PR.

VincentLanglet · 2020-04-09T21:46:50Z

I'm not satisfied with leaving u.id NOT IN(NULL).

I understand.
I still believe it's an improvement to not throw an exception which lead to an error 500 in production. Then I can still work on this NOT IN (NULL) issue in another PR.

What would you expect as query ?

But that's just my opinion and not a blocker if others decide to merge this PR.

Ok. I thought the change requested status was blocking the merge.

@greg0ire Since you approved the PR, would you agree to merge it ?

greg0ire · 2020-04-11T06:12:51Z

lib/Doctrine/ORM/Query/Expr/Func.php

@@ -73,6 +75,10 @@ public function getArguments()
     */
    public function __toString()
    {
+        if (count($this->arguments) === 0) {
+            return $this->name . '(NULL)';


Wait so IN is considered a function 🤔 🤔 🤔 ? I mean it has a left operand…

Yes, that's what I discovered 😅

This makes it a bit scary… touching such a generic method looks like it could have unforeseen repercussions.

greg0ire · 2020-04-11T06:23:35Z

I gave it more thought, and I can't see why people would want to use in or notIn with an empty array… using NULL as a solution will yield no results, which means what is very probably a programming error will go hidden. If we have to be consistent, I'd say we should probably go with the exception, but that's a BC-break, so you will have to trigger a deprecation instead.

VincentLanglet · 2020-04-11T08:27:40Z

I gave it more thought, and I can't see why people would want to use in or notIn with an empty array… using NULL as a solution will yield no results, which means what is very probably a programming error will go hidden.

The real use case I met, was using a variable, a const, a property, a select list of choice from the user, ...

$query->andWhere($queryBuilder->expr()->in('contractOperation.type', ':baz'))->setParameter('baz', $baz]);

Is really more convenient than

if (!empty($baz)) {
    $query->andWhere($queryBuilder->expr()->in('contractOperation.type', ':baz'))->setParameter('baz', $baz]);
} else {
    $query->andWhere('1 = 0');
}

Plus, it needs less SQL-specific knowledge.

When I searched for other ORM behaviour with empty array, it seems that they're generally trying to handle the query. For example: https://www.bennadel.com/blog/3288-its-safe-to-use-empty-arrays-with-the-in-operator-in-sequelize.htm

greg0ire · 2020-04-11T08:31:18Z

I felt it would be something like that, what I don't understand (or rather, remember, I probably encountered this myself in the past) is how and why do you let the user pick an empty baz in the first place?

VincentLanglet · 2020-04-11T08:45:56Z

The user-choice was not my best example indeed. But I have others.

Sometimes, we didn't succeed making only one request and then we did something like.

$ids = $queryOne->select(id)->....->getQuery()->getResult();

$query->andWhere($queryBuilder->expr()->in('foo.id', ':ids'))->setParameter('ids', $ids]);

Maybe the request was possible using only one query and we're not good enough with sql/doctrine, but this is still a possible use case.

Sometimes, the array list is coming from the user property

$query->andWhere($queryBuilder->expr()->in('foo.allowedCategory', ':userCategory'))->setParameter('userCategory', $this->getUser()->getCategory()]);

And it was possible for us to have no category for a user.

You might say that I can do:

if (empty($baz)) {
    return [];
}

$query->andWhere($queryBuilder->expr()->in('contractOperation.type', ':baz'))->setParameter('baz', $baz]);

But that's not always possible.

Some function asked to return the non-executed query.
The filters of sonata or the createQuery/configureQuery methods are two examples you know ;)

greg0ire · 2020-04-11T09:06:44Z

Some function asked to return the non-executed query.

Ah ok, that's why… IMO in that case we should not even attempt to execute a query since we know it should always return an empty result set. We should definitely throw, that should push the user to do more controls before attempting to create the query. And explanation like "This user has no categories" is way better than an empty list that will not explain why nothing was found, isn't it?

VincentLanglet · 2020-04-11T09:41:27Z

Some function asked to return the non-executed query.

Ah ok, that's why… IMO in that case we should not even attempt to execute a query since we know it should always return an empty result set. We should definitely throw, that should push the user to do more controls before attempting to create the query. And explanation like "This user has no categories" is way better than an empty list that will not explain why nothing was found, isn't it?

I don't understand how an exception would help for the case ?

public function configureQuery($query) {
    $query->andWhere('foo.bar IN (:baz)')->setParameter('baz', $baz]);

    return $query;
}

If an exception is thrown, I won't be able to catch it...

And if I user some filters on my list which lead me to a IN () for any reason, I just expect no result. And it avoid the developper to handle every possible empty case if he doesn't consider this like necessary.

I met few hundred of developers, none of them was checking for an empty array before doing

$query->andWhere($queryBuilder->expr()->in('contractOperation.type', ':baz'))->setParameter('baz', $baz]);

This is not necessarily the best developers we can found and they doesn't talk for every other developers, but it still showed me that checkin the empty array case is not natural/obvious/necessary/... for everyone.

greg0ire · 2020-04-11T10:13:50Z

If an exception is thrown, I won't be able to catch it...

IMO there should be a method that the developer can implement to decide whether to build the query at all, before even calling configureQuery. The Doctrine exception should be caught and used as the $previous argument of a Sonata exception telling the developer they need to implement that method.

And if I user some filters on my list which lead me to a IN () for any reason, I just expect no result.

But you can always know there will be no result in such cases, without running SQL, right? Whether to display "no result" or an explanation might depend on the context, and could be configurable, but in any case, there is no need to ask your RDBMS to execute that statement.

And it avoid the developper to handle every possible empty case if he doesn't consider this like necessary.

I think this is where we disagree, but I guess we could leave the possibility to implement a createQuery that would return some kind of NullQuery (from the null object pattern when getting that kind of exception. Then Sonata could check if the query is instance of this type and chose not to pass it to Doctrine and return an empty result immediately if that's the case.

This is not necessarily the best developers we can found and they doesn't talk for every other developers, but it still showed me that checkin the empty array case is not natural/obvious/necessary/... for everyone.

With phpstan and psalm coming to maturity in the last years, there is a healthy trend of more input checking, and eliminating subtle bugs such as those that can arise when you support weird edge cases. This is a nice talk about what can happen when you try to support all the edge cases: https://www.destroyallsoftware.com/talks/wat

VincentLanglet · 2020-04-11T10:42:11Z

@greg0ire If some rule should be added to decide if yes or no the query should be executed, it would be more useful to do it in doctrine, this way the developer won't have to make every time the check (this could lead more easily to duplicate code, errors, difficulty to debug...).
When I use an ORM, I expect to be able to do it with as few knowledge as possible about how a database works. If I wanted to write SQL query and handle everything, I could do it by my own.

I think you're taking a too much elaborate way of thinking the solution. There is a lot of beginners/junior developper using this library.

From my experience I only saw three things:

Developper who wanted to avoid the exception and wrote

if (!empty($baz)) {
    $query->andWhere($queryBuilder->expr()->in('contractOperation.type', $bar));
} else {
    $query->andWhere('1 = 0');
}

Developper who used setParameter, and luckily avoid the exception

$query->andWhere($queryBuilder->expr()->in('contractOperation.type', ':baz'))->setParameter('baz', $baz]);

Developper who didn't try the empty array case and got an exception in production

$query->andWhere($queryBuilder->expr()->in('contractOperation.type', $bar))

I never saw a developper who wanted/tried/asked to handle the empty array case the way you describe.

The fact that empty array are supported when using setParameter was voluntary introduced, cf : doctrine/dbal#264. It's allowed from 2013, this is an old habits and nobody seems to have complained about it since.

greg0ire · 2020-04-11T11:31:32Z

If some rule should be added to decide if yes or no the query should be executed, it would be more useful to do it in doctrine, this way the developer won't have to make every time the check (this could lead more easily to duplicate code, errors, difficulty to debug...).
When I use an ORM, I expect to be able to do it with as few knowledge as possible about how a database works.

When you use an ORM you don't need to know about SQL at all to understand that asking for a contract operation which type is in [] is absolute nonsense. Sometimes you don't care, some other times though, it's a programming mistake, and you want to know. You don't want to debug a huge DQL statement to find where the mistake lies.

Besides isn't there a bug in the UI/the js lib that posts the form with the filters, if we allow it to send []? You keep talking about beginners and junior devs, wouldn't a clear exception be a good way to help them think about the edge cases they overlooked? Even better, if they did not mean to send an empty array at all, but still do because they are junior devs, hiding the issue and returning zero results might be very confusing.

I never saw a developper who wanted/tried/asked to handle the empty array case the way you describe.

Why would you? I mean, sending an empty array inside IN() is probably not something that happens that often, is it?

To sum up my thoughts, doing so happens if:

you made a programming mistake, in which case you want to know
you did not think at all about the edge case, in which case it's better to fail as early and as noisily as possible, and let you handle the edge case however you want to. In Sonata, one could imagine to have that result on returning an empty list, or another, nicer behavior like explaining to the user what is wrong with their query.

VincentLanglet · 2020-04-11T12:00:14Z

When you use an ORM you don't need to know about SQL at all to understand that asking for a contract operation which type is in [] is absolute nonsense.

That's where I disagree with you.
Since

select id from asv_client where id in (select id from asv_client where 1 = 0);

works.

I expect the same If I compute by myself what's inside the in condition.

Sometimes we didn't find other way than doing things like

if (ChoiceType::TYPE_NOT_CONTAINS !== $value['type']) {
    $matchingClientsIds = $this->clientRepository->getClientIdsByContractTypeName($selectedContractTypeNames[0]);

    foreach ($allContractTypeNames as $contractTypeName) {
        if (in_array($contractTypeName, $selectedContractTypeNames)) {
            $clientIds = $this->clientRepository->getClientIdsByContractTypeName($contractTypeName);
            $matchingClientsIds = array_intersect($matchingClientsIds, $clientIds);
        } elseif (ChoiceType::TYPE_EQUAL === $value['type']) {
            $clientIds = $this->clientRepository->getClientIdsByContractTypeName($contractTypeName);
            $excludedClientIds = array_intersect($matchingClientsIds, $clientIds);
            $matchingClientsIds = array_diff($matchingClientsIds, $excludedClientIds);
        }
    }
} else {
    $matchingClientsIds = $this->clientRepository->getAllClientIds();
    foreach ($allContractTypeNames as $contractTypeName) {
        if (in_array($contractTypeName, $selectedContractTypeNames)) {
            $clientIds = $this->clientRepository->getClientIdsByContractTypeName($contractTypeName);
            $excludedClientIds = array_intersect($matchingClientsIds, $clientIds);
            $matchingClientsIds = array_diff($matchingClientsIds, $excludedClientIds);
        }
    }
}

$matchingClientsIds = array_unique($matchingClientsIds);

$queryBuilder->andWhere("o.id IN (:foo)");
$queryBuilder->setParameter('foo', $matchingClientsIds);

You may say that performance are bad, I agree.
You may say that it could be done in SQL, I'm just not good enough, certainly.
But it's still making sens to me that this should be a legit query.

And I have no reason to have a different UI if the pre-computed result is empty. The user doesn't have to know if I compute in sql or in php.

Besides isn't there a bug in the UI/the js lib that posts the form with the filters, if we allow it to send []?

I gave you other example. You may compute the $array before using it in the query.

You keep talking about beginners and junior devs, wouldn't a clear exception be a good way to help them think about the edge cases they overlooked?

Not when the exception can occur in production and have huge impacts.

In twig, foo.fieldWhoDoesntExist throw an exception in dev environment, but not in production. If an exception should be thrown for an empty array, I would expect at least the same behaviour. But I don't even consider this is always a programming mistake.

I never saw a developper who wanted/tried/asked to handle the empty array case the way you describe.

Why would you? I mean, sending an empty array inside IN() is probably not something that happens that often, is it?

I met this case dozens of times.

~~Actually~~ Currently, the situation is the worst since there is different behaviour. My solution has the benefit to be BC and in the continuation of what was made before doctrine/dbal#264.

greg0ire · 2020-04-11T12:18:41Z

Maybe you're right, I don't know. What I'm sure about is that I'm no longer approving of this PR. I won't block it, but I will let people that deal with that kind of situation more often than I do take care of it.

I changed my mind: the implications of touching Func.php are not 100% clear to me, and I feel that we should tell our users that using IN with an empty array is not going to work instead of trying to make it work with NULL. I might be wrong, because IIRC Doctrine sometimes uses IN itself with identifiers obtained from somewhere else, but I can't remember.

VincentLanglet · 2020-04-11T12:32:14Z

I won't block it, but I will let people that deal with that kind of situation more often than I do take care of it.

I feel really bad about this @greg0ire. You're already the second one taking this position.
And since you're all seems to not care about this situation, this will lead to no change.

This was approved here : doctrine/dbal#264
This was approved also here : doctrine/DoctrineBundle#1144

Ignoring this PR will still let a big inconsistency in the behaviour of the query builder for empty array... Having something who sometimes works sometimes not, is the worst situation.

If you don't want to handle empty array, I strongly invite you to create a PR to revert both doctrine/dbal#264 and doctrine/DoctrineBundle#1144, and throw the exception you want instead.

This non-consistent behaviour should not exist.

greg0ire · 2020-04-11T12:53:34Z

doctrine/DoctrineBundle#1144 should be reverted after the exception I think might be good is thrown (meaning the bundle requires a version that has the exception). Otherwise, it will lead the profiler to show wrong information.
Regarding doctrine/dbal#264, I disapprove of it, but it was merged by people that are wiser than I. They merged it 7 years ago though, and might not merge it today, I don't know. I don't know, and this is why I would like other contributors to review this and give their point of view. It's not that I don't care, it's that I'm afraid of making the wrong decision here. This is a widely used package.

VincentLanglet · 2020-04-11T13:37:15Z

I don't know, and this is why I would like other contributors to review this and give their point of view. It's not that I don't care, it's that I'm afraid of making the wrong decision here. This is a widely used package.

I understand, and be happy to have other reviews.
My main concern is to not let the actual situation be forgotten.
And since this PR is already more than one month old, I was afraid it'll be.

Thanks for the request-review you made.

VincentLanglet · 2020-05-03T20:52:04Z

I don't know, and this is why I would like other contributors to review this and give their point of view. It's not that I don't care, it's that I'm afraid of making the wrong decision here. This is a widely used package.

Hi @greg0ire. It seems that this problem does not interest a lot of people.
Should I just wait more or can I do something to help ?

greg0ire · 2020-05-03T21:06:27Z

I'd wait until the end of the lockdown. Some of us have families to take care of, and can't spend much time on open source during that time.

VincentLanglet · 2020-05-03T21:16:23Z

I'd wait until the end of the lockdown. Some of us have families to take care of, and can't spend much time on open source during that time.

My bad. I never saw it this way since, because of the lockdown, I can't be with my family and don't have much to do except open source 😅

Let's wait then !

VincentLanglet · 2020-06-04T22:37:10Z

Hi @greg0ire, friendly reminder. :)
It's been one month since the last message. We wait again ?

greg0ire · 2020-06-05T17:14:43Z

I think you should start with a rebase, looks like your build does not have runs for Github Actions that are now mandatory.

VincentLanglet · 2020-06-05T18:17:57Z

Done :)

greg0ire · 2020-06-05T18:36:58Z

@doctrine/team-orm please review

VincentLanglet · 2020-07-12T10:35:42Z

Hi, @greg0ire.

Can we do something about this PR ? :)

greg0ire · 2020-07-21T18:26:38Z

I don't know. @SenseException can you maybe answer #8035 (comment) ?

SenseException · 2020-07-21T21:40:00Z

It's still like I wrote in #8035 (comment). A lot of thoughts and time were put into this PR, which I appreciate a lot, but I cannot become comfortable with the NOT IN(NULL) part.

@greg0ire mentioned other arguments (especially #8035 (comment)) that makes me keep my position on this. In my DX, I don't want software to run code and have an unexpected result that needs to be debugged and costs developer time. SQL Queries with invalid arguments or with an expected empty result shouldn't be forwarded to the database anyway. In the end, you would still need an empty array check in an application if the empty result isn't the expected behaviour for a developer. Not having to run SQL queries has a positive effect on performance (depending on the projects) and the exception makes developers reconsider how they write project-code.

I still would like to see more reviewers for this PR though.

VincentLanglet · 2020-07-21T21:58:26Z

It's still like I wrote in #8035 (comment). A lot of thoughts and time were put into this PR, which I appreciate a lot, but I cannot become comfortable with the NOT IN(NULL) part.

@greg0ire mentioned other arguments (especially #8035 (comment)) that makes me keep my position on this. In my DX, I don't want software to run code and have an unexpected result that needs to be debugged and costs developer time. SQL Queries with invalid arguments or with an expected empty result shouldn't be forwarded to the database anyway. In the end, you would still need an empty array check in an application if the empty result isn't the expected behaviour for a developer. Not having to run SQL queries has a positive effect on performance (depending on the projects) and the exception makes developers reconsider how they write project-code.

I agree with your arguments. Here, more than everything, I look for consistency.
And adding an exception is a BC-break, removing an exception is not one.

Currently, we have the following inconsistency

$query->andWhere($queryBuilder->expr()->in('foo.type', ':baz'))->setParameter('baz', []); // No exception
$query->andWhere($queryBuilder->expr()->notIn('foo.type', ':baz'))->setParameter('baz', []); // No exception
$query->andWhere($queryBuilder->expr()->in('contractOperation.type', [])) // Throw exception
$query->andWhere($queryBuilder->expr()->notIn('contractOperation.type', [])) // Throw exception

What about changing this to

Not throwing an exception in doctrine:2.7.
Throwing an exception for all of them in next major, doctrine 3.

This way we'll have consistency in both version of doctrine, and it'll go in your way if you prefer an exception.

VincentLanglet · 2020-08-22T11:04:19Z

Currently, we have the following inconsistency
$query->andWhere($queryBuilder->expr()->in('foo.type', ':baz'))->setParameter('baz', []); // No exception
$query->andWhere($queryBuilder->expr()->notIn('foo.type', ':baz'))->setParameter('baz', []); // No exception
$query->andWhere($queryBuilder->expr()->in('contractOperation.type', [])) // Throw exception
$query->andWhere($queryBuilder->expr()->notIn('contractOperation.type', [])) // Throw exception
What about changing this to

Not throwing an exception in doctrine:2.7.

Throwing an exception for all of them in next major, doctrine 3.

This way we'll have consistency in both version of doctrine, and it'll go in your way if you prefer an exception.

WDYT about this strategy @greg0ire @SenseException ?

SenseException · 2020-08-31T19:55:42Z

Throwing an exception for all of them in next major, doctrine 3.

This matches with what I would expect for the next major release.

Not throwing an exception in doctrine:2.7

Either way, not throwing or throwing for all, wouldn't this be a BC break? Maybe that's what @greg0ire was referring to in #8035 (comment). This PR might be one that should target 3.0.

VincentLanglet changed the base branch from master to 2.7 February 21, 2020 23:34

beberlei reviewed Feb 22, 2020

View reviewed changes

SenseException requested changes Feb 24, 2020

View reviewed changes

VincentLanglet requested review from SenseException and beberlei February 24, 2020 11:41

VincentLanglet requested a review from greg0ire March 20, 2020 00:42

greg0ire previously approved these changes Mar 20, 2020

View reviewed changes

VincentLanglet dismissed greg0ire’s stale review via 800425c March 20, 2020 18:08

greg0ire previously approved these changes Mar 20, 2020

View reviewed changes

SenseException mentioned this pull request Apr 5, 2020

Fixed white gap on right side of page doctrine/doctrine-website#343

Closed

greg0ire reviewed Apr 11, 2020

View reviewed changes

greg0ire requested a review from a team April 11, 2020 12:53

Handle Empty Array

d9c546d

VincentLanglet closed this Aug 31, 2020

Handle Empty Array for IN and NOT IN queries. #8035

Handle Empty Array for IN and NOT IN queries. #8035

Conversation

VincentLanglet commented Feb 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Feb 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentLanglet commented Mar 8, 2020

greg0ire commented Mar 20, 2020

VincentLanglet commented Mar 20, 2020

kimhemsoe commented Mar 21, 2020

VincentLanglet commented Mar 21, 2020 • edited

VincentLanglet commented Mar 29, 2020

VincentLanglet commented Apr 8, 2020

SenseException commented Apr 9, 2020

VincentLanglet commented Apr 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Apr 11, 2020

VincentLanglet commented Apr 11, 2020

greg0ire commented Apr 11, 2020

VincentLanglet commented Apr 11, 2020

greg0ire commented Apr 11, 2020 • edited

VincentLanglet commented Apr 11, 2020

greg0ire commented Apr 11, 2020 • edited

VincentLanglet commented Apr 11, 2020

greg0ire commented Apr 11, 2020

VincentLanglet commented Apr 11, 2020 • edited

greg0ire commented Apr 11, 2020

VincentLanglet commented Apr 11, 2020

greg0ire commented Apr 11, 2020

VincentLanglet commented Apr 11, 2020 • edited

VincentLanglet commented May 3, 2020

greg0ire commented May 3, 2020

VincentLanglet commented May 3, 2020

VincentLanglet commented Jun 4, 2020

greg0ire commented Jun 5, 2020

VincentLanglet commented Jun 5, 2020

greg0ire commented Jun 5, 2020

VincentLanglet commented Jul 12, 2020

greg0ire commented Jul 21, 2020

SenseException commented Jul 21, 2020

VincentLanglet commented Jul 21, 2020

VincentLanglet commented Aug 22, 2020

SenseException commented Aug 31, 2020

greg0ire Feb 29, 2020 •

edited

VincentLanglet commented Mar 21, 2020 •

edited

VincentLanglet commented Apr 9, 2020 •

edited

greg0ire commented Apr 11, 2020 •

edited

greg0ire commented Apr 11, 2020 •

edited

VincentLanglet commented Apr 11, 2020 •

edited

VincentLanglet commented Apr 11, 2020 •

edited