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.

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

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

Merged
merged 1 commit into from Apr 15, 2021

Conversation

mdumoulin
Copy link
Contributor

@mdumoulin mdumoulin commented Apr 9, 2021

Q A
Type bug
BC Break no
Fixed issues #4591

Summary

This PR fix a BC break on return type for :

  • Doctrine\DBAL\Connection::execute()
  • Doctrine\DBAL\Query\QueryBuilder::execute()

Previously, this methods expected to return a Doctrine\DBAL\Driver\Statement object as result.

Contents description

Situation in 2.12.x

  • Connection::executeCacheQuery() return a Driver\ResultStatement
  • Connection::executeQuery() return either a Driver\ResultStatement if cache is used, or a Driver\Statement if query is executed
  • QueryBuilder::execute() return either a Driver\Statement when the query is a select, or an integer for any other statement

Situation in 2.13.0 :

  • Connection::executeCacheQuery() return a Driver\ResultStatement & DBAL\Result
  • Connection::executeQuery() return a Driver\ResultStatement & DBAL\Result
  • QueryBuilder::execute() return a Driver\ResultStatement & DBAL\Result

Situation in this PR

Create 2 new interfaces ensuring forward compatibility :

  • ForwardCompatibility\DriverResultStatement extends Driver\ResultStatement and DBAL\ResultStatement
  • ForwardCompatibility\DriverStatement extends Driver\Statement and DBAL\Statement

Query execution result :

  • Connection::executeCacheQuery() return a ForwardCompatibility\Result wrapping Driver\ResultStatement
  • Connection::executeQuery() return a ForwardCompatibility\Result wrapping Driver\ResultStatement if cache is used, or a ForwardCompatibility\Result wrapping Driver\Statement if query is executed
  • QueryBuilder::execute() return either a a ForwardCompatibility\Result wrapping Driver\Statement when the query is a select, or an integer for any other statement

ForwardCompatibility/Result now implements Driver\Statement interface. If a Driver\Statement object is provided to constructor, Driver\Statement features will be availables, if a Driver\ResultStatement object is provided an exception will be thrown when trying to use Driver\Statement features.

@beberlei beberlei added this to the 2.13.1 milestone Apr 10, 2021
@beberlei
Copy link
Member

beberlei commented Apr 10, 2021

Its amazing that we can actually make this work, like a frankenstein of interfaces coming together for forward compatibility, thank you so much @mdumoulin for this amazing work :)

@beberlei
Copy link
Member

beberlei commented Apr 10, 2021

@mdumoulin what needs to be done for the remaining test failures?

@mdumoulin
Copy link
Contributor Author

mdumoulin commented Apr 11, 2021

Its amazing that we can actually make this work, like a frankenstein of interfaces coming together for forward compatibility, thank you so much @mdumoulin for this amazing work :)

Thanks, I'm glad if it can help to migrate on newer version of library.

lib/Doctrine/DBAL/ForwardCompatibility/Result.php Outdated Show resolved Hide resolved
return $this->stmt->bindValue($param, $value, $type);
}

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

@morozov morozov Apr 13, 2021

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

@mdumoulin mdumoulin Apr 13, 2021

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

@morozov morozov Apr 14, 2021

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

@mdumoulin mdumoulin Apr 14, 2021

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

@mdumoulin mdumoulin Apr 14, 2021

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

@morozov morozov Apr 14, 2021

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

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

@beberlei beberlei Apr 15, 2021

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.

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

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

beberlei
beberlei previously approved these changes Apr 15, 2021
Copy link
Member

@beberlei beberlei left a comment

Good work!

SenseException
SenseException previously approved these changes Apr 15, 2021
Following methods are returning Doctrine\DBAL\Driver\Statement as before :
- Doctrine\DBAL\Connection::executeQuery()
- Doctrine\DBAL\Query\QueryBuilder::execute()
@morozov morozov merged commit f4a801b into doctrine:2.13.x Apr 15, 2021
49 checks passed
@morozov
Copy link
Member

morozov commented Apr 15, 2021

Thanks, @mdumoulin!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants