zendframework
Fix AlterTable::dropConstraint() by allowing the use of decorator
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).
Fixed errors after merge with master. Ready for integration in 3.0.0 if everything's ok for you.
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.
@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?
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.
@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 — Can you please provide an update on this?
Sorry, but no. Maybe try pinging @alextech instead?
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
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?)
@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?
@alexdenvir
Maybe try pinging @alextech instead?
Yeah. Sorry for the inconvenience.
@webimpress
We can accept both -
stringandConstraintInterface, 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
@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?
@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.
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.
This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at https://github.com/laminas/laminas-db/issues/72.