Skip empty expression parts

[infabo]

Bug Report

Q	A
BC Break	yes
Version	2.11.0

Summary

The commit 1d50325 introduced a breaking change.

CompsiteExpression->with() directly adds parts, whereas the old CompositeExpression->addMultiple() utilized CompositeExpression->add() which had an empty-check.

• has a guard
  see https://github.com/doctrine/dbal/blob/2.11.x/lib/Doctrine/DBAL/Query/Expression/CompositeExpression.php#L100

• has no guard
  see https://github.com/doctrine/dbal/blob/2.11.x/lib/Doctrine/DBAL/Query/Expression/CompositeExpression.php#L126

Current behaviour

parts with empty-string result in AND () which is an invalid where-expression.

How to reproduce

• Instantiate QueryBuilder instance.
• provide an empty string to andWhere

$queryBuilder->andWhere('', "foo = 'bar'")->getSQL();

Expected behaviour

Do not add empty where conditions in CompositeExpression->with() as CompositeExpression->add() did.

[greg0ire]

@BenMorel can you please have a look?

[BenMorel]

I’m not sure to see the value in providing this check; when is it useful IRL?

There are many invalid WHERE expressions, an empty string is just one of them; and it’s not the responsibility of this library to validate them. If anything, I’d rather throw an exception rather than silently discarding the value.

But if you really ask me, I would keep the current behaviour and document the BC break. Unless there’s a good case for this, obviously!

[morozov]

@infabo could you provide a code example that did work and no longer works? As far as I understand your case, you're explicitly switching from add() to with(). Thereby you're opting in to using the new API which deliberately doesn't have the empty check.

I see, it's the addWhere() method that now calls with() internally.

@BenMorel while I agree with your point, it is a BC break which shouldn't be introduced in 2.x. We can add a temporary private method like withIgnoringEmpty() and use it in all methods that currently call with(). This will be reverted in 3.0.x.

[BenMorel]

Ah, sorry, I realize now that this has been included in 2.x as well. I only had 3.0 in mind.
I'll fix this in a PR!

[infabo]

Thank you!

[liayn]

Could it be that this still fails?

$queryBuilder->andWhere('')->getSQL();

This works in 2.10

[BenMorel]

@liayn This should work in 2.11.1. Which version are you using?

[discordier]

If I'm not mistaken, the fix (and tests) only cover cases where there is at least one non empty argument.

In his case, there is only one empty condition and no non empty prior call to ->where().

The unshift here: https://github.com/doctrine/dbal/pull/4286/files#diff-305699f4a9180da213eb29c5671ab925R839 produces [''] and hence things break again, as we yet again produce an empty WHERE part.

Edit: shouldn't we return out early if $args is empty after filtering?

[BenMorel]

Unless I missed something, the fix should cover all cases. Could you please provide a failing test case? Not even as a PR, you can just copy/paste the code here.

If there's a regression, I'll fix it.

[discordier]

Reproducer:

diff --git a/tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php b/tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php
index 038097c89..d70127625 100644
--- a/tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php
+++ b/tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php
@@ -950,6 +950,18 @@ class QueryBuilderTest extends DbalTestCase
         $qb->getSQL();
     }
 
+    public function testAndWhereEmptyString(): void
+    {
+        $qb = new QueryBuilder($this->conn);
+
+        $qb->select('id')
+            ->from('foo');
+
+        $qb->andWhere('');
+
+        self::assertSame('SELECT id FROM foo', $qb->getSQL());
+    }
+
     public function testAndWhereEmptyStringStartingWithEmptyExpression(): void
     {
         $qb = new QueryBuilder($this->conn);
@@ -975,6 +987,18 @@ class QueryBuilderTest extends DbalTestCase
         self::assertSame('SELECT id FROM foo WHERE (a = b) AND (c = d)', $qb->getSQL());
     }
 
+    public function testOrWhereEmptyString(): void
+    {
+        $qb = new QueryBuilder($this->conn);
+
+        $qb->select('id')
+            ->from('foo');
+
+        $qb->orWhere('');
+
+        self::assertSame('SELECT id FROM foo', $qb->getSQL());
+    }
+
     public function testOrWhereEmptyStringStartingWithEmptyExpression(): void
     {
         $qb = new QueryBuilder($this->conn);

[BenMorel]

Thanks for the test case, @discordier! I can confirm that it fails indeed.

But I'm afraid it also fails on 2.10.x, so this is not a regression:

git checkout -b 2.10.4 tags/2.10.4
composer install
(... apply patch manually)
vendor/bin/phpunit

There were 2 failures:

1. Doctrine\Tests\DBAL\Query\QueryBuilderTest::testAndWhereEmptyString
   Failed asserting that two strings are identical.
   --- Expected
   +++ Actual
   @@ @@
   -'SELECT id FROM foo'
   +'SELECT id FROM foo WHERE ()'

2. Doctrine\Tests\DBAL\Query\QueryBuilderTest::testOrWhereEmptyString
   Failed asserting that two strings are identical.
   --- Expected
   +++ Actual
   @@ @@
   -'SELECT id FROM foo'
   +'SELECT id FROM foo WHERE ()'

Do we want to fix this for 2.10.x, @morozov?

[liayn]

https://github.com/georgringer/news/blob/e16807909889336b3c510f4e30ec630492fdcfe3/Classes/Backend/RecordList/RecordListConstraint.php#L251
worked without issues with 2.10.x (call always had '' here since the parameter was never true-ish)

[discordier]

I have not tested against 2.10.x, I was simply providing a test case for #4282 (comment). I did not check against the prior behavior. Sorry for the noise.

I'll have a look at #4282 (comment) and try to come up with a more specific one.

[liayn]

The difference is that we have a where() call in the linked code whereas your tests do not.

[BenMorel]

So is there still a regression now, @liayn?

[discordier]

He is right:

diff --git a/tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php b/tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php
index 038097c89..383c069d5 100644
--- a/tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php
+++ b/tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php
@@ -950,6 +950,19 @@ class QueryBuilderTest extends DbalTestCase
         $qb->getSQL();
     }
 
+    public function testWhereExpressionAndWhereEmptyString(): void
+    {
+        $qb = new QueryBuilder($this->conn);
+
+        $qb->select('id')
+            ->from('foo')
+            ->where('a = b');
+
+        $qb->andWhere('');
+
+        self::assertSame('SELECT id FROM foo WHERE a = b', $qb->getSQL());
+    }
+
     public function testAndWhereEmptyStringStartingWithEmptyExpression(): void
     {
         $qb = new QueryBuilder($this->conn);
@@ -975,6 +988,19 @@ class QueryBuilderTest extends DbalTestCase
         self::assertSame('SELECT id FROM foo WHERE (a = b) AND (c = d)', $qb->getSQL());
     }
 
+    public function testWhereExpressionOrWhereEmptyString(): void
+    {
+        $qb = new QueryBuilder($this->conn);
+
+        $qb->select('id')
+            ->from('foo')
+            ->where('a = b');
+
+        $qb->orWhere('');
+
+        self::assertSame('SELECT id FROM foo WHERE a = b', $qb->getSQL());
+    }
+
     public function testOrWhereEmptyStringStartingWithEmptyExpression(): void
     {
         $qb = new QueryBuilder($this->conn);

dbal 2.11.x:

There was 1 error:

1) Doctrine\Tests\DBAL\Query\QueryBuilderTest::testWhereExpressionAndWhereEmptyString
ArgumentCountError: Too few arguments to function Doctrine\DBAL\Query\Expression\CompositeExpression::with(), 0 passed in /home/xtra/development/github/doctrine/dbal/lib/Doctrine/DBAL/Query/QueryBuilder.php on line 837 and exactly 1 expected

/home/xtra/development/github/doctrine/dbal/lib/Doctrine/DBAL/Query/Expression/CompositeExpression.php:119
/home/xtra/development/github/doctrine/dbal/lib/Doctrine/DBAL/Query/QueryBuilder.php:837
/home/xtra/development/github/doctrine/dbal/tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php:961

ERRORS!
Tests: 5744, Assertions: 9882, Errors: 1, Skipped: 697, Incomplete: 11.

dbal 2.10.x:

OK, but incomplete, skipped, or risky tests!
Tests: 5633, Assertions: 9780, Skipped: 684, Incomplete: 11.

Interestingly, only the andWhere breaks here.

[BenMorel]

@discordier Thank you. orWhere() does fail, if the previous non-empty call is orWhere() as well, I modified your test accordingly.

Fixed in #4330.

[maddy2101]

any hints on when you will release a bugfix version? Trying to decide whether we can wait for it or need to pin the version to 2.10.x. It would be really nice to get a date or maybe a timeframe (tomorrow, in 6 weeks, in less than 6 months, ... you get the idea?)

Thank you very much for all the work done.

[morozov]

@maddy2101 I plan to release it this week. See the associated milestone.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.