Support `IS NULL` checking in `Connection#delete()` and `Connection#update()` generated criteria, allowing for `null` column searches #2688

jnvsor · 2017-03-25T18:02:11Z

If you pass a null to the where array of update or delete it's
pretty obvious what you want.

Most (all?) databases won't match anything if you actually try
to compare with NULL making this a silent failure without it.

jnvsor · 2017-03-26T19:39:17Z

Can anyone tell me what I should do about adding a test? Should I just pick a number that doesn't exist yet and stick it in a @group annotation or is there a protocol to follow here?

Ocramius · 2017-03-26T23:24:35Z

Pick the number for this PR for now (2688)

…

On 26 Mar 2017 2:39 p.m., "Jonathan Vollebregt" ***@***.***> wrote: Can anyone tell me what I should do about adding a test? Should I just pick a number that doesn't exist yet and stick it in a @group annotation or is there a protocol to follow here? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2688 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakCgC1-OeEoYbzhCjvCy1RW8pExS0ks5rpr7mgaJpZM4MpKH9> .

jnvsor · 2017-03-27T05:23:39Z

Thanks! The test is added, so feel free to merge if my points on backwards compatibility are ok

lcobucci · 2017-03-27T09:18:21Z

lib/Doctrine/DBAL/Connection.php

-            $columnList[] = $columnName;
-            $criteria[] = $columnName . ' = ?';
-            $paramValues[] = $value;
+            if (is_null($value)) {


Can we not duplicate this on the two different methods?

I don't see how without passing columnList, criteria and paramValues as references to the helper method. (Which is probably why they're duplicated in the first place) Is that ok?

lcobucci · 2017-03-27T09:18:47Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+     */
+    public function testUpdateWithIsNull()
+    {
+        $driverMock = $this->createMock('Doctrine\DBAL\Driver');


Please use the ::class syntax

I copy pasted these from existing test testUpdateWithSameColumnInDataAndIdentifiers - I guess the code style has changed since then? I'll get the changes done

lcobucci · 2017-03-27T09:19:05Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+            ->method('connect')
+            ->will($this->returnValue(new DriverConnectionMock()));
+
+        $conn = $this->getMockBuilder('Doctrine\DBAL\Connection')


Same as https://github.com/doctrine/dbal/pull/2688/files#r108123891

Can you extract this to a method (you can also encapsulate $driverMock there) so that it can be reused on delete()?

lcobucci · 2017-03-27T09:19:19Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+            ->will($this->returnValue(new DriverConnectionMock()));
+
+        $conn = $this->getMockBuilder('Doctrine\DBAL\Connection')
+            ->setMethods(array('executeUpdate'))


Please use the short array syntax

lcobucci · 2017-03-27T09:19:29Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+
+        $conn = $this->getMockBuilder('Doctrine\DBAL\Connection')
+            ->setMethods(array('executeUpdate'))
+            ->setConstructorArgs(array(array('platform' => new Mocks\MockPlatform()), $driverMock))


Same as https://github.com/doctrine/dbal/pull/2688/files#r108124008

lcobucci · 2017-03-27T09:20:21Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+     */
+    public function testDeleteWithIsNull()
+    {
+        $driverMock = $this->createMock('Doctrine\DBAL\Driver');


Same as https://github.com/doctrine/dbal/pull/2688/files#r108123891

lcobucci · 2017-03-27T09:20:25Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+            ->method('connect')
+            ->will($this->returnValue(new DriverConnectionMock()));
+
+        $conn = $this->getMockBuilder('Doctrine\DBAL\Connection')


Same as https://github.com/doctrine/dbal/pull/2688/files#r108123891

lcobucci · 2017-03-27T09:20:48Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+            ->will($this->returnValue(new DriverConnectionMock()));
+
+        $conn = $this->getMockBuilder('Doctrine\DBAL\Connection')
+            ->setMethods(array('executeUpdate'))


Same as https://github.com/doctrine/dbal/pull/2688/files#r108124008

lcobucci · 2017-03-27T09:20:51Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+
+        $conn = $this->getMockBuilder('Doctrine\DBAL\Connection')
+            ->setMethods(array('executeUpdate'))
+            ->setConstructorArgs(array(array('platform' => new Mocks\MockPlatform()), $driverMock))


Same as https://github.com/doctrine/dbal/pull/2688/files#r108124008

deeky666

Concerning my comments about the tests, it might be a good idea to add a simple functional test for each operation, too.

deeky666 · 2017-04-14T09:42:55Z

lib/Doctrine/DBAL/Connection.php

+     * @param array &$criteria    Output array of criteria
+     * @param array &$paramValues Output bound values
+     */
+    protected function setCriteria($identifiers, &$columnList, &$criteria, &$paramValues)


Please make this method private as it is not meant to be an extension point for derived classes. Also I would suggest naming the method differently as it is not really modifying the state of the object but rather gathering criteria. So renaming it to something like gatherCriteria() would make sense to me.

deeky666 · 2017-04-14T09:44:01Z

lib/Doctrine/DBAL/Connection.php

+    protected function setCriteria($identifiers, &$columnList, &$criteria, &$paramValues)
+    {
+        foreach ($identifiers as $columnName => $value) {
+            if (is_null($value)) {


Please use a null === $value here

deeky666 · 2017-04-14T09:45:07Z

lib/Doctrine/DBAL/Connection.php

+        foreach ($identifiers as $columnName => $value) {
+            if (is_null($value)) {
+                $criteria[] = $this->getDatabasePlatform()->getIsNullExpression($columnName);
+            } else {


Please avoid the else here. You can continue early at the end of the if block.

deeky666 · 2017-04-14T10:08:46Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+            [
+                'text' => 'string',
+                'is_edited' => 'null',
+                'id' => 'null',


Specifying 'null' type is a bit weird because Doctrine does not have such type. As the testsuite somewhat also serves as API usage documentation I would suggest not promoting misusage here. I am pretty sure that code won't work when actually executed as Doctrine should complain about not knowing null type.
IMO the best would be to not specify any type for null values and let Doctrine decide what to do with it. As an alternative, if you want to complete type specification here, you should use the approriate types that reflect the column type like for id you would probably pass integer as type.

deeky666 · 2017-04-14T10:15:36Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+                'name' => 'foo',
+            ],
+            [
+                'id' => 'null',


Same as above. Also specifying a type for null values here does make even less sense as those values won't be turned into bound parameters.

jnvsor · 2017-04-14T16:50:21Z

@deeky666 Addressed your points and added tests to WriteTest. Is there an up to date code style guide somewhere? Lots of the older code doesn't look anything like your suggestions

phansys

Status: Needs work.

phansys · 2017-05-03T17:14:54Z

tests/Doctrine/Tests/DBAL/Functional/WriteTest.php

+
+        $data = $this->_conn->fetchColumn('SELECT COUNT(*) FROM write_table WHERE test_int = 30');
+
+        $this->assertEquals(1, $data);


Please, use assertCount() instead.

assertCount isn't used anywhere in the WriteTest file - are you sure?

IMO, no matter if the right assertion was used in other tests or not, this must not prevent you to make a proper check.

Ok, I'll use count then. As I asked before:

Is there an up to date code style guide somewhere?

Which type of assert choose isn't related to CS, it depends on the type of data you need to check.

Right, but the data was an int. Choosing between doing the count in the assert or in the query is CS

phansys

Status: Needs work.

phansys · 2017-05-03T17:16:11Z

lib/Doctrine/DBAL/Connection.php

+     * @param array &$criteria    Output array of criteria
+     * @param array &$paramValues Output bound values
+     */
+    private function gatherCriteria($identifiers, &$columnList, &$criteria, &$paramValues)


Can these arguments be declared?

How do you mean?

private function gatherCriteria(array $identifiers, array &$columnList, array &$criteria, array &$paramValues)

jnvsor · 2017-06-23T16:09:42Z

Any chance I could get a new review?

Ocramius

Patch looks awesome, but I will need to go through it to remove that by-ref gatherCriteria signature (returning a triplet instead)

jnvsor · 2017-06-23T19:38:12Z

I'll get on that right now then, thanks for the feedback!

Ocramius · 2017-06-23T19:39:26Z

@jnvsor ideally, the result would be list($columnList, $criteria, $parameterValues) = $this->theFunction(...)

jnvsor · 2017-06-23T19:40:20Z

How should I write up the docstring? Just @returns array [x, y, z]?

Ocramius · 2017-06-23T19:42:39Z

@jnvsor

/**
 * ...
 * @return string[][] a triplet with:
 *                    - the first key being the column list
 *                    - the second key being the criteria string
 *                    - the third key being the bound parameter types
 */

jnvsor · 2017-06-23T20:07:48Z

Ok, I've cleaned it up a bit:

    /**
     * Gathers criteria for an update or delete call.
     *
     * @param array $identifiers Input array of columns to values
     * @param array $columns     List of assigned column names
     * @param array $values      List of assigned values
     *
     * @return string[][] a triplet with:
     *                    - the first key being the column names
     *                    - the second key being the values
     *                    - the third key being the criteria strings
     */
    private function gatherCriteria(array $identifiers, array $columns = [], array $values = [])

[...]

list($columnList, $paramValues, $criteria) = $this->gatherCriteria($identifier, $columnList, $paramValues);

How's that?

Ocramius · 2017-06-23T20:11:02Z

Why do you need , $columnList, $paramValues);?

jnvsor · 2017-06-23T20:12:45Z

Because the SET in an UPDATE query comes before the WHERE - if we don't append to them the parameters will be bound out of order. That said, they're optional because you don't have a SET in a DELETE (only in an UPDATE)

Ocramius · 2017-06-23T20:13:19Z

if we don't append

array_merge() the results ;-)

jnvsor · 2017-06-23T20:13:43Z

Yeah but then you can't destructure can you? Or should I just array_merge on the other side?

Ocramius · 2017-06-23T20:14:58Z

list($columnList, $criteria, $parameterValues) = $this->theFunction(...);
$columns = array_merge($previouslyDefinedColumns, $columnList);
$filter = array_merge($previouslyDefinedCriteria, $criteria);

If you pass a null to the where array of update or delete it's pretty obvious what you want. Most (all?) databases won't match anything if you actually try to compare with NULL making this a silent failure without it.

jnvsor · 2017-06-23T20:19:44Z

Done. Though looking at it the variables in update/delete could stand a renaming. columns, values, conditions?

Edit: You know what, never mind. If we start nitpicking variable naming we'll be here another 3 months!

Ocramius · 2017-06-23T20:22:13Z

@jnvsor very good improvement! The last bit to improve is to avoid overwriting variables at all, and then we're done. This bit specifically:

$columnList = array_merge($columnList, $whereColumns);
$paramValues = array_merge($paramValues, $whereValues);

Indeed, a variable rename is needed

jnvsor · 2017-06-23T20:30:20Z

Added a variable rename and stopped the overwriting

Ocramius · 2017-06-23T20:31:33Z

@jnvsor awesome! Waiting for travis and then this can be merged :-)

Ocramius · 2017-06-23T20:42:55Z

🚢

Ocramius · 2017-06-23T20:43:07Z

Thanks @jnvsor!

jnvsor force-pushed the isnull branch from 16ba23d to 04919c9 Compare March 25, 2017 18:37

jnvsor force-pushed the isnull branch from 04919c9 to 06898e6 Compare March 27, 2017 05:22

jnvsor changed the title ~~[WIP] Connection: Support IS NULL in update/delete criteria~~ Connection: Support IS NULL in update/delete criteria Mar 27, 2017

lcobucci suggested changes Mar 27, 2017

View reviewed changes

jnvsor force-pushed the isnull branch 2 times, most recently from a2f8ef2 to fc7ff5c Compare March 27, 2017 17:17

deeky666 suggested changes Apr 14, 2017

View reviewed changes

deeky666 added Connections New Feature Prepared Statements labels Apr 14, 2017

deeky666 self-assigned this Apr 14, 2017

jnvsor force-pushed the isnull branch 2 times, most recently from 01532e4 to d047285 Compare April 14, 2017 16:48

jnvsor force-pushed the isnull branch from d047285 to 4cd3fb5 Compare April 14, 2017 16:51

phansys suggested changes May 3, 2017

View reviewed changes

jnvsor force-pushed the isnull branch from 4cd3fb5 to c2f8db8 Compare May 4, 2017 16:04

Ocramius approved these changes Jun 23, 2017

View reviewed changes

jnvsor force-pushed the isnull branch 3 times, most recently from 4087216 to e2e3abc Compare June 23, 2017 20:05

jnvsor added 2 commits June 23, 2017 22:17

Connection: Support IS NULL in update/delete criteria

07a4846

If you pass a null to the where array of update or delete it's pretty obvious what you want. Most (all?) databases won't match anything if you actually try to compare with NULL making this a silent failure without it.

Extract repeated code to getExecuteUpdateMockConnection method

cab04d0

jnvsor force-pushed the isnull branch from e2e3abc to cab04d0 Compare June 23, 2017 20:18

Connection: insert/update/delete variable naming cleanup

f4437ac

Ocramius added this to the 2.6 milestone Jun 23, 2017

Ocramius assigned Ocramius and unassigned deeky666 Jun 23, 2017

Ocramius merged commit 03e1718 into doctrine:master Jun 23, 2017

Ocramius changed the title ~~Connection: Support IS NULL in update/delete criteria~~ Support IS NULL checking in Connection#delete() and Connection#update() generated criteria, allowing for null column searches Jul 22, 2017

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


		$data = $this->_conn->fetchColumn('SELECT COUNT(*) FROM write_table WHERE test_int = 30');

		$this->assertEquals(1, $data);

Support IS NULL checking in Connection#delete() and Connection#update() generated criteria, allowing for null column searches #2688

Support IS NULL checking in Connection#delete() and Connection#update() generated criteria, allowing for null column searches #2688

Conversation

jnvsor commented Mar 25, 2017 • edited Loading

jnvsor commented Mar 26, 2017

Ocramius commented Mar 26, 2017 via email

jnvsor commented Mar 27, 2017

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

deeky666 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

jnvsor commented Apr 14, 2017

phansys 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

phansys 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

jnvsor commented Jun 23, 2017

Ocramius left a comment

Choose a reason for hiding this comment

jnvsor commented Jun 23, 2017

Ocramius commented Jun 23, 2017

jnvsor commented Jun 23, 2017

Ocramius commented Jun 23, 2017

jnvsor commented Jun 23, 2017

Ocramius commented Jun 23, 2017

jnvsor commented Jun 23, 2017

Ocramius commented Jun 23, 2017

jnvsor commented Jun 23, 2017 • edited Loading

Ocramius commented Jun 23, 2017

jnvsor commented Jun 23, 2017 • edited Loading

Ocramius commented Jun 23, 2017

jnvsor commented Jun 23, 2017

Ocramius commented Jun 23, 2017

Ocramius commented Jun 23, 2017

Ocramius commented Jun 23, 2017

Support `IS NULL` checking in `Connection#delete()` and `Connection#update()` generated criteria, allowing for `null` column searches #2688

Support `IS NULL` checking in `Connection#delete()` and `Connection#update()` generated criteria, allowing for `null` column searches #2688

jnvsor commented Mar 25, 2017 •

edited

Loading

jnvsor commented Jun 23, 2017 •

edited

Loading

jnvsor commented Jun 23, 2017 •

edited

Loading