Remove hard dependency on PDO #2958

morozov · 2018-01-01T06:44:39Z

The first part of the pull request (prior to the commits labeled [BC]) contains the non-breaking changes which should be back-ported to master. The remaining part contains the removal of the dependency and breaking API changes.

Fixes: #2953.

Ocramius

Overall, I understand why this is being attempted, but I don't think users will benefit much from it, as most of our users do in fact use PDO, so the rest shouldn't have a problem with keeping it installed.

What I see though, is using bitmasks to define bound parameter types, which is problematic when we consider things like multi-dimensional parameters. This doesn't affect low-level API, but it is one of the pain-points of our API (binding with Connection::PARAM_INT_ARRAY, for example).

I'm not sure if that's relevant to this patch though.

Ocramius · 2018-01-01T12:49:05Z

UPGRADE.md

@@ -1,3 +1,9 @@
+# Upgrade to NEXT


Should we open develop?

Let's open develop, get everything merged there and then back-port the relevant pieces to master.

Develop already exists here, it just needs rebase.

Ocramius · 2018-01-01T12:50:07Z

lib/Doctrine/DBAL/Cache/ResultCacheStatement.php

@@ -147,30 +146,38 @@ public function getIterator()
 /**
 * {@inheritdoc}
 */
- public function fetch($fetchMode = null, $cursorOrientation = \PDO::FETCH_ORI_NEXT, $cursorOffset = 0)
+ public function fetch($fetchMode = null, $_ = null, $__ = null)


Should we deprecate those parameters?

We should. This particular commit should only go to develop. Initially, I wanted to get rid of them entirely but here's the trick:

DBAL\PDOStatement extends PDOStatement and implements ResultStatement, both of which declare fetch().

It looks impossible to use a custom PDO statement class which doesn't extend PDOStatement since PDO instantiates it without calling constructor (this is my current understanding, I may be wrong).

Therefore, the declaration of ResultStatement::fetch() and all its implementors should be compatible with PDOStatement::fetch().

I'd love to be able to rework DBAL\PDOStatement extends PDOStatement into a composition and get rid of those parameters entirely but it's technically challenging. Any ideas?

Ocramius · 2018-01-01T12:50:37Z

lib/Doctrine/DBAL/Connection.php

@@ -75,14 +75,14 @@ class Connection implements DriverConnection
 *
 * @var integer
 */
- const PARAM_INT_ARRAY = 101;
+ const PARAM_INT_ARRAY = Statement::PARAM_INT + self::ARRAY_PARAM_OFFSET;


Is + intentional? Shouldn't this be |?

Ah, it is an offset... didn't see that.

Ocramius · 2018-01-01T12:50:43Z

lib/Doctrine/DBAL/Connection.php


 /**
 * Represents an array of strings to be expanded by Doctrine SQL parsing.
 *
 * @var integer
 */
- const PARAM_STR_ARRAY = 102;
+ const PARAM_STR_ARRAY = Statement::PARAM_STR + self::ARRAY_PARAM_OFFSET;


Is + intentional? Shouldn't this be |?

Ah, it is an offset... didn't see that.

Ocramius · 2018-01-01T13:01:05Z

lib/Doctrine/DBAL/Driver/Statement.php

@@ -19,6 +19,8 @@

 namespace Doctrine\DBAL\Driver;

+use PDO;


Probably to be removed

Ocramius · 2018-01-01T13:02:08Z

lib/Doctrine/DBAL/Portability/Connection.php

@@ -20,6 +20,7 @@
 namespace Doctrine\DBAL\Portability;

 use Doctrine\DBAL\Cache\QueryCacheProfile;
+use PDO;


To be removed, I'd say. Use the FQCN in the docblock

Ocramius · 2018-01-01T13:03:22Z

lib/Doctrine/DBAL/Driver/PDOStatement.php

@@ -27,6 +29,29 @@
 */
 class PDOStatement extends \PDOStatement implements Statement


In this case we keep the dependency?

Why? This class will be only loaded if a PDO driver is used in runtime. Currently, all tests pass w/o the PDO extension loaded.

morozov · 2018-01-01T18:55:15Z

@Ocramius added this to the 2.7.0 milestone

This pull request is meant to be split in two parts:

All commits prior to the ones marked as [BC] go to master and the next minor release.
The rest goes to develop.

Or the other way around: everything goes to develop, then non-breaking changes get ported to master. The second approach should be better maintainable IMO.

morozov · 2018-01-01T18:58:23Z

oci runtime error: exec failed: container_linux.go:265: starting container process caused "could not create session key: disk quota exceeded"

Anyone, have you seen these errors before? Looks irrelevant.

Majkl578 · 2018-01-01T19:00:10Z

Travis is messed up today, other build failed due to MySQL not starting...

morozov · 2018-01-01T19:04:14Z

[…] but I don't think users will benefit much from it, as most of our users do in fact use PDO, so the rest shouldn't have a problem with keeping it installed.

@Ocramius the hard dependency hardens the adoption of the DBAL by existing projects. Particularly, SugarCRM is migrating its DB layer to DBAL. It supports several DB platforms but doesn't use or support PDO. Currently, it depends on v2.5 and uses a PDO shim to avoid requiring the extension.

The absence of a dependency on PDO makes the upgrade from a non-DBAL-based version to the DBAL-based seamless. Otherwise, the customers will be required to have PDO installed for no reason.

morozov · 2018-01-01T19:10:30Z

What I see though, is using bitmasks to define bound parameter types, which is problematic when we consider things like multi-dimensional parameters. This doesn't affect low-level API, but it is one of the pain-points of our API (binding with Connection::PARAM_INT_ARRAY, for example).

Could you elaborate? I was thinking about replacing Connection::PARAM_INT_ARRAY with Statement::PARAM_INT | Statement::PARAM_ARRAY. Otherwise, we have related constants in different classes. But this is outside of the scope. We can do that in 3.0 before or after getting this pull request merged.

Ocramius · 2018-01-01T19:18:22Z

@morozov ACK on adoption.

As for the PARAM_INT_ARRAY question, that was indeed feature creep: let's leave it out.

morozov · 2018-01-04T03:56:12Z

@Ocramius please take a look. Instead of removing additional arguments from the fetch() and fetchAll() methods I replaced them with ...$args (see #2527 (comment)). Therefore, there's no need to deprecate them. Callers will be able to keep passing them as they did but implementors will have to change their signatures accordingly.

Ocramius · 2018-01-04T07:17:26Z

@morozov for 3.0, wouldn't it be better for us to take ownership of the fetch and fetchAll signatures?

morozov · 2018-01-04T07:22:07Z

We could. I just don’t have an idea of a better set of signatures. We still need to be able to take variable set of arguments depending on the mode. Do you have any concrete idea?

Ocramius · 2018-01-04T07:24:41Z

Nope, no better ideas so far :-\

Ocramius · 2018-01-04T07:25:17Z

.travis.yml

@@ -306,6 +306,11 @@ jobs:
 if: type = pull_request
 php: 7.2
 script:
+ - |


Should be gone after a rebase, no?

I’ll remove it manually if it doesn’t. I tested the fix here. Gonna rebase develop and this branch tomorrow.

Majkl578 · 2018-01-25T22:08:23Z

I'm really happy we decided to stick with the FetchMode and ParameterType classes with unprefixed/unsuffixed constant names, it really makes more sense this way. 🎉

…PDOStatement

…ement::bindParam()

…ion to avoid having to replicate the \PDOStatement interface in ResultStatement

Ocramius suggested changes Jan 1, 2018

View reviewed changes

Ocramius added Compatibility Improvement In Discussion PDO Prepared Statements labels Jan 1, 2018

Ocramius added this to the 2.7.0 milestone Jan 1, 2018

Ocramius assigned morozov Jan 1, 2018

morozov added the BC Break label Jan 2, 2018

morozov force-pushed the issues/2953 branch 2 times, most recently from b048689 to 965353c Compare January 3, 2018 00:03

morozov changed the base branch from master to develop January 3, 2018 07:07

morozov force-pushed the issues/2953 branch 5 times, most recently from b92f436 to cc7a6ce Compare January 4, 2018 02:54

Ocramius reviewed Jan 4, 2018

View reviewed changes

morozov force-pushed the develop branch from 12fb8e1 to a14b808 Compare January 4, 2018 18:47

morozov force-pushed the issues/2953 branch from cc7a6ce to 7a7411b Compare January 4, 2018 18:49

morozov deleted the issues/2953 branch January 25, 2018 23:04

morozov mentioned this pull request Jan 25, 2018

Forward compatibility with 3.x #2996

Merged

Majkl578 mentioned this pull request Jan 27, 2018

Extract some constants into TransactionIsolationLevel, TrimMode and DateIntervalUnit #2998

Closed

Majkl578 mentioned this pull request Feb 14, 2018

Fix compatibility with DBAL develop doctrine/orm#7065

Merged

This was referenced Feb 22, 2018

adapted some mocks to php 7.2 doctrine/orm#7087

Closed

Drop explicit PDO dependency doctrine/orm#7088

Merged

javiereguiluz mentioned this pull request Mar 6, 2018

Update docs for latest Doctrine removals symfony/symfony-docs#9401

Closed

8 tasks

morozov mentioned this pull request Mar 14, 2018

Memory leak with PDOConnection->prepare() #3047

Closed

This was referenced Mar 22, 2018

Missing parameter fetchArgument for fetch and fetchAll methods #3066

Closed

Dropped support of fetching objects and non-zero column #3070

Merged

morozov modified the milestones: 2.7.0, 3.0.0 Apr 4, 2018

Majkl578 mentioned this pull request Apr 18, 2018

PDO::FETCH_KEY_PAIR Missing from FetchMode Constants #3111

Closed

morozov mentioned this pull request Aug 31, 2018

Add .doctrine-project.json to root of the project. #3275

Merged

morozov mentioned this pull request Jan 10, 2019

pdo_mysql with persistent connection #2315

Closed

morozov restored the issues/2953 branch December 30, 2019 01:11

morozov mentioned this pull request Dec 30, 2019

Research the possibility to backport PDO-related changes from master to 2.10 #3798

Closed

1 task

morozov added a commit to morozov/dbal that referenced this pull request Dec 30, 2019

[doctrineGH-2958] Added conversion of DBAL constants to PDO ones for …

fc2814b

…PDOStatement

morozov added a commit to morozov/dbal that referenced this pull request Dec 30, 2019

[doctrineGH-2958] Removed support for PDO::PARAM_INPUT_OUTPUT in Stat…

0edb3d1

…ement::bindParam()

morozov added a commit to morozov/dbal that referenced this pull request Dec 30, 2019

[doctrineGH-2958] Replaced extension of \PDOStatement with a composit…

31e72b6

…ion to avoid having to replicate the \PDOStatement interface in ResultStatement

morozov added a commit to morozov/dbal that referenced this pull request Dec 30, 2019

[doctrineGH-2958] Replaced PDO constants with values

4e8f3b6

morozov added a commit to morozov/dbal that referenced this pull request Dec 30, 2019

[doctrineGH-2958] Updated UPGRADE.md

9a7fc25

morozov mentioned this pull request Dec 30, 2019

Backport PDO-related changes from master to 3.0.x #3803

Merged

8 tasks

morozov deleted the issues/2953 branch January 17, 2020 00:58

morozov modified the milestones: 4.0.0, 3.0.0 Aug 15, 2020

morozov mentioned this pull request Oct 22, 2020

Manually merge 2.12.x into 3.0.x #4381

Merged

beberlei mentioned this pull request Mar 6, 2021

Fix #3082: Add BC for PDO-only fetch modes #3088

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove hard dependency on PDO #2958

Remove hard dependency on PDO #2958

morozov commented Jan 1, 2018 •

edited

Loading

Ocramius left a comment

Ocramius Jan 1, 2018

morozov Jan 2, 2018

Majkl578 Jan 2, 2018

Ocramius Jan 1, 2018

morozov Jan 1, 2018

Ocramius Jan 1, 2018

Ocramius Jan 1, 2018

Ocramius Jan 1, 2018

Ocramius Jan 1, 2018

Ocramius Jan 1, 2018

Ocramius Jan 1, 2018

Ocramius Jan 1, 2018

morozov Jan 1, 2018

morozov commented Jan 1, 2018

morozov commented Jan 1, 2018

Majkl578 commented Jan 1, 2018

morozov commented Jan 1, 2018 •

edited

Loading

morozov commented Jan 1, 2018 •

edited

Loading

Ocramius commented Jan 1, 2018

morozov commented Jan 4, 2018

Ocramius commented Jan 4, 2018

morozov commented Jan 4, 2018

Ocramius commented Jan 4, 2018

Ocramius Jan 4, 2018

morozov Jan 4, 2018

Majkl578 commented Jan 25, 2018

		@@ -27,6 +29,29 @@
		*/
		class PDOStatement extends \PDOStatement implements Statement

Remove hard dependency on PDO #2958

Remove hard dependency on PDO #2958

Conversation

morozov commented Jan 1, 2018 • edited Loading

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov commented Jan 1, 2018

morozov commented Jan 1, 2018

Majkl578 commented Jan 1, 2018

morozov commented Jan 1, 2018 • edited Loading

morozov commented Jan 1, 2018 • edited Loading

Ocramius commented Jan 1, 2018

morozov commented Jan 4, 2018

Ocramius commented Jan 4, 2018

morozov commented Jan 4, 2018

Ocramius commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Majkl578 commented Jan 25, 2018

morozov commented Jan 1, 2018 •

edited

Loading

morozov commented Jan 1, 2018 •

edited

Loading

morozov commented Jan 1, 2018 •

edited

Loading