Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures #3494

morozov · 2019-03-19T06:22:37Z

Q	A
Type	improvement
BC Break	yes

This breaking change is needed to enforce the type of $start natively. Additionally, all function arguments that accept an SQL expression will be type-hinted as string or ?string, regardless of which SQL type they represent. Function arguments have been rename closer to their SQL analogs.

morozov · 2019-03-19T06:23:21Z

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

-    /**
-     * {@inheritDoc}
-     */
-    public function getSubstringExpression($value, $from, $length = null)


This is the same as the default implementation.

morozov · 2019-03-19T06:24:00Z

lib/Doctrine/DBAL/Platforms/SqlitePlatform.php

@@ -96,28 +96,26 @@ public function getTrimExpression(string $str, int $mode = TrimMode::UNSPECIFIED

    /**
     * {@inheritDoc}
-     *
-     * SQLite only supports the 2 parameter variant of this function


This is obsolete.

Do we know since when? We should probably require SQLite to be at least newer than X

morozov · 2019-03-19T06:24:29Z

tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php

@@ -332,7 +332,6 @@ public function testGeneratesSQLSnippets()
        self::assertEquals("'1987/05/02' - 10 YEAR", $this->platform->getDateSubYearsExpression("'1987/05/02'", 10));
        self::assertEquals(' WITH RR USE AND KEEP UPDATE LOCKS', $this->platform->getForUpdateSQL());
        self::assertEquals('LOCATE(substring_column, string_column)', $this->platform->getLocateExpression('string_column', 'substring_column'));
-        self::assertEquals('LOCATE(substring_column, string_column)', $this->platform->getLocateExpression('string_column', 'substring_column'));


This is a duplicate of the above.

Ocramius · 2019-03-19T06:30:57Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

-     * @param int|false $startPos Position to start at, beginning of string by default.
-     *
-     * @return string
+     * @param string      $string    String to locate the substring in, SQL expression.


This comment is unclear to me - can you rephrase it, please?

This is the best I could come up with. Otherwise, it was "The SQL expression which represents the string in which the substring should be located". And this "The SQL expression which represents" will be in a lot of places. Is that better?

Aha, the wording "sql expression" put after the comma did confuse me

So what do you recommend?

"SQL expression producing the haystack" or something like that?

Ocramius · 2019-03-19T06:31:32Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

-     *
-     * @return string
+     * @param string      $string    String to locate the substring in, SQL expression.
+     * @param string      $substring Substring to locate, SQL expression.


I think a better naming could be "needle"

I'd keep it closer to the SQL naming, not to PHP. Otherwise, it'd have to be $haystack and $needle.

Ocramius · 2019-03-19T06:32:13Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

-     * @return string
+     * @param string      $string    String to locate the substring in, SQL expression.
+     * @param string      $substring Substring to locate, SQL expression.
+     * @param string|null $start     Position to start at, SQL expression. Defaults to the beginning of the string.


Alao confusing - why is this a string? Could we maybe add example usages?

Because it's an SQL expression, not a value (see the test). It could be a reference to another column in the query or a function call or whatever else. There will be a lot of changes like this in the API. Not sure if we want to put an example in every occurrence. What would you suggest?

Ocramius · 2019-03-19T06:33:44Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

-     *
-     * @return string
+     * @param string      $string String to get a substring in, SQL expression.
+     * @param string      $start  Position to start at, SQL expression.


Similar to the above, start being a string is confusing to me - we'll need usage examples, IMO

There are more than 40 methods in this class which accept SQL expressions. We do need some explanation but it doesn't have to be a phpDoc example for every one of them. Maybe it could be some footnote or a link to the online documentation?

I'd like to leave the doc block in a good enough shape and proceed with the code changes. I hope, someone could join later and improve the documentation as well.

Ocramius · 2019-03-19T06:34:31Z

lib/Doctrine/DBAL/Platforms/SqlitePlatform.php

@@ -96,28 +96,26 @@ public function getTrimExpression(string $str, int $mode = TrimMode::UNSPECIFIED

    /**
     * {@inheritDoc}
-     *
-     * SQLite only supports the 2 parameter variant of this function


Do we know since when? We should probably require SQLite to be at least newer than X

morozov · 2019-03-19T06:50:46Z

Do we know since when? We should probably require SQLite to be at least newer than X

No, but at least since 2008.

…pression() signatures

morozov · 2019-03-21T20:59:07Z

@Ocramius do you still have any concerns or we can proceed?

Ocramius

Let's proceed 👍

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures

morozov commented Mar 19, 2019

View reviewed changes

morozov added Improvement Platforms BC Break labels Mar 19, 2019

morozov force-pushed the get-locate-expr branch from c6e6e20 to 5dd0d27 Compare March 19, 2019 06:28

Ocramius suggested changes Mar 19, 2019

View reviewed changes

morozov force-pushed the get-locate-expr branch from 5dd0d27 to 297ca6b Compare March 19, 2019 15:37

morozov requested a review from Ocramius March 19, 2019 15:55

Modified AbstractPlatform::getLocateExpression() and ::getSubstringEx…

6c74cf2

…pression() signatures

morozov force-pushed the get-locate-expr branch from 297ca6b to 6c74cf2 Compare March 20, 2019 03:25

morozov requested a review from Majkl578 March 20, 2019 15:18

Ocramius approved these changes Mar 21, 2019

View reviewed changes

Ocramius self-assigned this Mar 21, 2019

Ocramius added this to the 3.0.0 milestone Mar 21, 2019

Ocramius merged commit 8dba786 into doctrine:develop Mar 21, 2019

morozov deleted the get-locate-expr branch March 22, 2019 01:17

morozov mentioned this pull request Mar 22, 2019

Enforce parameter and return value types in the codebase #3348

Merged

morozov pushed a commit that referenced this pull request Apr 16, 2019

Merge pull request #3494 from morozov/get-locate-expr

6c5f79b

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures

morozov pushed a commit that referenced this pull request May 6, 2019

Merge pull request #3494 from morozov/get-locate-expr

d340da9

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures

morozov pushed a commit that referenced this pull request May 23, 2019

Merge pull request #3494 from morozov/get-locate-expr

3631ed5

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures

morozov added the Strict Typing label May 23, 2019

morozov pushed a commit to morozov/dbal that referenced this pull request May 31, 2019

Merge pull request doctrine#3494 from morozov/get-locate-expr

08873d8

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures

morozov pushed a commit that referenced this pull request Jun 13, 2019

Merge pull request #3494 from morozov/get-locate-expr

9af7a9f

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures

morozov pushed a commit that referenced this pull request Jun 27, 2019

Merge pull request #3494 from morozov/get-locate-expr

ca864fa

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures

morozov pushed a commit that referenced this pull request Jun 27, 2019

Merge pull request #3494 from morozov/get-locate-expr

33b7c82

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures

morozov pushed a commit that referenced this pull request Jun 27, 2019

Merge pull request #3494 from morozov/get-locate-expr

ad76157

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures

morozov pushed a commit to morozov/dbal that referenced this pull request Aug 26, 2019

Merge pull request doctrine#3494 from morozov/get-locate-expr

b4b6bb3

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures

morozov pushed a commit that referenced this pull request Nov 2, 2019

Merge pull request #3494 from morozov/get-locate-expr

91ede91

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures

morozov mentioned this pull request Jan 22, 2020

Enforced argument and return types in QueryBuilder and ExpressionBuilder #3841

Merged

morozov mentioned this pull request Aug 23, 2020

Upgrade Psalm to its latest version #4222

Merged

morozov mentioned this pull request Dec 18, 2021

Improve compatibility with Doctrine DBAL 4 doctrine/orm#9266

Merged

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

Uh oh!

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures #3494

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures #3494

Uh oh!

Conversation

morozov commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morozov commented Mar 21, 2019

Uh oh!

Ocramius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

morozov commented Mar 19, 2019 •

edited

Loading

morozov Mar 19, 2019 •

edited

Loading

morozov Mar 19, 2019 •

edited

Loading

morozov Mar 19, 2019 •

edited

Loading

morozov commented Mar 19, 2019 •

edited

Loading