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

feat: QueryBuilder raw SQL string support #5817

Merged
merged 10 commits into from
May 4, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Mar 21, 2022

Related: #4355, #3970, #5810

Description
See https://forum.codeigniter.com/showthread.php?tid=80720

  • add RawSql class
  • select() supports RawSql
  • where() supports RawSql
  • like() supports RawSql

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis marked this pull request as draft March 21, 2022 05:50
@kenjis kenjis added the enhancement PRs that improve existing functionalities label Mar 21, 2022
@kenjis kenjis force-pushed the feat-qb-raw-sql branch 3 times, most recently from 6b94ae9 to a7ab894 Compare March 21, 2022 12:15
@iRedds
Copy link
Collaborator

iRedds commented Mar 22, 2022

Is it possible to add a second argument to the constructor to pass the binding?
As for BaseConnection::query();

new RawSql('a = ? AND b = ?', [1, 2]);
new RawSql('a = :a AND b = :b', ['a'=>1, 'b' => 2]);

@kenjis
Copy link
Member Author

kenjis commented Mar 23, 2022

@iRedds I'm not sure.

Can you tell me what's good if it is possible?
Use case?

@iRedds
Copy link
Collaborator

iRedds commented Mar 23, 2022

Usage example above.
It seems to me that it will be convenient with a large SQL statement and a large number of variables.

For example, for such a query (from #5810 )

$builder->orLike("(SELECT COALESCE(GROUP_CONCAT(CONCAT(corr.description, ' ', corr.note) ORDER BY corr.order_field ASC SEPARATOR ' '), '') FROM correspondence corr WHERE corr.contact_fk = c. contact_id AND 1 = 1)", "", "both", false);

Although this query does not use value substitution, this is a possible situation.

@kenjis kenjis force-pushed the feat-qb-raw-sql branch 3 times, most recently from 8699ada to a1eabc7 Compare March 31, 2022 11:30
@kenjis
Copy link
Member Author

kenjis commented Apr 1, 2022

@iRedds What's about escaping?
If we need to escape the values, RawSql must have DB Connection.

@kenjis kenjis marked this pull request as ready for review April 1, 2022 04:02
@iRedds
Copy link
Collaborator

iRedds commented Apr 1, 2022

It seems to me that there are two options without using DBC

  1. The developer himself will escape the values.
  2. The binds will be transferred to the $binds property when processing the RawSql instance.

@kenjis
Copy link
Member Author

kenjis commented Apr 7, 2022

I do not see much need for binding, so I will not include it in this PR.

@MGatner MGatner added the database Issues or pull requests that affect the database layer label Apr 10, 2022
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

I think this solution is fine, and I don't see an issue with leaving binds out. Being raw SQL I don't think that's unexpected. Thanks @kenjis

But it does need docs added.

@kenjis
Copy link
Member Author

kenjis commented Apr 13, 2022

I added docs.

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

I thought that the $escape parameter would do the trick for this type of scenario but I guess I was wrong. I think it is a good addition.

@kenjis kenjis merged commit 2a5322b into codeigniter4:develop May 4, 2022
@kenjis kenjis deleted the feat-qb-raw-sql branch May 4, 2022 06:43
@kenjis
Copy link
Member Author

kenjis commented May 4, 2022

@michalsn Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Issues or pull requests that affect the database layer enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants