Delete from aliased table #342

OleksandrBonar · 2018-11-12T14:36:16Z

This fix possible delete items from aliased table through TableGateway class

michalbundyra

@OleksandrBonar Please add test case to cover your changes and description why the change is needed.

OleksandrBonar · 2018-11-13T09:40:57Z

This fix possible delete items from aliased table through TableGateway class.
@webimpress Done

src/TableGateway/AbstractTableGateway.php

michalbundyra · 2018-11-15T11:47:15Z

src/TableGateway/AbstractTableGateway.php

@@ -439,7 +439,10 @@ public function deleteWith(Delete $delete)
    protected function executeDelete(Delete $delete)
    {
        $deleteState = $delete->getRawState();
-        if ($deleteState['table'] != $this->table) {
+        if ($deleteState['table'] != $this->table
+            && (is_array($deleteState['table'])


I think you want to have here || instead of &&. This should be covered in tests. I can't see related test to this change.

In "executeSelect" method the same logic and it works

Hm... So I guess in both places it's wrong. What do you think?

executeSelect for aliased table it works!

@OleksandrBonar You are missing test to cover that change.

@OleksandrBonar can you provide the unit test mentioned by @webimpress? Thanks!

test/unit/TableGateway/AbstractTableGatewayTest.php

OleksandrBonar · 2018-11-16T08:37:09Z

@malukenho, @webimpress what next, guys?

michalbundyra · 2018-11-16T08:41:37Z

@OleksandrBonar

I'd really like to help, but I am not the maintainer of the library... You have to be patient :)
Please add more test to cover all changes you made. See my comment above.

OleksandrBonar · 2019-01-02T10:06:09Z

@webimpress, @ezimuel Tests have already been added. What need to add?

michalbundyra · 2019-01-02T13:27:13Z

@OleksandrBonar I was saying that there are missing test cases for the condition you've updated where we should get exception. I have some "wrong feelings" about that conditions and tests will verify them :)

OleksandrBonar · 2019-01-02T15:41:28Z

@webimpress Method testDeleteWithArrayTable in file test/unit/TableGateway/AbstractTableGatewayTest.php must catch an exception

michalbundyra · 2019-01-02T15:49:00Z

@OleksandrBonar

Method testDeleteWithArrayTable in file test/unit/TableGateway/AbstractTableGatewayTest.php must catch an exception

As I can see that method tests just positive scenario, without the exception. I'd like to have scenarios when as the result we should get the exception so something with:

$this->expectException(...);
$this->expectExceptionMessage(...);

and it is not there, but it should be.

OleksandrBonar · 2019-01-03T08:49:50Z

I apologize. Looking at a fresh look at the code, I realized that checking the table variable was overly complicated.

OleksandrBonar · 2019-01-03T10:21:15Z

@webimpress In the delete method, when the table name is an array with an alias, the original name is retrieved to fulfill the query. Then the alias is restored. This situation is tested in the file TableGatewayTest.php

michalbundyra

@OleksandrBonar It makes much more sense right now. Please see my comments, update PR and then it will be all good for me. Thanks! 👍

michalbundyra · 2019-01-03T20:59:18Z

test/unit/TableGateway/TableGatewayTest.php

@@ -260,4 +261,66 @@ public function testUpdateShouldResetTableToUnaliasedTable($tableValue, $expecte
            $state['table']
        );
    }
+
+    /**
+     *@dataProvider aliasedTables


Please add space after *.

test/unit/TableGateway/TableGatewayTest.php

OleksandrBonar · 2019-01-04T08:48:55Z

@webimpress done! )

michalbundyra

LGTM 👍

ThomasRo · 2019-01-04T16:07:25Z

The provided solution is consistent with the one for the update statement (method AbstractTableGateway::executeUpdate), but i do not like both of them.
They both disable the possibility to use aliases with update and delete statements, while indeed aliases make sense, in particular if you make use of additional joins with your update or delete statements. zend-db natively does allow joins in update statements; some RDBMS do also allow joins in delete statements and the zend sources can easily extended to make use of them.
I would suggest to fix Zend\Db\Sql\AbstractSql::resolveTable in order to support aliases in all statements.
Overwriting attributes (and losing information) while executing a statement, does not feel right. Furthermore a statements gets build, which is different from that one, which you get by calling AbstractSql::getSqlString (for debugging purposes) because there, the overwriting does not take place.

michalbundyra · 2019-01-04T16:39:24Z

@ThomasRo Can you provide test case, please? Then we will try to find solution to resolve both issues.

ThomasRo · 2019-01-08T15:24:05Z

@webimpress I have tried to, but came across many impacts to other statements so that the necessary efforts are to big... Since i won't deliver a better solution, i go with the one of @OleksandrBonar

OleksandrBonar · 2019-02-27T11:55:43Z

@ezimuel updates published

OleksandrBonar · 2019-03-22T08:46:42Z

bump

OleksandrBonar · 2019-04-11T13:07:00Z

@malukenho please review and approve or decline these changes

michalbundyra · 2019-12-31T17:23:13Z

Thanks, @OleksandrBonar!

…teFromAliasedTable Delete from aliased table

Forward port #342

OleksandrBonar added 2 commits November 12, 2018 16:31

Update AbstractTableGateway.php

5dba6c7

Update AbstractTableGateway.php

5dd1010

OleksandrBonar changed the base branch from develop to master November 12, 2018 14:37

michalbundyra suggested changes Nov 12, 2018

View reviewed changes

OleksandrBonar added 5 commits November 13, 2018 09:41

Update AbstractTableGatewayTest.php

d4ee14c

Update AbstractTableGateway.php

9749ec0

Update AbstractTableGateway.php

7ba698b

Syntax fix

3797dfb

Fixed the called method name

defc64e

malukenho reviewed Nov 14, 2018

View reviewed changes

src/TableGateway/AbstractTableGateway.php Outdated Show resolved Hide resolved

michalbundyra reviewed Nov 15, 2018

View reviewed changes

test/unit/TableGateway/AbstractTableGatewayTest.php Outdated Show resolved Hide resolved

Removed unnecessary code

23f475f

ezimuel self-assigned this Dec 6, 2018

ezimuel added the awaiting author updates label Dec 6, 2018

Update AbstractTableGateway.php

41b82ee

OleksandrBonar added 3 commits January 3, 2019 11:20

Update AbstractTableGatewayTest.php

6eb0f8b

Update TableGatewayTest.php

0742dc5

Update AbstractTableGatewayTest.php

5e2fe27

michalbundyra suggested changes Jan 3, 2019

View reviewed changes

Update TableGatewayTest.php

02b0e21

michalbundyra approved these changes Jan 4, 2019

View reviewed changes

Merge branch 'master' into OleksandrBonar-Patch-DeleteFromAliasedTable

ee1f6d9

michalbundyra added bug and removed awaiting author updates labels Dec 31, 2019

michalbundyra added this to the 2.10.1 milestone Dec 31, 2019

michalbundyra added a commit that referenced this pull request Dec 31, 2019

Merge pull request #342 from OleksandrBonar/OleksandrBonar-Patch-Dele…

257168e

…teFromAliasedTable Delete from aliased table

michalbundyra added a commit that referenced this pull request Dec 31, 2019

Adds CHANGELOG entry for #342

b844310

michalbundyra added a commit that referenced this pull request Dec 31, 2019

Merge branch 'hotfix/342' into develop

66151ad

Forward port #342

michalbundyra closed this in 19c9bfb Dec 31, 2019

michalbundyra merged commit ee1f6d9 into zendframework:master Dec 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete from aliased table #342

Delete from aliased table #342

OleksandrBonar commented Nov 12, 2018 •

edited

Loading

michalbundyra left a comment

OleksandrBonar commented Nov 13, 2018 •

edited

Loading

michalbundyra Nov 15, 2018

OleksandrBonar Nov 15, 2018 •

edited

Loading

michalbundyra Nov 15, 2018

OleksandrBonar Nov 15, 2018

michalbundyra Nov 16, 2018

ezimuel Dec 6, 2018

OleksandrBonar commented Nov 16, 2018

michalbundyra commented Nov 16, 2018

OleksandrBonar commented Jan 2, 2019

michalbundyra commented Jan 2, 2019

OleksandrBonar commented Jan 2, 2019

michalbundyra commented Jan 2, 2019

OleksandrBonar commented Jan 3, 2019

OleksandrBonar commented Jan 3, 2019

michalbundyra left a comment

michalbundyra Jan 3, 2019

OleksandrBonar commented Jan 4, 2019

michalbundyra left a comment

ThomasRo commented Jan 4, 2019

michalbundyra commented Jan 4, 2019

ThomasRo commented Jan 8, 2019

OleksandrBonar commented Feb 27, 2019

OleksandrBonar commented Mar 22, 2019

OleksandrBonar commented Apr 11, 2019

michalbundyra commented Dec 31, 2019

Delete from aliased table #342

Delete from aliased table #342

Conversation

OleksandrBonar commented Nov 12, 2018 • edited Loading

michalbundyra left a comment

Choose a reason for hiding this comment

OleksandrBonar commented Nov 13, 2018 • edited Loading

michalbundyra Nov 15, 2018

Choose a reason for hiding this comment

OleksandrBonar Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

michalbundyra Nov 15, 2018

Choose a reason for hiding this comment

OleksandrBonar Nov 15, 2018

Choose a reason for hiding this comment

michalbundyra Nov 16, 2018

Choose a reason for hiding this comment

ezimuel Dec 6, 2018

Choose a reason for hiding this comment

OleksandrBonar commented Nov 16, 2018

michalbundyra commented Nov 16, 2018

OleksandrBonar commented Jan 2, 2019

michalbundyra commented Jan 2, 2019

OleksandrBonar commented Jan 2, 2019

michalbundyra commented Jan 2, 2019

OleksandrBonar commented Jan 3, 2019

OleksandrBonar commented Jan 3, 2019

michalbundyra left a comment

Choose a reason for hiding this comment

michalbundyra Jan 3, 2019

Choose a reason for hiding this comment

OleksandrBonar commented Jan 4, 2019

michalbundyra left a comment

Choose a reason for hiding this comment

ThomasRo commented Jan 4, 2019

michalbundyra commented Jan 4, 2019

ThomasRo commented Jan 8, 2019

OleksandrBonar commented Feb 27, 2019

OleksandrBonar commented Mar 22, 2019

OleksandrBonar commented Apr 11, 2019

michalbundyra commented Dec 31, 2019

OleksandrBonar commented Nov 12, 2018 •

edited

Loading

OleksandrBonar commented Nov 13, 2018 •

edited

Loading

OleksandrBonar Nov 15, 2018 •

edited

Loading