Bug: [QueryBuilder] `where()` with `BETWEEN` creates bad SQL even with `$escape = false`

[tangix]

PHP Version

8.1

CodeIgniter4 Version

4.3.0

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

macOS

Which server did you use?

apache

Database

MySQL 8.0.31

What happened?

Upgrading from 4.2.11 to 4.3.0 I notice changes in the QueryBuilder´s escape logic affecting BETWEEN statements.

Steps to Reproduce

This code from 4.2.11 works as expected:

            $db      = db_connect(Table::READONLY);
            $between = 'BETWEEN ' . $db->escape($from) . ' AND ' . $db->escape($to);

            $completed = $db
                ->table(Table::subsredeem)
                ->selectCount('*', 'cnt')
                ->where('created_on IS NOT NULL', '', false)
                ->where('created_on ' . $between, '', false)    // <--- This is the interesting line!
                ->where('completed', 1)
                ->get()
                ->getRow(0)?->cnt;

After upgrading to 4.3.0 the "interesting line" produces bad SQL causing an Exception to be thrown. The QueryBuilder adds a =:

$this->getCompiledSelect(false)
SELECT COUNT(*) AS `cnt`
FROM `subsredeem`
WHERE created_on IS NOT NULL 
AND created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59' = 
AND `completed` = 1

Expected Output

Working SQL query.

Anything else?

The work-around I'm currently using:

 $completed = $db
                ->table(Table::subsredeem)
                ->selectCount('*', 'cnt')
                ->where('created_on IS NOT NULL', '', false)
                ->where(new RawSql('created_on ' . $between)) // <--- Use RawSql here!
                ->where('completed', 1)
                ->get()
                ->getRow(0)?->cnt;

[sclubricants]

Your problem seems to be that you are using an empty string for the second parameter of where().

Should be $builder->where('created_on ' . $between, null, false)

where($key[, $value = null[, $escape = null]])

The default value of the second parameter is null, not an empty string. This is because you may want column = ''

If this was working in 4.3.x it was probably by mistake.

https://www.codeigniter.com/user_guide/database/query_builder.html#where

[kenjis]

Using '':

        $builder = $this->db->table('subsredeem');
        $between = "created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59'";
        $sql = $builder->selectCount('*', 'cnt')
            ->where('created_on IS NOT NULL', '', false)
            ->where($between, '', false)
            ->where('completed', 1)
            ->getCompiledSelect();

SELECT COUNT(*) AS "cnt"
FROM "subsredeem"
WHERE created_on IS NOT NULL 
AND created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59' = 
AND "completed" = 1

Using null:

        $builder = $this->db->table('subsredeem');
        $between = "created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59'";
        $sql = $builder->selectCount('*', 'cnt')
            ->where('created_on IS NOT NULL', null, false)
            ->where($between, null, false)
            ->where('completed', 1)
            ->getCompiledSelect();

SELECT COUNT(*) AS "cnt"
FROM "subsredeem"
WHERE created_on IS NOT NULL
AND created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59'
AND "completed" = 1

[kenjis]

Without $escape = false:

        $builder = $this->db->table('subsredeem');
        $between = "created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59'";
        $sql = $builder->selectCount('*', 'cnt')
            ->where('created_on IS NOT NULL')
            ->where($between)
            ->where('completed', 1)
            ->getCompiledSelect();

SELECT COUNT(*) AS "cnt"
FROM "subsredeem"
WHERE "created_on" IS NOT NULL
AND "created_on" BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59'
AND "completed" = 1

[kenjis]

I don't know why $value = '' works in v4.2, but it should be $value = null when you don't set a value.

[sclubricants]

You need $value = '' in cases where you looking for a column with empty but not null values.

[kenjis]

I mean in v4.2.12 the following code works:

        $builder = $this->db->table('subsredeem');
        $between = "created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59'";
        $sql = $builder->selectCount('*', 'cnt')
            ->where('created_on IS NOT NULL', '', false)
            ->where($between, '', false)
            ->where('completed', 1)
            ->getCompiledSelect();

SELECT COUNT(*) AS "cnt"
FROM "subsredeem"
WHERE created_on IS NOT NULL 
AND created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59'
AND "completed" = 1

[sclubricants]

Well you could never really use $value = '' when escape is false.. maybe this created a loophole.

[tangix]

Related or not - https://github.com/codeigniter4/CodeIgniter4/issues/7143 I do start to find QueryBuilder scary to use...

[kenjis]

This issue is not a bug, but a misuse. In v4.2.12 or before, the misuse generated working SQL statement. It is a bug.

Misuse:

$builder = $this->db->table('subsredeem');
        $between = "created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59'";
        $sql = $builder->selectCount('*', 'cnt')
            ->where($between, '', false) // Using `''` with `false`
            ->getCompiledSelect();
/*
string (118) "SELECT COUNT(*) AS "cnt"
FROM "subsredeem"
WHERE created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59' = "
*/

Correct usage:

$builder = $this->db->table('subsredeem');
        $between = "created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59'";
        $sql = $builder->selectCount('*', 'cnt')
            ->where($between, null, false) // Using `null` with `false`
            ->getCompiledSelect();
/*
string (115) "SELECT COUNT(*) AS "cnt"
FROM "subsredeem"
WHERE created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59'"
*/

[kenjis]

@tangix @sclubricants I sent a PR to update the user guide: #7151

[kenjis]

I found the cause of this issue. See #7155

But I still think where($between, '', false) is a misuse. Because '' will be empty string when $escape is false.