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

Bug: [QueryBuilder] join() with BETWEEN causes TypeError preg_quote(): Argument #1 ($str) must be of type string, false given #8791

Closed
oakhurstmgmt opened this issue Apr 15, 2024 · 7 comments · Fixed by #8792
Labels
bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer

Comments

@oakhurstmgmt
Copy link

PHP Version

8.3

CodeIgniter4 Version

4.5.1

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Windows, Linux

Which server did you use?

apache

Database

MariaDB 11.2.3

What happened?

After upgrading to 4.5.0 and then 4.5.1, I am now getting TypeError preg_quote(): Argument #1 ($str) must be of type string, false given at SYSTEMPATH/Database/BaseBuilder.php at line 680 with some of my SQL queries. These queries all use the BETWEEN Operator to compare dates.

These queries worked perfectly fine going all the way back to CI 4.0.x up through 4.4.8.

Below is a screenshot of the error:
Screenshot 2024-04-15 at 10-50-16 TypeError

Steps to Reproduce

Execute an SQL query created using the Query Builder Class utilizing the BETWEEN operator.

Expected Output

A view with the results of the SQL query.

Anything else?

No response

@oakhurstmgmt oakhurstmgmt added the bug Verified issues on the current code behavior or pull requests that will fix them label Apr 15, 2024
@kenjis
Copy link
Member

kenjis commented Apr 15, 2024

How do we reproduce?
It seems Query Builder does not support BETWEEN.
https://codeigniter4.github.io/CodeIgniter4/database/query_builder.html

@kenjis kenjis added database Issues or pull requests that affect the database layer and removed bug Verified issues on the current code behavior or pull requests that will fix them labels Apr 15, 2024
@oakhurstmgmt
Copy link
Author

oakhurstmgmt commented Apr 15, 2024

I probably did not phrase the bug report correctly. I have two SQL queries that have worked perfectly from CI version 4.0.x all the way through 4.4.8. Both of these queries are constructed using the Query Builder Class. Here are the lines from each Query Builder that appear to be throwing the type error:

->join('leases', 'units.unit_id = leases.unit_id AND CURDATE() BETWEEN lease_start_date AND lease_exp_date', 'left')

->join('leases', 'units.unit_id = leases.unit_id AND workorder_start BETWEEN lease_start_date AND lease_exp_date', 'left')

The only common thread seems to be the use of the BETWEEN SQL operator. I'm not sure that has anything to do with it.

It does seem to be an issue in Query Builder because it no longer puts together the query without throwing an error.

It could simply be a strict typing issue like some of the other 4.5.0 bugs, but I will do some further troubleshooting.

@kenjis
Copy link
Member

kenjis commented Apr 15, 2024

Thank you for the feedback.

There is no sample code with BETWEEN in join().
https://codeigniter4.github.io/CodeIgniter4/database/query_builder.html#join
and there is no test case:
https://github.com/codeigniter4/CodeIgniter4/blob/develop/tests/system/Database/Builder/JoinTest.php

We need further investigation.

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Apr 15, 2024
@kenjis kenjis changed the title Bug: TypeError preg_quote(): Argument #1 ($str) must be of type string, false given Bug: [QueryBuilder] join() with BETWEEN causes TypeError preg_quote(): Argument #1 ($str) must be of type string, false given Apr 15, 2024
@kenjis
Copy link
Member

kenjis commented Apr 16, 2024

It seems join() does not support BETWEEN, but it worked by accident in previous versions.

When there is a conditon 'units.unit_id = leases.unit_id AND CURDATE() BETWEEN lease_start_date AND lease_exp_date', it is parsed in $conditions as

array (
  0 => 'units.unit_id = leases.unit_id',
  1 => 'CURDATE() BETWEEN lease_start_date',
  2 => 'lease_exp_date',
)

but it is of course wrong.

Fixing this behavior is difficult, so I sent ad hoc fix PR #8792

@oakhurstmgmt
Copy link
Author

Ahh, I understand now.

I replaced the BETWEEN operators in my JOIN clauses with a combination of >= and <= operators to workaround this limitation with Query Builder.

This project also has many WHERE clauses constructed by Query Builder utilizing the BETWEEN operator that are still working as expected in 4.5.1. Does where() officially support BETWEEN or is is this also working by accident?

@kenjis
Copy link
Member

kenjis commented Apr 16, 2024

There is one test case.

public function testWhereCustomStringWithBetweenEscapeFalse(): void
{
$builder = $this->db->table('jobs');
$where = "created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59'";
$builder->where($where, null, false);
$expectedSQL = "SELECT * FROM \"jobs\" WHERE created_on BETWEEN '2022-07-01 00:00:00' AND '2022-12-31 23:59:59'";
$this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect()));
$expectedBinds = [];
$this->assertSame($expectedBinds, $builder->getBinds());
}

But note that uses false in the third parameter $escape:
$builder->where($where, null, false);

If $escape is set to false, the value as is becomes a part of the SQL statement, so you can freely create SQL statements, but QueryBuilder does not provide any protection/escaping.

@kenjis
Copy link
Member

kenjis commented Apr 28, 2024

@oakhurstmgmt If you agree to merge #8792, approve the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants