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

Remove Abstraction\Result #4294

Merged
merged 1 commit into from
Sep 26, 2020
Merged

Conversation

morozov
Copy link
Member

@morozov morozov commented Sep 24, 2020

Q A
Type improvement
BC Break yes

The interface is proposed for deprecation in #4291.

@@ -5,7 +5,8 @@
use Doctrine\Common\Cache\Cache;
use Doctrine\DBAL\Driver\Exception;
use Doctrine\DBAL\Driver\FetchUtils;
use Doctrine\DBAL\Driver\Result;
use Doctrine\DBAL\Driver\Result as DriverResult;
use Doctrine\DBAL\Result;
Copy link
Member

Choose a reason for hiding this comment

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

Is having a Doctrine\DBAL\Result in $result on purpose instead of a Driver\Result and planned for 3.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The caching layer only works with the wrapper-level result. Prior to this change, the wrapper result would implement the driver result interface, so this signature would work. As of this change, it's no longer true.

Additional references:

  1. The same is true for the wrapper Connection and Statement classes as of Do not implement driver-level interfaces by wrapper-level classes #4159.
  2. We may rework the caching layer into a driver-level middleware where it belongs but it's not a top priority for me. See Rework the portability layer to act as a middleware #4157, there's basically a similar issue here.

SenseException
SenseException previously approved these changes Sep 25, 2020
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

If the answer to my previous question is "yes", this then this is ready for merge IMO.

@morozov morozov marked this pull request as ready for review September 26, 2020 16:20
@morozov morozov force-pushed the remove-abstraction-result branch 2 times, most recently from 5b38026 to 4f57b1d Compare September 26, 2020 16:53
@morozov morozov force-pushed the remove-abstraction-result branch from 4f57b1d to 496bd71 Compare September 26, 2020 16:58
@morozov morozov requested a review from greg0ire September 26, 2020 17:16
@morozov morozov merged commit 6b57e8b into doctrine:3.0.x Sep 26, 2020
@morozov morozov deleted the remove-abstraction-result branch September 26, 2020 17:41
@morozov morozov added this to the 3.0.0 milestone Sep 26, 2020
@morozov morozov self-assigned this Sep 26, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 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.

3 participants