Additional space in a parameter which is a string (mysql)

[nowackipawel]

During my tests I noticed sth what should not happened I noticed that i've values are around 2 time lower than they should:
$this->select('ROUND(100*AVG(IFNULL(t_apk_game2users.agu_ok, 0))/AVG(LENGTH(aga_questions) - LENGTH( REPLACE ( aga_questions, ",", "")) +1)) agu_avg_ok')->...
but it produces query with additional space inside replace:
ROUND(100*AVG(IFNULL(t_apk_game2users.agu_ok, 0))/AVG(LENGTH(aga_questions) - LENGTH( REPLACE ( aga_questions, here-->", "<--here, "")) +1))

aga_questions is varchar which stores json_encode of int[]
agu_ok is TINYINT column

So as for now I had to change my query to get real results:
ROUND(100*AVG(IFNULL(t_apk_game2users.agu_ok, 0))/AVG((LENGTH(aga_questions) - LENGTH( REPLACE ( aga_questions, ", ", "")) +2)/2))

[michalsn]

It seems like a bug inherited from CI3.

It's worth mention that this bug is valid only for , sign.

We can easy fix this by changing https://github.com/bcit-ci/CodeIgniter4/blob/develop/system/Database/BaseBuilder.php#L2408 but then the readability of produced query will suffer.

Any other ideas?

[nowackipawel]

maybe we could use regexp to know if we are trying to modify string (between closest (n+1)" and (n+2)" or ') or we just formatting arguments. (i'm not regexp master :()

[michalsn]

To be honest I have no idea if we can provide a regex expression, that will handle all the possibilities... and is seems like complicating a simple thing.

Personally I would go with implode changes, since it would match what we do ealier in select method with explode. We should leave the formating part of the select query to the developer entirely.

@lonnieezell any thoughts?

[nowackipawel]

I think if query is provided by developer as plain text (not ie. array of columns, etc.) then it is his duty to make it format as is required by his/her standards. Anyway I can imagine that in production environment it can cause serious problems if we left it as is now. I will go even farther and I'd like to suggest similar changes in CI3 --but not here.

[lonnieezell]

Honestly I don’t think there’s a good fix for this. The select clause you’re using is a pretty complex one and is definitely more complex than the system was intended for. If we were to remove the space from the implode than many more people would complain about the formatting of their code. It might have other side effects but I’m not sure on that.

I can’t inagine writing a regular expression that could interpret every single expression correctly and not mess other things up.

As mentioned in the docs, though, you can pass false as the second argument to the select() method and it will not try to protect your identifiers. This is specifically intended for when you have complex selects.

[nowackipawel]

I'm almost sure I passed false during my test with the same result (this parameter is in CI since CI2 I think...) ... I will be back if I find out it still will mess up with false as 2nd parameter. thx.

[michalsn]

Well... I had a feeling it's gonna end like this. Fair enough :)

For complex queries we should stay with $db->query() method then.

BTW - protecting identifiers is not an issue here.

[nowackipawel]

Ok - not complex query at all:

$this->builder()->select('LENGTH(",")',false)->get()->getResult();

or
$this->select('LENGTH(",")',false)->get()->getResult();
gives me:
"SELECT LENGTH(", ") FROM t_apk_game2users",

[michalsn]

Heh... every select query that depends somehow on , sign must be considered as "complex" with CodeIgniter Query Builder :)

At least we have a workaround.

[nowackipawel]

Will be a good solution additional 3rd optional parameter for select $itIsMyDutyToFormatMyQuery = false? :)