Fix issue with dropColumn command in SQLite (#1086) #1093
Conversation
|
I've been hunting down a pretty nasty Phinx bug for the last 5 hours and this PR pointed me in the right direction. The issue is with I haven't fixed it but at least now I know where the issue is coming from. I'll re-visit the issue when I have more time, for now I've just disabled migrations on my sqlite database. |
This fixes a pretty serious bug in the Phinx SQLiteAdapter that causes columns to be destroyed when you're removing the default value of `''` from a string column. See comment here: cakephp#1093 (comment) There are no tests in phinx that prove what I've changed hasn't broken anything, and I'm not about to start writing all the test cases to prove this works. Maybe I have indeed broken some weird implementation detail that this regex was intended to target... oh well.
This fixes a pretty serious bug in the Phinx SQLiteAdapter that causes columns to be destroyed when you're removing the default value of `''` from a string column. See comment here: cakephp#1093 (comment) There are no tests in phinx that prove what I've changed hasn't broken anything, and I'm not about to start writing all the test cases to prove this works. Maybe I have indeed broken some weird implementation detail that this regex was intended to target... oh well.
|
I fixed it for myself in this commit: lxrco@cd20df1 |
|
@nebez how about just sending a pull request with your fix? |
|
@lorenzo - the change has no test coverage, but I will send a PR now regardless. |
This fixes a pretty serious bug in the Phinx SQLiteAdapter that causes columns to be destroyed when you're removing the default value of `''` from a string column. See comment here: #1093 (comment) There are no tests in phinx that prove what I've changed hasn't broken anything, and I'm not about to start writing all the test cases to prove this works. Maybe I have indeed broken some weird implementation detail that this regex was intended to target... oh well.
|
@lorenzo any thoughts on my PR here? I see that the appveyor build failed but everything passes locally for me...judging from AppVeyor output, some needed PDO driver isn't being loaded, but I'm not sure why this would be the case. I'm assuming it has something to do with the change I made in EnvironmentTest.php to create an in-memory DB but I'm not entirely sure why that would be throwing a missing driver exception...nothing I can find online indicates any required voodoo to enable the memory driver. Appreciate any thoughts you have, thanks! |
src/Phinx/Db/Adapter/PdoAdapter.php
Outdated
| @@ -205,6 +205,9 @@ public function setConnection(\PDO $connection) | |||
| { | |||
| $this->connection = $connection; | |||
|
|
|||
| // ensure connection throws exceptions on errors (SQL syntax errors, etc) | |||
| $this->connection->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION); | |||
There was a problem hiding this comment.
Please remove this line, this is done here already https://github.com/cakephp/phinx/pull/1122/files
| @@ -11,6 +11,7 @@ class PDOMock extends \PDO | |||
| { | |||
| public function __construct() | |||
| { | |||
| parent::__construct('sqlite::memory:'); | |||
There was a problem hiding this comment.
I think the problem is here. You are forcing all tests to use sqlite::memory, regardless of what the tests is setup to do.
There was a problem hiding this comment.
Appreciate it Lorenzo, will take a look at those 2 items ASAP
|
@lorenzo I apologize for the delay, finally had some time to look into this again. Requested changes have been made. |
|
@pr0ggy Thanks! |
ghost
deleted the
Fix for the regex issue found (#1086) in
Phinx\Db\Adapter\SQLiteAdapter::dropColumn()which resulted in malformed SQL being executed. Note that included in this fix is a call to set the PDO error mode to throw exceptions in the event of error (malformed SQL being executed, etc) as I believe this will make such errors much more visible in the future (there were some failing unit tests that were covered up and marked as passing because of the exact same problem...malformed SQL being executed but no exceptions being thrown and no error codes checked for). This was done in thePhinx\Db\Adapter\PdoAdapter::setConnection()method, so it will apply to all adapter classes derived from it. I wanted to call this to attention as I'm not 100% certain this omission wasn't by design. Let me know if any issues are found, will get them fixed ASAP. Appreciate the time/effort put into Phinx thus far, I've found it very useful!