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

corphi · 2018-04-04T22:37:52Z

The test case covers most of the sane PDO fetch modes that are not availably as DBAL fetch mode. Unfortunately, the fetch mode cannot be read by userland code. We have to trust that they are actually swapped for every method that uses them (well, they are); or use reflection to test the private method convertFetchMode().
closes #3082

Edit by @beberlei:

This deprecation was introduced as part of the removal of Doctrine DBAL API relying on implicit PDO behavior. This was started here #2958 and disallows the use of PDO::PARAM_* constants in Doctrine DBAL APIs.

Before:

$statement->bindParam(1, $value, PDO::PARAM_INT);

After:

$statement->bindParam(1, $value, \Doctrine\DBAL\Types\Types::INTEGER);

in addition PDO Fetch modes that are unsupported by Doctrine explictly are disallowed, for example PDO::FETCH_KEY_PAIR. Use the new fetch*() APIs instead. See #4019 for details.

morozov · 2018-04-04T23:42:00Z

@corphi thank you for the patch. Please replicate the same logic for convertParamType(). I don't know which of the PDO-specific parameter type users can use, but the same issue is still there.

As for the testing, could you turn your test into a functional one so that apart from verifying the deprecation message, it also makes sure that a PDO-specific mode still works. I will help us ensure that we don't break this functionality again.

UPD: at least, we need to check that the fetch mode is set as is. The deprecation notice is secondary.

Majkl578

Nicely handled. 👍

Majkl578 · 2018-04-05T00:05:25Z

lib/Doctrine/DBAL/Driver/PDOStatement.php

@@ -231,7 +234,13 @@ private function convertFetchMode(?int $fetchMode) : ?int
        }

        if (! isset(self::FETCH_MODE_MAP[$fetchMode])) {
-            throw new \InvalidArgumentException('Invalid fetch mode: ' . $fetchMode);
+            // next major: throw an exception


Can you prefix this with TODO: ...?

Majkl578 · 2018-04-05T00:07:36Z

lib/Doctrine/DBAL/Driver/PDOStatement.php

-            throw new \InvalidArgumentException('Invalid fetch mode: ' . $fetchMode);
+            // next major: throw an exception
+            @trigger_error(sprintf(
+                'Using a PDO fetch mode (%d given) is deprecated and will cause an error in Doctrine 3.0',


The message should also mention conbination of modes:

Using a PDO fetch mode or their combination (%d given) is deprecated and will not be supported in Doctrine 3.0'

morozov · 2018-04-06T15:33:23Z

@corphi are you planning on finishing this up or I could take over?

corphi · 2018-04-06T20:13:59Z

@morozov I addressed the minor points raised by @Majkl578, but can’t refactor the test until Thursday.

Concerning param types: I think these are a minor issue in comparison and beyond the scope of what I intended. I never used param types other than those supported by Doctrine: The native string ones are new, that basically leaves PDO::PARAM_INPUT_OUTPUT for use with stored procedures.

morozov · 2018-04-06T21:08:27Z

@morozov I addressed the minor points raised by @Majkl578, but can’t refactor the test until Thursday.

No worries. I can take care of it myself. If you don't mind, I'll update this pull request. Otherwise, I'll file a new one.

Concerning param types: I think these are a minor issue in comparison and beyond the scope of what I intended. I never used param types other than those supported by Doctrine: The native string ones are new, that basically leaves PDO::PARAM_INPUT_OUTPUT for use with stored procedures.

I see your point and I think the same regarding custom parameters. The problem is that if somebody even in theory uses them, it will be a breaking change for them same as this one was for you. I want to get it fixed even if it's not reported.

Other drivers (including the cache) never supported fetch modes that are not listed in `FetchMode`. Converting between the contants is only done for PDO, so BC only needs to be restored here. PDO will validate the fetch mode, we don’t have to.

Majkl578 · 2018-04-07T03:16:29Z

lib/Doctrine/DBAL/Driver/PDOStatement.php

-            throw new \InvalidArgumentException('Invalid parameter type: ' . $type);
+            // TODO: next major: throw an exception
+            @trigger_error(sprintf(
+                'Using a PDO parameter type (%d given) is deprecated and will cause an error in Doctrine 3.0',


Can we have a test for this one too? 🙏

I cannot come up with a reasonable example. Ideas?

PostgeSQL doesn't like PDO::PARAM_INPUT_OUTPUT (see https://travis-ci.org/doctrine/dbal/jobs/363396885).

Majkl578 · 2018-04-07T04:44:43Z

Thanks @corphi!

Backporting doctrine#3088 into 2.7

Majkl578 · 2018-04-07T04:54:09Z

Backported into 2.7 @ 2cc12a1

@corphi

This release fixes unintentional BC breaks: 1. It was impossible to use deprecated fetch modes with PDO-based drivers. 2. An unsupported option passed to the `Column` object prevented subsequent options from being applied. 3. Date interval values stored prior to upgrade to `v2.7.0` were reported as invalid. Total issues resolved: **10** **Backwards Compatibility Fixes:** - [3082: Custom PDO fetch modes and 2.7.0](doctrine#3082) thanks to @corphi - [3088: Fix doctrine#3082: Add BC for PDO-only fetch modes](doctrine#3088) thanks to @corphi - [3089: Don't skip column options.](doctrine#3089) thanks to @andytruong - [3093: When updating to version v2.7 type DateInterval throws errors](doctrine#3093) thanks to @fnagel - [3097: Fix compatibility for pre-2.7 DateIntervalType format](doctrine#3097) thanks to @Majkl578 **Documentation Improvements:** - [3083: Document the correct way of configuring a MariaDB database with serverVersion](doctrine#3083) thanks to @tristanbes - [3084: README: Add 2.7, drop 2.5](doctrine#3084) thanks to @Majkl578 **Continuous Integration Improvements:** - [3085: Tests: remove implicit verbose flag](doctrine#3085) thanks to @Majkl578 - [3090: Add symfony tests listener](doctrine#3090) thanks to @greg0ire - [3095: CI: Add missing listener for MariaDB @ mysqli](doctrine#3095) thanks to @Majkl578 # gpg: directory `/n/.gnupg' created # gpg: new configuration file `/n/.gnupg/gpg.conf' created # gpg: WARNING: options in `/n/.gnupg/gpg.conf' are not yet active during this run # gpg: DBG: locking for `/n/.gnupg/pubring.gpg.lock' done via O_EXCL # gpg: keyring `/n/.gnupg/pubring.gpg' created # gpg: Signature made Sun Apr 8 07:24:49 2018 using RSA key ID 543AE995 # gpg: Can't check signature: public key not found # Conflicts: # .gitignore # lib/Doctrine/DBAL/Driver/OCI8/Driver.php

corphi changed the base branch from master to 2.7 April 4, 2018 22:38

morozov changed the base branch from 2.7 to master April 5, 2018 00:06

Majkl578 reviewed Apr 5, 2018

View reviewed changes

Majkl578 added Regression Drivers PDO Compatibility Deprecation labels Apr 5, 2018

Majkl578 added this to the 2.7.1 milestone Apr 5, 2018

alongosz mentioned this pull request Apr 5, 2018

[Composer] Added conflict with doctrine/dbal:2.7.0 due to fetch_style bug ezsystems/ezpublish-kernel#2299

Merged

2 tasks

corphi force-pushed the fix-pdo-fetch-mode-bc branch 2 times, most recently from ee64650 to d131c0f Compare April 6, 2018 20:13

corphi force-pushed the fix-pdo-fetch-mode-bc branch from d131c0f to 5562c66 Compare April 6, 2018 20:34

Allow using PDO-only fetch modes for now

c6d8b41

Other drivers (including the cache) never supported fetch modes that are not listed in `FetchMode`. Converting between the contants is only done for PDO, so BC only needs to be restored here. PDO will validate the fetch mode, we don’t have to.

morozov force-pushed the fix-pdo-fetch-mode-bc branch 2 times, most recently from 78eda0a to 8b076d8 Compare April 7, 2018 02:35

[DBAL-3082] Updated convertParamType(), added a functional test

47755a7

morozov force-pushed the fix-pdo-fetch-mode-bc branch from 8b076d8 to 47755a7 Compare April 7, 2018 02:49

morozov requested a review from Majkl578 April 7, 2018 03:09

Majkl578 reviewed Apr 7, 2018

View reviewed changes

morozov force-pushed the fix-pdo-fetch-mode-bc branch 2 times, most recently from dbb3d1b to 47755a7 Compare April 7, 2018 03:56

morozov approved these changes Apr 7, 2018

View reviewed changes

Majkl578 approved these changes Apr 7, 2018

View reviewed changes

Majkl578 merged commit d4a96de into doctrine:master Apr 7, 2018

Majkl578 self-assigned this Apr 7, 2018

Majkl578 added a commit to Majkl578/doctrine-dbal that referenced this pull request Apr 7, 2018

Merge branch 'bpo/2.7/doctrine#3088' into 2.7

2cc12a1

Backporting doctrine#3088 into 2.7

Majkl578 added the BC Fix label Apr 7, 2018

corphi deleted the fix-pdo-fetch-mode-bc branch April 7, 2018 07:29

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 6, 2021

[doctrineGH-3088] Deprecate usage of PDO Fetch Mode and Param constants.

779cbd5

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 7, 2021

[doctrineGH-3088] Deprecate usage of PDO Fetch Mode and Param constants.

debd8cd

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 22, 2021

[doctrineGH-3088] Deprecate usage of PDO Fetch Mode and Param constants.

f33960f

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

corphi commented Apr 4, 2018 •

edited by beberlei

Loading

morozov commented Apr 4, 2018 •

edited

Loading

Majkl578 left a comment

Majkl578 Apr 5, 2018

Majkl578 Apr 5, 2018

morozov commented Apr 6, 2018

corphi commented Apr 6, 2018 •

edited

Loading

morozov commented Apr 6, 2018

Majkl578 Apr 7, 2018

morozov Apr 7, 2018

morozov Apr 7, 2018

Majkl578 commented Apr 7, 2018

Majkl578 commented Apr 7, 2018 •

edited

Loading

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

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

Conversation

corphi commented Apr 4, 2018 • edited by beberlei Loading

morozov commented Apr 4, 2018 • edited Loading

Majkl578 left a comment

Choose a reason for hiding this comment

Majkl578 Apr 5, 2018

Choose a reason for hiding this comment

Majkl578 Apr 5, 2018

Choose a reason for hiding this comment

morozov commented Apr 6, 2018

corphi commented Apr 6, 2018 • edited Loading

morozov commented Apr 6, 2018

Majkl578 Apr 7, 2018

Choose a reason for hiding this comment

morozov Apr 7, 2018

Choose a reason for hiding this comment

morozov Apr 7, 2018

Choose a reason for hiding this comment

Majkl578 commented Apr 7, 2018

Majkl578 commented Apr 7, 2018 • edited Loading

corphi commented Apr 4, 2018 •

edited by beberlei

Loading

morozov commented Apr 4, 2018 •

edited

Loading

corphi commented Apr 6, 2018 •

edited

Loading

Majkl578 commented Apr 7, 2018 •

edited

Loading