Add RawSql to BaseConnection->escape()

[sclubricants]

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:

• 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

[MGatner]

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 ?

[kenjis]

Please update the all notes in query_builder.rst:

.. note:: All values are escaped automatically producing safer queries.

[MGatner]

Looking good! Thanks for all the reviews.

[sclubricants]

.. 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.

[kenjis]

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.

[kenjis]

.. note:: All values except RawSql are escaped automatically producing safer queries.

Good. And can you add the following warning, too?

[sclubricants]

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.

[sclubricants]

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().

[kenjis]

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.

[sclubricants]

Maybe RawSql needs its own page

[sclubricants]

I wonder if this will conflict with binds..

[MGatner]

I wonder if this will conflict with binds..

Sounds like a test case is in order 😊

[sclubricants]

No conflict with binds but did have to fix objectToArray() in BaseBuilder

[kenjis]

No conflict with binds but did have to fix objectToArray() in BaseBuilder

Why doesn't the existing test fail?
Is a test case missing?

[sclubricants]

The existing test just tests escape(). Ill write some tests to test things like insert().

[sclubricants]

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.

[MGatner]

@kenjis last call?

[kenjis]

Sorry, can you rebase to resolve the conflict?

[iRedds]

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);

[sclubricants]

The usage example is rather ambiguous.
It can be replaced with.

Your trick won't work on insertBatch()

[kenjis]

@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.

[MGatner]

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?

[kenjis]

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.

[MGatner]

Thanks all!