fix: fix share reminder job for oracle #48182
Conversation
Signed-off-by: Robin Appelman <robin@icewind.nl>
icewind1991
force-pushed
the
oracle-share-reminder
branch
from
00c13be
to
b45f242
Compare
icewind1991
requested review from
a team,
ArtificialOwl,
Altahrim,
nfebe,
artonge and
come-nc
and removed request for
a team
| @@ -28,7 +29,7 @@ interface IQueryBuilder { | |||
| /** | |||
| * @since 9.0.0 | |||
| */ | |||
| public const PARAM_BOOL = ParameterType::BOOLEAN; | |||
| public const PARAM_BOOL = Types::BOOLEAN; | |||
There was a problem hiding this comment.
This sounds like a serious bug in doctrine which we should report and get addressed upstream?
The binding type should be ParameterType::
https://github.com/nextcloud/3rdparty/blob/9ca60e9d2b12f711a756c35d37fd19c178db685b/doctrine/dbal/src/Types/BooleanType.php#L56-L59
Doc block of ParameterType also states that it is for statement parameters:
https://github.com/nextcloud/3rdparty/blob/a9be8d0755e6d786c34e44ef29e88d8f9b90c65b/doctrine/dbal/src/ParameterType.php#L6
The Types are the dbal types
There was a problem hiding this comment.
Let me try if bumping doctrine to 3.9.1 fixes it #48306
There was a problem hiding this comment.
Okay, still fails with 3.9.1
There was a problem hiding this comment.
Thanks!
I've debugged the weirdness with OCI and booleans a while ago: #45630
The type constants from doctrine/dbal allows us to use doctrine type conversion logic. I was unsure back then about changing it because apps, especially on OCI, may depend on the broken behavior.
Should we also update the other constants to use the doctrine types?
There was a problem hiding this comment.
Should we also update the other constants to use the doctrine types?
As said I think the switch that was done here is actually wrong. It uses the DBAL architectural type now and not the PDO parameter type anymore.
There was a problem hiding this comment.
As said I think the switch that was done here is actually wrong. It uses the DBAL architectural type now and not the PDO parameter type anymore.
Depends ;)
Using the DBAL type leads to doctrine taking care of the type juggeling for us. That's probably good, but also a risky change. We don't know if the results are still the same.
We are not using the pdo driver for OCI. I think that's the reason for the weird behavior with OCI, especially with booleans. The OCI driver just didn't know what to do with type "5" (because that's the pdo constant) and fallbacks to string as default. If we use the dbal type doctrine will convert the boolean to 0/1 and set type int.
I've discussed #45630 with Christoph a while ago, and we didn't know how to move forward 🤷
We updated doctrine since then, not sure if my observations are still correct
There was a problem hiding this comment.
Note that we already use the DBAL types for datetime and json
|
Backport? |
|
For v30 it sounds like a safe bet. For former versions, if an issue that it should be fixed too |
|
@nickvergessen does your backport cover 30? |
|
I only backported the database/querybuilder change, not the reminder job stuff |
|
@icewind1991 does this need backporting (broken on 30 or older?) |
|
The share reminder didn't get backported so it's not required to backport this |
This was always broken but nobody noticed because of CI issues