Don't add 'NOT NULL' to the 'ALTER TABLE' when that hasn't changed #360

Merged
merged 1 commit into from Aug 24, 2013

Projects

None yet

5 participants

@bartv2
Contributor
bartv2 commented Aug 23, 2013

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-583

We use Jira to track the state of pull requests and the versions they got
included in.

@guilhermeblanco guilhermeblanco and 1 other commented on an outdated diff Aug 23, 2013
lib/Doctrine/DBAL/Platforms/OraclePlatform.php
@@ -629,7 +629,11 @@ public function getAlterTableSQL(TableDiff $diff)
* Do not add query part if only comment has changed
*/
if ( ! ($columnHasChangedComment && count($columnDiff->changedProperties) === 1)) {
- $fields[] = $column->getQuotedName($this). ' ' . $this->getColumnDeclarationSQL('', $column->toArray());
+ $columnInfo = $column->toArray();
+ if (!in_array('notnull', $columnDiff->changedProperties)) {
guilhermeblanco
guilhermeblanco Aug 23, 2013 Owner

This block should be:

$columnInfo = $column->toArray(); 

if ( ! in_array('notnull', $columnDiff->changedProperties)) {
    $columnInfo['notnull'] = false; 
}

$fields[] = $column->getQuotedName($this). ' ' . $this->getColumnDeclarationSQL('', $columnInfo);
deeky666
deeky666 Aug 23, 2013 Member

What about this:

if ( ! $columnDiff->hasChanged('notnull')) {
    $columnInfo['notnull'] = false;
}

Using the encapsulated class method seems better IMO :)

Owner

Also, can we have a test that covers this situation?

@deeky666 deeky666 commented on an outdated diff Aug 23, 2013
lib/Doctrine/DBAL/Platforms/OraclePlatform.php
@@ -629,7 +629,11 @@ public function getAlterTableSQL(TableDiff $diff)
* Do not add query part if only comment has changed
*/
if ( ! ($columnHasChangedComment && count($columnDiff->changedProperties) === 1)) {
- $fields[] = $column->getQuotedName($this). ' ' . $this->getColumnDeclarationSQL('', $column->toArray());
+ $columnInfo = $column->toArray();
+ if (!in_array('notnull', $columnDiff->changedProperties)) {
+ $columnInfo['notnull'] = false;
+ }
+ $fields[] = $column->getQuotedName($this). ' ' . $this->getColumnDeclarationSQL('', $columnInfo);
deeky666
deeky666 Aug 23, 2013 Member

There is one space missing before the first concatenation ..

Contributor
bartv2 commented Aug 23, 2013

@guilhermeblanco @deeky666 thanks for the review, is this better?

@guilhermeblanco guilhermeblanco merged commit 7ffb270 into doctrine:master Aug 24, 2013

1 check passed

default The Travis CI build passed
Details
@bartv2 bartv2 deleted the unknown repository branch Aug 24, 2013
@AdamWill

Can this please be pulled into the 2.4 branch, or is it not considered appropriate? Trying to get all of OwnCloud's downstream patches to DBAL pulled back into upstream 2.4 so we (Fedora) can hopefully ship OC using an un-patched distro copy of DBAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment