Default Statement::bindParam $length to 0, as using null causes a deprecation with PHP8+

[cjunge-work]

When using PHP8+ with Doctrine DBALv2 and calling PDOStatement::bindParam() with just 2 params causes a deprecation for passing null to an int param. The default in PDO is 0, so this change defaults to 0 when calling the decorated statement.

Fixes #611

[ste93cry]

Thank you for taking the time to contribute this change! Besides adding an entry to the CHANGELOG, I just have a doubt about why the current solution does not work: if you call bindParam with up to 3 params, the array_slice should return an empty array, and the same thing should happen here. As a consequence of this, PHP should fallback to the default value for the $length param 🤔

[cjunge-work]

Thank you for taking the time to contribute this change! Besides adding an entry to the CHANGELOG, I just have a doubt about why the current solution does not work: if you call bindParam with up to 3 params, the array_slice should return an empty array, and the same thing should happen here. As a consequence of this, PHP should fallback to the default value for the $length param thinking

We don't run Sentry on our dev environments, and we have been unable to replicate this issue without Sentry enabled. I'm not sure why the DBAL PDOStatement is working fine but the Sentry TracingStatementv2 isn't.

I haven't done any in-depth exploration, but it appears to me that the default null on the tracing statement bindParam method becomes the default if it's not explicitly passed in. With <PHP8.1 it works fine (I guess PHP implicitly casts it to 0 or ignores it & uses the PDOStatement default), but under PHP8.1 with the changes to deprecate passing null to non-nullable internal function parameters (https://www.php.net/releases/8.1/en.php) the null is triggering the deprecation.

[cjunge-work]

@ste93cry I deleted my previous comment as it was wrong. Here's an example showing how the explicit null triggers the deprecation, but not providing the param doesn't: https://3v4l.org/biaIs#v8.1.4. I'm still not sure why the deprecation is being triggered in Sentry.

[Jean85]

Can we add a unit test to trigger the deprecation in our CI? Or is it already there, and then why it's not failing?

[ste93cry]

the explicit null triggers the deprecation, but not providing the param doesn't

That's the expected behaviour. If you explicitly pass null, then it's up to you to know that you will trigger a deprecation. Silly question: are you sure that there is no other code that pass that param explicitly?

[cjunge-work]

@ste93cry My point was that the explicit null triggers the deprecation, when not using Sentry, so it's not a side-effect of the code.

<?php

declare(strict_types=1);

use Doctrine\DBAL\Driver\Connection;

class aClass
{
    public function __construct(Connection $connection)
    {
        $this->connection = $connection;
    }

    public function aMethod($applicationAverageTime) {
        $statement = $this->connection->prepare('...');
// ...
        $statement->bindParam(':applicationAverageTime', $applicationAverageTime); // <---
// ...
    }
}

that's the line that our last deprecation was triggered on, we don't tend to even use the type.

[ste93cry]

Sorry for being silent. I reproduced the issue locally, and I found out that the problem originates from here. Basically, since the statement is wrapped by our TracingStatementForV2 class, the instanceof PDOStatement returns false. Then, the $length parameter is explicitly passed to the bindParam method with a null value and this triggers the deprecation. I would say that the check in the DBAL is pretty weak as-is, and should be changed to take into account the fact that the statement could be wrapped. Anyway, your fix is good, so if you are willing to fix the requested changes I would happily merge this PR.

[ste93cry]

I took the time to finish your work, hopefully you don't mind. Thank you for your contribution, I will release the fix in the next days