Change postgresql to use concat() instead of || - #19584
Conversation
210249f to
dc28882
Compare
|
@markstory Because only the bound parameters are cast, we could use the postgres shorthand |
| $driver = Mockery::mock(Postgres::class) | ||
| ->makePartial() | ||
| ->shouldAllowMockingProtectedMethods(); | ||
| $driver->__construct([]); | ||
| $driver->shouldReceive('enabled')->andReturn(true); | ||
| $driver->shouldReceive('connect')->andReturnNull(); | ||
| $driver->shouldReceive('getPdo')->andReturn(Mockery::mock(PDO::class)); | ||
| $driver->shouldReceive('version')->andReturn('16.0'); |
There was a problem hiding this comment.
Is this mock necessary? We could skip the test if the postgres version is too low instead.
There was a problem hiding this comment.
Is this mock necessary? We could skip the test if the postgres version is too low instead.
Not sure. It was just following other examples. It just tests sql generation on all builds I guess. Other tests already include concat(). I can double-check they test different parameter types.
There was a problem hiding this comment.
Update to use postgres connection and verify sql. The FunctionsBuilder test just checks we generate a valid function expression.
dc28882 to
4efb56b
Compare
4efb56b to
ca5b391
Compare
2 participants
I think this change probably belongs in 6.0.
PostgreSQL added
concat()in 9.1 with proper support for NULL arguments - they are ignored instead of returning NULLIn postgrs, bound parameters have to be cast to
textto satisfy the variadic arguments since it requires types be known at compile time to support argument overloading.The concat() function should be what we want and only be safer, but since it would change the return type for NULL arguments, it's not technically backwards compatible.