codeigniter4
Add RawSql to BaseConnection->escape()
This PR adds RawSql type to BaseConnection->escape(). It does not escape the RawSql.
This allows passing SQL functions in columns used for DML operations.
For instance you could pass CURRENT_TIMESTAMP() to a date time field instead of a literal value.
You could call your own SQL function as well.
Another is to pass SQL constants like DEFAULT in order to fill a column with default values.
<?php
$data = [
'id' => new RawSql('DEFAULT'),
'title' => 'My title',
'name' => 'My Name',
'date' => '2022-01-01',
'last_update' => new RawSql('CURRENT_TIMESTAMP()'),
];
$builder->insert($data);
/* Produces:
INSERT INTO mytable (id, title, name, date, last_update)
VALUES (DEFAULT, 'My title', 'My name', '2022-01-01', CURRENT_TIMESTAMP())
*/
Checklist:
- [x] Securely signed commits
- [x] Component(s) with PHPDoc blocks, only if necessary or adds value
- [x] Unit testing, with >80% coverage
- [x] User guide updated
- [x] Conforms to style guide
Looks like a cool feature! The changes look good to me but I'd rather someone with a better idea of the larger database implications handle reviews. @iRedds or @paulbalandan ?
Please update the all notes in query_builder.rst:
.. note:: All values are escaped automatically producing safer queries.
.. note:: All values are escaped automatically producing safer queries.
@kenjis What do you propose we do here?
How about:
.. note:: All values except ``RawSql`` are escaped automatically producing safer queries.
I think the following section is weird. Because it explains escape(), but it explains about not escaping a lot.
I think we should remove the sample and description for RawSql here.
Because it is not needed here.
This section explains how to use escape() and its behavior.
Devs never writes or should not write code like $db->escape(new RawSql('...')).
It does not make sense.
See the first sample code. The assumption here is that SQL statements are generated by concatenating strings. Devs can write SQL functions and constants as they like in a string. But the sentence "This allows you to call SQL functions and constants" is an explanation when we use QueryBuilder.
.. note:: All values except
RawSqlare escaped automatically producing safer queries.
Good. And can you add the following warning, too?
So you want to remove everything? Then none will know about its availability for use.
I think the example is good because it explains what the method does. The method does different things to different types of data. It could be possible to add other data types such as Datetime.
Its important to know how these datatypes are treated.
Developers might should understand that the escape method is what they are relying on when they employ functions like insert. They might not write the actual code for db->escape() but they are none the less calling it via ignoring the escape parameter on functions like insert().
So you want to remove everything?
I thought again, and yes. Because the page explains how to use $db->query() (and simpleQuery()).
So RawSql is not needed for them.
Your explanation about RawSql and escape() is good, but it should not in the place.
Developers might should understand that the escape method is what they are relying on when they employ functions like insert. They might not write the actual code for db->escape() but they are none the less calling it via ignoring the escape parameter on functions like insert().
Yes, and the explanation should be in the insert() page. It is a QueryBuilder method.
I wonder if this will conflict with binds..
Sounds like a test case is in order 😊
No conflict with binds but did have to fix objectToArray() in BaseBuilder
No conflict with binds but did have to fix objectToArray() in BaseBuilder
Why doesn't the existing test fail? Is a test case missing?
The existing test just tests escape(). Ill write some tests to test things like insert().
Added RawSql test.. to test custom SQL function:
SELECT setDateTime('2022-06-01')
Returns: 2022-06-01 01:01:11
This way the RawSql has quotes in it to test and make sure quotes aren't being escaped.
What do you guys think about adding a helper method:
/**
* Creates RawSql class
*/
public function rawSql(string $sql): RawSql
{
return new RawSql($sql);
}
$data = [
'id' => $this->db->rawSql('DEFAULT'),
'title' => 'My title',
'name' => 'My Name',
'date' => '2022-01-01',
'last_update' => $this->db->rawSql('CURRENT_TIMESTAMP()'),
];
I've got some more uses of RawSql I'm working on. I think we should create a page in documentation just for RawSql.
I think a dedicated documentation page is a good idea.
I don't like the rawSql() method - it functions as a named constructor which should belong on the class itself. In actuality though the constructor itself is sufficient, so this simply ends up creating a class dependency that wasn't there before.
Open to other ideas or push-back if I have kissed something.
If there's impetus to wrap this in a method then it should be something like RawSql::of($sql), with an optional protected constructor. But I still don't know that it adds anything.
I was just looking for a solution that made easy access to class without having to include the class path. I thought a method like would make development easier.
I was just looking for a solution that made easy access to class without having to include the class path.
Why do you want not having the class path? Don't you use IDE? If you really want it, it would be good to have a helper (global function) for it.
I won't look back through all of it, so unless anyone else has more comments this is good by me. Thanks for this feature @sclubricants - very nice addition!
I made a couple of fixes. One is that I didn't get the logic quite right for the objectToArray() method.
The other is that I was using is_a($val, RawSql::class) and changed it to $val instanceof RawSql.
The php function is_a() requires the first parameter be an object or class, whereas instanceof does not.
https://www.php.net/manual/en/function.is-a.php
https://www.php.net/manual/en/language.operators.type.php
is_a() defaults to objects only unless you use the third parameter, so what you had was fine but I think instanceof is the preferred choice here anyway.
The usage example is rather ambiguous. It can be replaced with.
$data = [
'title' => 'My title',
'name' => 'My Name',
'date' => '2022-01-01',
];
$builder->set([
'id' => 'DEFAULT',
'last_update' => 'CURRENT_TIMESTAMP()',
], null, false)->insert($data);
The usage example is rather ambiguous. It can be replaced with.
Your trick won't work on insertBatch()
@iRedds To be honest, I would like to throw an Exception to such code. It is difficult to read.
By the way, what do you mean ambiguous ? It seems to me the sample code is easy to read and understand.
I think @iRedds was pointing out that we can already accomplish the same thing with other tools. insertBatch() is a good counter-example. I also assume this is the only way to use a Model to insert non-escaped data?
I agree that we can already accomplish the same thing with other tools. But I think the sample code is okay even if there is another way to do the same thing.