Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix BC break on QueryBuilder::execute() #4596

Merged
merged 1 commit into from Apr 15, 2021
Merged

Fix BC break on QueryBuilder::execute() #4596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 6 additions & 9 deletions lib/Doctrine/DBAL/Connection.php
View file
Open in desktop
Expand Up @@ -18,7 +18,6 @@
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Query\Expression\ExpressionBuilder;
use Doctrine\DBAL\Query\QueryBuilder;
use Doctrine\DBAL\Result as BaseResult;
use Doctrine\DBAL\Schema\AbstractSchemaManager;
use Doctrine\DBAL\Types\Type;
use Doctrine\Deprecations\Deprecation;
Expand Down Expand Up @@ -1264,7 +1263,9 @@ public function prepare($sql)
* @param array<int, mixed>|array<string, mixed> $params Query parameters
* @param array<int, int|string|Type|null>|array<string, int|string|Type|null> $types Parameter types
*
* @return ResultStatement&BaseResult The executed statement.
* @return ForwardCompatibility\DriverStatement|ForwardCompatibility\DriverResultStatement
*
* The executed statement or the cached result statement if a query cache profile is used
*
* @throws Exception
*/
Expand Down Expand Up @@ -1320,7 +1321,7 @@ public function executeQuery($sql, array $params = [], $types = [], ?QueryCacheP
* @param array<int, mixed>|array<string, mixed> $params Query parameters
* @param array<int, int|string|Type|null>|array<string, int|string|Type|null> $types Parameter types
*
* @return ResultStatement&BaseResult
* @return ForwardCompatibility\DriverResultStatement
*
* @throws CacheException
*/
Expand Down Expand Up @@ -1365,15 +1366,11 @@ public function executeCacheQuery($sql, $params, $types, QueryCacheProfile $qcp)
}

/**
* @return ResultStatement&BaseResult
* @return ForwardCompatibility\Result
*/
private function ensureForwardCompatibilityStatement(ResultStatement $stmt)
{
if ($stmt instanceof BaseResult) {
return $stmt;
}

return new ForwardCompatibility\Result($stmt);
return ForwardCompatibility\Result::ensure($stmt);
}

/**
Expand Down
9 changes: 9 additions & 0 deletions lib/Doctrine/DBAL/ForwardCompatibility/DriverResultStatement.php
View file
Open in desktop
@@ -0,0 +1,9 @@
<?php

namespace Doctrine\DBAL\ForwardCompatibility;

use Doctrine\DBAL;

interface DriverResultStatement extends DBAL\Driver\ResultStatement, DBAL\Result
{
}
9 changes: 9 additions & 0 deletions lib/Doctrine/DBAL/ForwardCompatibility/DriverStatement.php
View file
Open in desktop
@@ -0,0 +1,9 @@
<?php

namespace Doctrine\DBAL\ForwardCompatibility;

use Doctrine\DBAL;

interface DriverStatement extends DBAL\Driver\Statement, DBAL\Result
{
}
121 changes: 115 additions & 6 deletions lib/Doctrine/DBAL/ForwardCompatibility/Result.php
View file
Open in desktop
Expand Up @@ -2,10 +2,10 @@

namespace Doctrine\DBAL\ForwardCompatibility;

use Doctrine\DBAL\Driver\ResultStatement as DriverResultStatement;
use Doctrine\DBAL\Driver;
use Doctrine\DBAL\Exception;
use Doctrine\DBAL\Exception\NoKeyValue;
use Doctrine\DBAL\Result as BaseResult;
use Doctrine\DBAL\ParameterType;
use Doctrine\Deprecations\Deprecation;
use IteratorAggregate;
use PDO;
Expand All @@ -18,18 +18,27 @@
* A wrapper around a Doctrine\DBAL\Driver\ResultStatement that adds 3.0 features
* defined in Result interface
*/
class Result implements IteratorAggregate, DriverResultStatement, BaseResult
class Result implements IteratorAggregate, DriverStatement, DriverResultStatement
{
/** @var DriverResultStatement */
/** @var Driver\ResultStatement */
private $stmt;

public function __construct(DriverResultStatement $stmt)
public static function ensure(Driver\ResultStatement $stmt): Result
{
if ($stmt instanceof Result) {
return $stmt;
}

return new Result($stmt);
}

public function __construct(Driver\ResultStatement $stmt)
{
$this->stmt = $stmt;
}

/**
* @return DriverResultStatement
* @return Driver\ResultStatement
*/
public function getIterator()
{
Expand Down Expand Up @@ -301,4 +310,104 @@ private function ensureHasKeyValue(): void
throw NoKeyValue::fromColumnCount($columnCount);
}
}

/**
* {@inheritDoc}
*
* @deprecated This feature will no longer be available on Result object in 3.0.x version.
*/
public function bindValue($param, $value, $type = ParameterType::STRING)
{
Deprecation::triggerIfCalledFromOutside(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/4019',
'Result::bindValue() is deprecated, no replacement.'
);

if ($this->stmt instanceof Driver\Statement) {
return $this->stmt->bindValue($param, $value, $type);
}

throw Exception::notSupported('bindValue');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting a bit lost: the forward-compatible result implements the statement interface and can act as a statement depending on the wrapped implementation. @beberlei could you review this please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 2.12.x version :

  • Doctrine\DBAL\Connection::executeCacheQuery() returned a Driver\ResultStatement object
  • Doctrine\DBAL\Connection::executeQuery() returned a Driver\Statement object, except when using cache where a Driver\ResultStatement is returned instead

The solution makes both return a result implementing Driver\Statement, but Statement feature will only be available when using Doctrine\DBAL\Connection::executeQuery().

We can be more specific in methods @return documentation by adding following interfaces :

  • ForwardCompatibility\DriverResultStatement implementing Driver\ResultStatement&DBAL\Result for executeCacheQuery()
  • ForwardCompatibility\DriverStatement implementing Driver\Statement&DBAL\Result for executeQuery()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation. Makes sense. I think it's enough to leave the return type documented via an annotation with the intersection type, no need to introduce more interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find a way to fix documentation in QueryBuilder without adding an interface : https://github.com/doctrine/dbal/pull/4596/files#diff-829b37fd7ed65c7ead12002f3e689f3a672986dd88fd0f5d4d5f92321a61211dL204

Without interface, we will have to write something like :

    /**
     * Executes this query using the bound parameters and their types.
     *
     * @return (ResultStatement&Doctrine\DBAL\Result)|int
     *
     * @throws Exception
     */

I test it with phpstan but it does not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think we should to be more accurate on Doctrine\DBAL\Connection::executeQuery() with something like :

    /**
     * ...
     * @return (Driver\ResultStatement&Doctrine\DBAL\Result)|(Driver\Statement&Doctrine\DBAL\Result) Return the executed result statement or cached statement if a cache profile is used
     *
     * @throws Exception
     */

Same issue here, I don't think phpstan can interpret complex type like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Then the interfaces are necessary. The question is, is there a way to test the newly introduced types? Currently, the build passes but there are changes needed to satisfy the static analysis on the consumer side. That means that we're not analyzing all the cases ourselves. E.g. it might help to enable phpstan for some of the test cases that describe the consumers of the old API and make sure that such analysis passes.

Copy link
Contributor Author

@mdumoulin mdumoulin Apr 14, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the meaning of tests added here : https://github.com/doctrine/dbal/pull/4596/files#diff-1056c8f8b7e1dbbb024efeed5bca59a3e9097603fafb63d0c7c7924531b2d63eR363-R381

But I still have to find a way to test executeQuery() with params.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct, it exposes the bind* api from the result, but since it is still acting as a statement in DBAL 2 this is correct and the deprecations trigger correctly. I believe we can roll with this.

}

/**
* {@inheritDoc}
*
* @deprecated This feature will no longer be available on Result object in 3.0.x version.
*/
public function bindParam($param, &$variable, $type = ParameterType::STRING, $length = null)
{
Deprecation::triggerIfCalledFromOutside(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/4019',
'Result::bindParam() is deprecated, no replacement.'
);

if ($this->stmt instanceof Driver\Statement) {
return $this->stmt->bindParam($param, $variable, $type, $length);
}

throw Exception::notSupported('bindParam');
}

/**
* {@inheritDoc}
*
* @deprecated The error information is available via exceptions.
*/
public function errorCode()
{
Deprecation::triggerIfCalledFromOutside(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/4019',
'Result::errorCode() is deprecated, the error information is available via exceptions.'
);

if ($this->stmt instanceof Driver\Statement) {
return $this->stmt->errorCode();
}

throw Exception::notSupported('errorCode');
}

/**
* {@inheritDoc}
*
* @deprecated The error information is available via exceptions.
*/
public function errorInfo()
{
Deprecation::triggerIfCalledFromOutside(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/4019',
'Result::errorInfo() is deprecated, the error information is available via exceptions.'
);

if ($this->stmt instanceof Driver\Statement) {
return $this->stmt->errorInfo();
}

throw Exception::notSupported('errorInfo');
}

/**
* {@inheritDoc}
*
* @deprecated This feature will no longer be available on Result object in 3.0.x version.
*/
public function execute($params = null)
{
Deprecation::triggerIfCalledFromOutside(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/4019',
'Result::execute() is deprecated, no replacement.'
);

if ($this->stmt instanceof Driver\Statement) {
return $this->stmt->execute($params);
}

throw Exception::notSupported('execute');
}
}
8 changes: 5 additions & 3 deletions lib/Doctrine/DBAL/Query/QueryBuilder.php
View file
Open in desktop
Expand Up @@ -3,8 +3,8 @@
namespace Doctrine\DBAL\Query;

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Driver\ResultStatement;
use Doctrine\DBAL\Exception;
use Doctrine\DBAL\ForwardCompatibility;
use Doctrine\DBAL\ParameterType;
use Doctrine\DBAL\Query\Expression\CompositeExpression;
use Doctrine\DBAL\Query\Expression\ExpressionBuilder;
Expand Down Expand Up @@ -201,14 +201,16 @@ public function getState()
/**
* Executes this query using the bound parameters and their types.
*
* @return ResultStatement|int
* @return ForwardCompatibility\DriverStatement|int
*
* @throws Exception
*/
public function execute()
{
if ($this->type === self::SELECT) {
return $this->connection->executeQuery($this->getSQL(), $this->params, $this->paramTypes);
return ForwardCompatibility\Result::ensure(
$this->connection->executeQuery($this->getSQL(), $this->params, $this->paramTypes)
);
Copy link
Contributor Author

@mdumoulin mdumoulin Apr 14, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensuring compatible result here has 2 advantages :

  • We can be more accurate on return type documentation, otherwise we will have to document ForwardCompatibility\DriverStatement|ForwardCompatibility\DriverResultStatement|int
  • If user has overridden Connection::executeQuery() the forward compatibility will be ensured

}

return $this->connection->executeStatement($this->getSQL(), $this->params, $this->paramTypes);
Expand Down