Affects Version/s: 2.4
Fix Version/s: None
Security Level: All
This is a follow-up to issue #457 ("Use int values instead of strings for PostgreSQL booleans"), which is still not fixed. Int values are no solution at all. In fact the root cause lies deeper, outside the PostgreSQL platform class.
1. The patch to fix #457 does not change the default behaviour of the PostgreSQL platform class (method convertBooleans() returns strings 'true'/'false'). When the PostgreSQL PDO driver is configured to emulate prepared statements, it still results in unexpected failures, storing boolean false entity values as true in the database.
2. The new alternative boolean conversion mode activated by PostgreSqlPlatform::setUseBooleanTrueFalseStrings(false) is of no use as it prevents execution of DQL queries with boolean conditions, because integers 0 and 1 are not valid boolean literals in PostgreSQL.
The root cause is the notion of a PHP value being convertible to a database value. Because in fact there are two different types of "database values":
- Literals used directly in SQL statements
- Values passed as parameters to prepared statements
To make this absolutely clear:
There are two locations in DBAL and ORM where AbstractPlatform::convertBooleans() is called to build SQL literals:
Wow, this is even being enclosed in single quotes!? But then the whole method is buggy anyway, e.g. using an unescaped string value for a string literal (scenarios for SQL injection unlikely but possible).
...and the result is later used as a boolean literal in an SQL query.
To solve this we need something like AbstractPlatform::convertBoolToSqlLiteral() (returning strings true and false for the Postgres platform) and AbstractPlatform::convertBoolToDbValue() (converting to integer 0 or 1 for platforms without a native bool type).
Note 1: The docs currently suggest to call $conn->getDatabasePlatform()->setUseBooleanTrueFalseStrings($flag). This is bad OO design, because getDatabasePlatform() returns an AbstracPlattform instance which does not have a contract for the method.
Note 2: What makes this problem so nasty is the fact that switching to emulated prepares makes an application fail in a non-obvious way. There will be no traceable errors but simply all boolean false values in ORM entities stored as boolean true. When integration tests use a different database (e.g. an SQLite in-memory DB to minimize test execution time) the problem will even escape the tests. And the distance between cause and effect also makes the problem very hard to find. Who would expect a database driver setting to cause booleans in the DB to be the opposite of what they're supposed to be? Especially as this only becomes apparent after later re-hydrating stored entities.
Note 3: Why emulated prepared statements matter: When PostgreSQL processes a prepared statement, its query planner works out a query plan and uses it for all subsequent executions of this query. This way it has to make a rather crude guess at the number of affected rows from each table in a join. When a non-prepared query is executed, the query planner can take into account the given values (mostly the ones in the "WHERE" part of the query) and make a much more specific guess at which plan will perform best.
In our case, we decided to switch to emulated prepares after we found out that a complex query in our application would run five times faster with emulated prepares.
Note 4: Is there a reason for AbstractPlatform::convertBooleans() accepting either a single bool value or an array of bool values? I didn't find client code calling it with an array. This makes the method less obvious, is currently implemented with code duplication and at least for the PostgreSQL plattform class, the "array of bool" functionality is not even tested.