Fix AlterTable::dropConstraint() by allowing the use of decorator #247

nanawel · 2017-05-22T16:43:19Z

Fixes a broken implementation under MySQL: DROP CONSTRAINT is not supported, you have to use DROP [FOREIGN KEY | INDEX | KEY] instead.

See issue #36

Warning: introduces a potential breaking change (string parameter is now a typed object).

…oken implementation under MySQL)

alextech · 2017-05-22T16:51:54Z

src/Sql/Ddl/AlterTable.php

@@ -74,7 +75,7 @@ class AlterTable extends AbstractSql implements SqlInterface
        ],
        self::DROP_CONSTRAINTS  => [
            "%1\$s" => [
-                [1 => "DROP CONSTRAINT %1\$s,\n", 'combinedby' => ""],
+                [2 => "DROP %1\$s %2\$s,\n", 'combinedby' => ""],


Do not modify generic syntax from abstract classes other adapters inherit from for engine specific fixes. Fixing one engine breaks assumptions of others within this library or others making custom adapters.

alextech · 2017-05-22T16:55:13Z

src/Sql/Ddl/AlterTable.php

-            $sqls[] = $adapterPlatform->quoteIdentifier($constraint);
+            $sqls[] = [
+                'CONSTRAINT',
+                $adapterPlatform->quoteIdentifier($constraint->getName())


Until this #233 goes through that fixes bugs in quoteIdentifier vs quoteIdentifierChain this will break PostgreSQL and MSSQL. If lucky and that PR gets merged prior to this, then its fine.

alextech · 2017-05-22T16:56:26Z

src/Sql/Platform/Mysql/Ddl/AlterTableDecorator.php

+        foreach ($this->dropConstraints as $constraint) {
+            $sqls[] = [
+                $constraint->getType(),
+                $adapterPlatform->quoteIdentifier($constraint->getName())


alextech · 2017-05-22T17:02:32Z

doc/book/sql-ddl.md

+use \Zend\Db\Metadata\Object\ConstraintObject;
+
+$fkConstraint = new ConstraintObject('my_fk', null);
+$fkConstraint->setType('FOREIGN KEY');


Disagree. If you want your constraint to represent foreign key, then use ForeignKey class that already does this.

Sure, I can do that. But as I said on issue #36, I thought it was better because it describes an existing constraint (metadata) that can be retrieved by analyzing the database, hence programatically.

Also, \Zend\Db\Sql\Ddl\Constraint\ForeignKey requires a lot of arguments, which are useless for dropping.

Oh I just noticed how ConstraintObject lets you do that... Why such duality... :( I guess at the moment both would work, but to go forward initiative to migrate to PHP 7.1 and all related type inconsistency fixes as discussed on development channel, this lose handling needs to go away. Imagine others who will not notice this duplication of functionality in ConstraintObject and ForeignKey.

I see... In that case metadata analyzer is broken not assigning foreign key constraint to ForeignKey constraint object. I should not be penalized for wanting to use given by the framework ForeignKey object to describe foreign key constraint. I suppose that means both need to be handled by the Decorator at this point since that's what the library gives us. Or clean creation of ConstraintObject to create ForeignKey, which would be further reaching change.

Or clean creation of ConstraintObject to create ForeignKey, which would be further reaching change.

\Zend\Db\Metadata\Source\AbstractSource::getConstraint() seems to be doing its job when analyzing the table and assigns the type of constraints correctly.

But yeah, when you dig that down you realize there are some nasty inconsistencies to take care of ahead of that bug.

Anyway, I'll take your remarks into account and I'll make some changes, hopefully for the best. But I'm not going to rewrite half of the framework either.

Was about to suggest to let dropConstraint method take in AbstractConstraint so you have access to best of both ConstraintObject and ForeignKey, but what do you know, ConstraintObject does not inherit from AbstractConstraint or ConstraintInterface as name would imply!
Just cannot win here without

rewrite half of the framework either.

But that's too much to ask for at this time ...

Great job on moving specifications to decorators. Those are tough to do.

For version 3, which according to discussions all new versions want to target PHP 7.1 strict type system, it would be needed to rewrite to tighten up the types allowed per method to weed out those

nasty inconsistencies to take care of ahead of that bug.

I would have fun doing it, but a little discouraging based on rate of PR reviews.

@alextech

…rface and \Zend\Db\Metadata\Object\ConstraintObject when dropping a constraint (thanks @alextech)

…raint # Conflicts: # test/Sql/Ddl/AlterTableTest.php # test/Sql/Platform/Mysql/Ddl/AlterTableDecoratorTest.php

nanawel · 2017-12-27T20:26:01Z

Fixed errors after merge with master. Ready for integration in 3.0.0 if everything's ok for you.

…raint

nanawel · 2018-08-12T12:24:39Z

I've updated my branch with the latest revision from develop.

Unfortunately it shows a failure with code coverage which is in fact invalid. The file /src/Sql/Platform/Mysql/Ddl/AlterTableDecorator.php should be almost entirely covered (tested in local with a good old die() to be sure) but it's shown as only 2% covered in Travis.

@alextech Have you ever had this issue?
BTW, this coverage problem is already present in earlier revisions, it's just that my commit increased a bit the reported missing coverage ratio.

alextech · 2018-08-12T14:06:16Z

@nanawel I too have this issue. (dont want to put in PR ID here to confuse the issue tracker relevancy, search "coverage" in open PRs where I point it out). Was passing then suddenly dropped. There was an effort to upgrade travis config files across all of ZF repositories to make it more strict and after it there was more failings here in unexpected commits. I think I see why mine failed (though its a bit stretched) where I formatted my error output and so travis wants me to tests the full formatting logic, which is just a printf() XD ....

According to the report there is unexercised "processDropConstraints" function. Line was added there but none of your tests seem to cover it. Even if the whole file has coverage by previous commits, that does not mean this particular commit is covered, therefore travis thinks it is new untested BC break (which makes sense). That is, travis checks that this set of commits is fully covering all of changes.

Do your new tests execute that line 234 or only old ones?

nanawel · 2018-08-12T17:09:29Z

There are actually 2 tests related to processDropConstraints() which are the drop of the FK and the index. I added those lines way before today's merge so nothing has changed there. In fact, the only changes required after the merge were code style related, to match how the strings are split up on develop, which is slightly different from how they were when I submitted the PR.

nanawel · 2018-12-18T20:36:20Z

Hi, still looking forward to seeing this in the upcoming 3.0.0. Thanks!

nanawel · 2019-07-01T11:00:59Z

Hi,
just bumping this PR, in case it got forgotten :)

binal-7span · 2019-08-19T09:54:18Z

@alexdenvir — Can you please provide an update on this?

I have a CMS and I am using Zend-DB. Right now I am not able to use the drop constraint functionality. I can use this code directly in my fork repo, but before that, I want to know the reason behind not letting this PR merged.

Update on this PR will be very helpful. So can you please provide it ASAP?

alexdenvir · 2019-08-27T12:13:06Z

@alexdenvir — Can you please provide an update on this?

Sorry, but no. Maybe try pinging @alextech instead?

alextech · 2019-08-27T12:22:20Z

I am just a contributor level :-)

alexdenvir · 2019-08-27T12:26:28Z

Same here, although never to zend-db specifically. I saw you had left a review and assumed @BJGajjar meant to ping you instead. My bad

michalbundyra · 2019-08-27T12:28:55Z

I see it's marked as BC Break so planned for v3 release. I guess it will be much easier to get it in without BC Break (if possible?)

michalbundyra · 2019-08-27T19:04:55Z

@alextech @nanawel @alexdenvir @BJGajjar
I think it is possible to change the code to keep BC.
We can accept both - string and ConstraintInterface, and deprecate on string.
If string provided we can create ConstraintObject internally so behaviour should really stay the same. Then for v3 we will accept only ConstraintInterface.

What do you think?

binal-7span · 2019-08-28T07:10:46Z

@alexdenvir

Maybe try pinging @alextech instead?

Yeah. Sorry for the inconvenience.

@webimpress

We can accept both - string and ConstraintInterface, and deprecate on the string.

Agree we can do that way also.

@nanawel

It seems like there is some issue with this PR. I tried to use this code in my fork repo and use the dropConstraint for Unique constraint as well as PrimaryKey. But not able to drop it.
When I print the SQL query it is returning "" as a constraint.

                    DROP CONSTRAINT ""

For your reference;
You have been added getConstraintType function in src/Sql/Platform/Mysql/Ddl/AlterTableDecorator.php and when I alter the table it is using the processDropConstraints function of src/Sql/Ddl/AlterTable.php

michalbundyra · 2019-08-28T07:12:41Z

@BJGajjar would you be able to provide PR based on that with changes I've described above, fixes of the issue you found, and tests to cover changes, please?

itsmerhp · 2019-10-23T19:47:06Z

@nanawel @webimpress @BJGajjar @alextech @alexdenvir

We can accept both - string and ConstraintInterface, and deprecate on string.
If string provided we can create ConstraintObject internally so behaviour should really stay the same.

As suggested by @webimpress, I think we can provide BC(accept both - string and ConstraintInterface) by below change :

public function dropConstraint($constraint) {
	$constraint = is_object($constraint) ? $constraint : new ConstraintObject($constraint, NULL);
	$this->dropConstraints[] = $constraint;
	return $this;
}

Please share your thoughts on it.

weierophinney · 2019-12-31T22:44:33Z

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

Squash all commits in your branch (git rebase -i origin/{branch})
Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
Run the laminas/laminas-migration tool on the code.
Clone laminas/laminas-db to another directory.
Copy the files from the second bullet point to the clone of laminas/laminas-db.
In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
We will be providing tooling via laminas/laminas-migration soon to help automate the process.

michalbundyra · 2020-01-16T19:23:58Z

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#72.

Fix AlterTable::dropConstraint() by allowing the use of decorator (br…

bfb73c8

…oken implementation under MySQL)

nanawel mentioned this pull request May 22, 2017

$AlterTable->dropConstraint() Throw error on MySQL,There is no support for "DROP CONSTRAINT" #36

Open

alextech suggested changes May 22, 2017

View reviewed changes

Update doc

e692ebc

alextech reviewed May 22, 2017

View reviewed changes

nanawel added 3 commits May 22, 2017 21:13

Adaptation to support both \Zend\Db\Sql\Ddl\Constraint\ConstraintInte…

529293a

…rface and \Zend\Db\Metadata\Object\ConstraintObject when dropping a constraint (thanks @alextech)

Remove unused use

cd5a061

Remove unused use

f287cf2

ezimuel added this to the 3.0.0 milestone Dec 4, 2017

ezimuel added the BC Break label Dec 4, 2017

nanawel added 3 commits December 27, 2017 12:44

Merge remote-tracking branch 'zend/develop' into fix-mysql-drop-const…

471667f

…raint # Conflicts: # test/Sql/Ddl/AlterTableTest.php # test/Sql/Platform/Mysql/Ddl/AlterTableDecoratorTest.php

Fix PHPCS error

308ac7b

Fix PHPCS warning

9ae5516

nanawel added 2 commits August 12, 2018 11:41

Merge remote-tracking branch 'zend/develop' into fix-mysql-drop-const…

1d0432a

…raint

Merge to match code style

07419e8

binal-7span mentioned this pull request Sep 3, 2019

Unable to remove unique on a field directus/v8-archive#1188

Open

michalbundyra mentioned this pull request Jan 16, 2020

Fix AlterTable::dropConstraint() by allowing the use of decorator laminas/laminas-db#72

Closed

nanawel mentioned this pull request Dec 27, 2020

Fix for MySQL DROP CONSTRAINT (Port of PR#247 on zend-db to laminas-db) laminas/laminas-db#179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix AlterTable::dropConstraint() by allowing the use of decorator #247

Fix AlterTable::dropConstraint() by allowing the use of decorator #247

nanawel commented May 22, 2017 •

edited

Loading

alextech May 22, 2017

alextech May 22, 2017 •

edited

Loading

alextech May 22, 2017

alextech May 22, 2017

nanawel May 22, 2017 •

edited

Loading

alextech May 22, 2017

alextech May 22, 2017

nanawel May 22, 2017

alextech May 22, 2017 •

edited

Loading

nanawel commented Dec 27, 2017

nanawel commented Aug 12, 2018

alextech commented Aug 12, 2018

nanawel commented Aug 12, 2018

nanawel commented Dec 18, 2018

nanawel commented Jul 1, 2019

binal-7span commented Aug 19, 2019

alexdenvir commented Aug 27, 2019

alextech commented Aug 27, 2019

alexdenvir commented Aug 27, 2019

michalbundyra commented Aug 27, 2019

michalbundyra commented Aug 27, 2019

binal-7span commented Aug 28, 2019

michalbundyra commented Aug 28, 2019 •

edited

Loading

itsmerhp commented Oct 23, 2019

weierophinney commented Dec 31, 2019

michalbundyra commented Jan 16, 2020

Fix AlterTable::dropConstraint() by allowing the use of decorator #247

Are you sure you want to change the base?

Fix AlterTable::dropConstraint() by allowing the use of decorator #247

Conversation

nanawel commented May 22, 2017 • edited Loading

alextech May 22, 2017

Choose a reason for hiding this comment

alextech May 22, 2017 • edited Loading

Choose a reason for hiding this comment

alextech May 22, 2017

Choose a reason for hiding this comment

alextech May 22, 2017

Choose a reason for hiding this comment

nanawel May 22, 2017 • edited Loading

Choose a reason for hiding this comment

alextech May 22, 2017

Choose a reason for hiding this comment

alextech May 22, 2017

Choose a reason for hiding this comment

nanawel May 22, 2017

Choose a reason for hiding this comment

alextech May 22, 2017 • edited Loading

Choose a reason for hiding this comment

nanawel commented Dec 27, 2017

nanawel commented Aug 12, 2018

alextech commented Aug 12, 2018

nanawel commented Aug 12, 2018

nanawel commented Dec 18, 2018

nanawel commented Jul 1, 2019

binal-7span commented Aug 19, 2019

alexdenvir commented Aug 27, 2019

alextech commented Aug 27, 2019

alexdenvir commented Aug 27, 2019

michalbundyra commented Aug 27, 2019

michalbundyra commented Aug 27, 2019

binal-7span commented Aug 28, 2019

michalbundyra commented Aug 28, 2019 • edited Loading

itsmerhp commented Oct 23, 2019

weierophinney commented Dec 31, 2019

michalbundyra commented Jan 16, 2020

nanawel commented May 22, 2017 •

edited

Loading

alextech May 22, 2017 •

edited

Loading

nanawel May 22, 2017 •

edited

Loading

alextech May 22, 2017 •

edited

Loading

michalbundyra commented Aug 28, 2019 •

edited

Loading