Forward compatibility with 3.x #2996

morozov · 2018-01-25T23:10:02Z

This is a back-port of non-BC-breaking part of #2958.

Fixes #2953.

Majkl578

Nice cleanup. 👍
Travis caught some CS issues on modified lines that are not directly caused by this PR.

docs/en/reference/portability.rst

lib/Doctrine/DBAL/Cache/ArrayStatement.php

Majkl578 · 2018-01-25T23:38:23Z

lib/Doctrine/DBAL/ColumnCase.php

+/**
+ * Contains portable column case conversions.
+ */
+class ColumnCase


Can we make these final, maybe?

Yeah. I took it as is from your draft and though final private function __construct() is enough. But it only prohibits instantiation, not extension.

Majkl578 · 2018-01-25T23:39:46Z

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php

-        \PDO::PARAM_INT => DB2_LONG,
-        \PDO::PARAM_STR => DB2_CHAR,
+        ParameterType::INTEGER => DB2_LONG,
+        ParameterType::STRING => DB2_CHAR,


Do we align array members the same way as assignments? 🤔

lib/Doctrine/DBAL/Cache/ArrayStatement.php

Majkl578 · 2018-01-25T23:43:52Z

lib/Doctrine/DBAL/Driver/Statement.php

     *
     * @return boolean TRUE on success or FALSE on failure.
     */
-    function bindValue($param, $value, $type = null);
-
+    public function bindValue($param, $value, $type = ParameterType::STRING);


Isn't change of default here from null to string BC breaking? (Applies to other signature changes as well.)

PHP is fine with that judging by https://3v4l.org/95JYL. Semantically, both null and ParameterType::STRING are the same since the latter is used by default in all implementations.

I'd be cautious with that. Does the test suite at least contain a test with bindValue() called with only 2 parameters?

@Ocramius there is a test like that:

dbal/tests/Doctrine/Tests/DBAL/Functional/StatementTest.php

Line 180 in bc23831

$stmt->bindValue(1, 1);

This change indeed was only required for develop to make this code work:

dbal/lib/Doctrine/DBAL/Driver/PDOStatement.php

Line 73 in 3fd9239

return $this->stmt->bindValue($param, $value, $type);

because NULL appears to not be a valid type for PDO:

$conn = new PDO('mysql:host=localhost;dbname=mysql'); $stmt = $conn->prepare('SELECT ?'); $stmt->bindValue(1, 'foo', null); $stmt->execute(); $value = $stmt->fetchColumn(); var_dump($value); // NULL

I understand your concern but I don't know what to look for. I manually checked all implementations, and they all either default to binding as string explicitly (IBM DB2, MySQLi, PDO) or don't specify the string/null type at all (OCI8, SQL Server) or seem to not even accept NULL (SQLAnywhere).

I can revert changes in all Statement::bind*() signatures if needed.

I'm fine if a test already exists and doesn't explicitly pass the third argument to bindValue().

My only issue was that without a test verifying the default parameter we'd have a stability problem :-)

And yes, PDO is (as usual) being weird with default values :-P

morozov · 2018-01-28T18:56:17Z

@Majkl578 looks like some of the code style violations are reported on wrong lines. For instance:

FILE: lib/Doctrine/DBAL/Cache/ArrayStatement.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  45 | ERROR | [x] Expected "int" but found "integer" in @var
     |       |     annotation.

The @var integer annotation is on the line 43 which is not affected by this pull request and therefore shouldn't be reported. It should be fixed on the code standard side. It should report the annotation itself, not the return value type. All other false positives are caused by the same reason.

Majkl578 · 2018-01-28T19:11:04Z

I noticed that too while working on #2998. The errors seem to be reported for the element they're connected to. See here - $pointer is the position of matched element (either T_VARIABLE or T_FUNCTION. Not sure if there is any reasoning behind this, normally it's not a problem, but here with the line-based filter it is...

morozov · 2018-01-28T19:29:59Z

@Majkl578 I think we can live with that. I suspect it may be hard to justify the needed changes in a 3rd-party library (not speaking of that someone will have to implement them). On the other hand, we could get rid of the false positives by fixing annotations.

Maybe @carusogabriel could take a look? 😉

carusogabriel · 2018-01-28T19:33:01Z

@morozov How can I help? 🤓

Majkl578 · 2018-01-28T19:37:41Z

@morozov Reported here: slevomat/coding-standard#250. It should be trivial to fix for LongTypeHInt sniff, but not sure about TypeHintDeclaration sniff which is pretty complex already.

I hope 2.7 will be shipped anytime soon and then we can merge develop & apply CS same as we did in ORM. 😎

kukulich · 2018-01-28T21:15:33Z

@morozov

It should be fixed on the code standard side.

Try composer update. It will work now, I hope :)

Majkl578 · 2018-01-28T21:24:11Z

@kukulich Awesome, thanks!

@morozov Retriggerred the CS stage: https://travis-ci.org/doctrine/dbal/jobs/334442418#L524, can you check now?

morozov · 2018-01-29T01:37:23Z

@carusogabriel judging by CI failures, we have some @param and @return method annotations which define type of value as integer or boolean. The code style requires them to be represented as int and bool accordingly.

morozov · 2018-01-29T01:47:26Z

@Majkl578, @kukulich thank you for the update. There's one more false positive like the fixed ones:

FILE: lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvStatement.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 101 | ERROR | Property
     |       | \Doctrine\DBAL\Driver\SQLSrv\SQLSrvStatement::$defaultFetchMode
     |       | does not have @var annotation.
----------------------------------------------------------------------

Unlike those, it doesn't seem possible to fix on the standard side since it reports the absence of annotation. Unless you have an idea.

@carusogabriel, mind fixing issues like this as well?

carusogabriel · 2018-01-29T01:49:12Z

@morozov No problem. I'm just finishing some stuff, I'll PR separately the annotation types and missing @var!

morozov · 2018-01-29T04:26:12Z

One more falses-positive:

FILE: tests/Doctrine/Tests/DBAL/Functional/PortabilityTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 137 | ERROR | [x] Method
     |       |     \Doctrine\Tests\DBAL\Functional\PortabilityTest::testPortabilityPdoSqlServer()
     |       |     does not need documentation comment.

The standard doesn't account for PHPUnit annotations.

Majkl578 · 2018-01-29T04:54:31Z

This last one is ours, configuration issue - we do have most of the common annotations whitelisted, but I probably forgot this one (@requires).

Ocramius · 2018-01-29T07:02:50Z

Argh, the CS changes introduced recently led to a few merge conflicts here, @morozov. Sorry!

Ocramius

Overall good improvement. Besides a change in a test that isn't really clear to me, this is good to go 👍

Ocramius · 2018-01-29T07:09:04Z

tests/Doctrine/Tests/DBAL/DriverManagerTest.php

@@ -217,7 +240,7 @@ public function databaseUrls()
                false,
            ),
            'URL without scheme but default PDO driver' => array(
-                array('url' => '//foo:bar@localhost/baz', 'pdo' => $pdoMock),
+                array('url' => '//foo:bar@localhost/baz', 'pdo' => true),


This is a test change that I don't understand.

@Ocramius look at the changes in testDatabaseUrl(). Instead of creating mock, the data provider passes a flag to the test method. It will either create the mock if PDO is loaded or will skip the data set.

Got it 👍

…dard

…ndency on the actual constant values

morozov · 2018-01-29T22:22:14Z

I believe we can ignore the code style CI failure for this PR. My entire idea of line-based validation was to avoid the "Fixed code style" commits. But given that this one is a back-port from develop, we cannot amend the ported commits because it will introduce further conflicts during rebase of develop.

morozov · 2018-02-01T05:34:55Z

@Ocramius anything else you want to get done before merging this?

Ocramius · 2018-02-01T07:18:36Z

Nope, 🚢

Ocramius · 2018-02-06T11:29:46Z

Handled further CS in #3002

Majkl578 · 2018-02-16T23:18:34Z

lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvStatement.php

@@ -20,7 +20,8 @@
 namespace Doctrine\DBAL\Driver\SQLSrv;

 use Doctrine\DBAL\Driver\StatementIterator;
-use PDO;


This is bug - there is still unqualified PDO::FETCH_ORI_NEXT reference.

Looks like the only place like this. Please see #3024.

Majkl578 suggested changes Jan 25, 2018

View reviewed changes

Majkl578 mentioned this pull request Jan 28, 2018

Annotation errors reported against wrong token/line slevomat/coding-standard#250

Closed

This was referenced Jan 29, 2018

CS: correct annotation types to use symbols specified in the CS rules #2999

Merged

Use short array syntax #3000

Closed

Use single quotes instead of double #3001

Closed

Use @var instead of @param #3002

Merged

Ocramius added Improvement QueryBuilder Prepared Statements Compatibility labels Jan 29, 2018

Ocramius assigned morozov Jan 29, 2018

Ocramius suggested changes Jan 29, 2018

View reviewed changes

morozov added 5 commits January 29, 2018 12:19

Added visibility for interface methods as required by the coding stan…

5e2b7f8

…dard

Removed HydratorMockStatement since it's not used anywhere since 2.0

99356c2

Introduced new constants

9ca04dd

Replaced PDO::FETCH_* and PDO::PARAM_* constants

1d09430

Replaced PDO::CASE_* constants

c2a4ae0

morozov added 8 commits January 29, 2018 13:16

Reworked tests to avoid hard dependency on PDO

b24fe97

Added conversion of DBAL constants to PDO ones for PDOStatement

4c80584

Expressed PARAM_*_ARRAY through other constants to eliminate the depe…

a979518

…ndency on the actual constant values

Updated UPGRADE.md for 2.x

8da47ff

Made enum classes final

9694138

Fixed mistake in documentation

7dadba4

Fixed code style

0e99c34

Fixed some more code style

4190669

morozov force-pushed the issues/2953-2.x branch from 97ef0b0 to 4190669 Compare January 29, 2018 21:18

More code style fixes

be4253f

Ocramius approved these changes Feb 1, 2018

View reviewed changes

Ocramius added this to the 2.7.0 milestone Feb 1, 2018

Ocramius assigned Ocramius and unassigned morozov Feb 1, 2018

Ocramius merged commit e5fe8c8 into doctrine:master Feb 1, 2018

morozov deleted the issues/2953-2.x branch February 1, 2018 15:56

Majkl578 mentioned this pull request Feb 5, 2018

Whitelist @requires annotation doctrine/coding-standard#25

Merged

Majkl578 reviewed Feb 16, 2018

View reviewed changes

morozov added the Deprecation label Apr 4, 2018

This was referenced Apr 4, 2018

Custom PDO fetch modes and 2.7.0 #3082

Closed

avoid using deprecated PDO:: constants in calls to doctrine dbal jackalope/jackalope-doctrine-dbal#355

Closed

Majkl578 mentioned this pull request Apr 18, 2018

PDO::FETCH_KEY_PAIR Missing from FetchMode Constants #3111

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forward compatibility with 3.x #2996

Forward compatibility with 3.x #2996

morozov commented Jan 25, 2018

Majkl578 left a comment

Majkl578 Jan 25, 2018

morozov Jan 25, 2018

Majkl578 Jan 25, 2018

Majkl578 Jan 25, 2018

morozov Jan 25, 2018 •

edited

Loading

Ocramius Jan 26, 2018 •

edited

Loading

morozov Jan 28, 2018 •

edited

Loading

Ocramius Jan 29, 2018

morozov commented Jan 28, 2018 •

edited

Loading

Majkl578 commented Jan 28, 2018

morozov commented Jan 28, 2018 •

edited

Loading

carusogabriel commented Jan 28, 2018

Majkl578 commented Jan 28, 2018 •

edited

Loading

kukulich commented Jan 28, 2018

Majkl578 commented Jan 28, 2018

morozov commented Jan 29, 2018

morozov commented Jan 29, 2018

carusogabriel commented Jan 29, 2018

morozov commented Jan 29, 2018

Majkl578 commented Jan 29, 2018 •

edited

Loading

Ocramius commented Jan 29, 2018

Ocramius left a comment

Ocramius Jan 29, 2018

morozov Jan 29, 2018

Ocramius Jan 29, 2018

morozov commented Jan 29, 2018

morozov commented Feb 1, 2018

Ocramius commented Feb 1, 2018

Ocramius commented Feb 6, 2018

Majkl578 Feb 16, 2018

morozov Feb 16, 2018

Forward compatibility with 3.x #2996

Forward compatibility with 3.x #2996

Conversation

morozov commented Jan 25, 2018

Majkl578 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

morozov Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Ocramius Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

morozov Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov commented Jan 28, 2018 • edited Loading

Majkl578 commented Jan 28, 2018

morozov commented Jan 28, 2018 • edited Loading

carusogabriel commented Jan 28, 2018

Majkl578 commented Jan 28, 2018 • edited Loading

kukulich commented Jan 28, 2018

Majkl578 commented Jan 28, 2018

morozov commented Jan 29, 2018

morozov commented Jan 29, 2018

carusogabriel commented Jan 29, 2018

morozov commented Jan 29, 2018

Majkl578 commented Jan 29, 2018 • edited Loading

Ocramius commented Jan 29, 2018

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

morozov commented Jan 29, 2018

morozov commented Feb 1, 2018

Ocramius commented Feb 1, 2018

Ocramius commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov Jan 25, 2018 •

edited

Loading

Ocramius Jan 26, 2018 •

edited

Loading

morozov Jan 28, 2018 •

edited

Loading

morozov commented Jan 28, 2018 •

edited

Loading

morozov commented Jan 28, 2018 •

edited

Loading

Majkl578 commented Jan 28, 2018 •

edited

Loading

Majkl578 commented Jan 29, 2018 •

edited

Loading