Fix bug with handling boolean values in set() method

[michalsn]

Description
This PR introduces some potential BC but I treat this as a bug that has to be fixed. At the end of the day calling
$builder->set('bool', true); and $builder->set(['bool' => true]); should have the same effect.

I have added additional tests for boolean values and upgrade instructions.

Fixes #4761

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[MGatner]

Changing the BaseBuilder signature seems like a pretty big ask. We've had numerous issues with boolean handling in the database layer, so I'm aware there might be some "mess" fixing things. But is this necessary to be able to accomplish the goal? Does set($key, '1') work for booleans with strict mode?

[michalsn]

Changing the BaseBuilder signature seems like a pretty big ask. We've had numerous issues with boolean handling in the database layer, so I'm aware there might be some "mess" fixing things. But is this necessary to be able to accomplish the goal?

Yes, I understand it isn't an easy change, but I feel that this is the right way to handle this.

Unless we want to "force" users to use "0" and "1" instead of booleans then I don't see another option. But even if we do that, we will still have different behavior when it comes to arrays. I would really like to avoid it.

Does set($key, '1') work for booleans with strict mode?

Yes, it does.

---

In general, this wasn't working as it should from the beginning. A few things:

• we can't blame users to try to use true and false values when it comes to the boolean field type. The tricky part is that setting true will work just fine for most cases (MySQLi, SQLite3, Postgre, SQLSRV). But will fail when the user will set false.
• I agree that forcing users to use "0" or "1" possibly could work (though it would involve many confusions)

The main reason I'm proposing this change is that I strongly feel that using $builder->set('bool', true) should be producing the same result as using $builder->set(['bool' => true]). Now we have a situation where arrays are handled properly and setting values by a single key is failing.

Let me summarize this by a table. Without this PR situation look like this:

	set(['bool' => true])	set(['bool' => false])	set('bool', true)	set('bool', false);
SQLite3	🆗 (1)	🆗 (0)	🆗 ("1")	❌ ("")
MySQLi	🆗 (1)	🆗 (0)	🆗 ("1")	❌ ("")
Postgre	🆗 ("TRUE")	🆗 ("FALSE")	🆗 ("1")	❌ ("")
SQLSRV	🆗 (1)	🆗 (0)	🆗 ("1")	❌ ("")

If this PR is merged, everything will be fixed.

---

I don't see the better way, but I'm always open to suggestions.

[michalsn]

Additionally, there is another problem with the set() method besides handling booleans. We can't assign an integer value since we're casting the variable. This will cause the escape() method to use escapeString() on variable instead of leaving it as an integer. So even if we write set('number', 1) in the query, we will get something like (number) VALUES ('1').

This will cause additional performance hit because the database engine will have to typecast the value, for most of the time it won't be something crucial though.

In any case, it prevents the developer from generating the query he expected.

[MGatner]

Thank you, that was a very helpful summary and explanation of the issue. I will continue this conversation with the caveat that the database layer is my weakest area and I have very little experience outside of MySQL and Firestore - which means I'm coming at this mostly from a code perspective. I would love some others to chime in as well.

I'm not saying I agree with the signature, but it is there and clearly at some point the intent was for input to be strings. Could we go the opposite direction and enforce $key objects/arrays to have string-only values? This would make it clearer for the developer that "everything must be a string" so there would be no expectation that either would work:

• $builder->set('bool', true)
• $builder->set(['bool' => true])

[MGatner]

One more alternative: since this is only an issue for MSSQL, we could loosen the signature to set($key, $value = '', bool $escape = null) on just that builder for now and make note of this as a change to roll up in version 5. (Though I secretly hope version 5 has a completely different database layer.)

[michalsn]

I haven't checked at what point casting for $value had entered the game (maybe it was there from the beginning?) but I think the initial intention was to prevent users from assigning an array (probably). The reasons behind this were most likely valid but at the same time some things were missed (integers, booleans) and now we have some problems.

Could we go the opposite direction and enforce $key objects/arrays to have string-only values?

I think that casting everything to strings would be even more damaging - since it would possibly break applications that are working just fine right now. Please notice that when we call update([]) or insert([]) all these values under the hood are set via set([]) method.

One more alternative: since this is only an issue for MSSQL

Not really. It's actually an issue for every driver when we're trying to assign a false value.

I understand concerns about removing casting in this method but with proper documentation, it shouldn't cause many issues for developers. I personally think the current implementation is faulty.

I look forward to hearing from others.

[paulbalandan]

I'm fine with this if this would be the straightforward fix to the issue rather than make convoluted workarounds.

[lonnieezell]

If @michalsn can get the upgrade doc included in a toctree, I'm good with these changes.

[michalsn]

@MGatner I guess this is ready to go?

[MGatner]

🙈🙉🙊