Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[PostgresPlatform] Fixing change detection when a default is removed #44

Closed
wants to merge 1 commit into from

3 participants

Richard Fullmer Jörn Friedrich Dreyer Steve Müller
Richard Fullmer

Change detection when the default value is removed from a field is presently broken and produces invalid SQL.

This patch checks to see if a new default value actually exists before adding the SET DEFAULT '';

Uses DROP DEFAULT when the new default value does not exist

Jörn Friedrich Dreyer

@richardfullmer we are stumbling over this in ownCloud: owncloud/core#6310
Why did you close the PR. Is it the wrong approach?

Jörn Friedrich Dreyer

Also summoning @deeky666: we are stumbling over this in ownCloud: owncloud/core#6310
Why did you close the PR. Is it the wrong approach? http://www.doctrine-project.org/jira/browse/DBAL-156 gives no reason.

Jörn Friedrich Dreyer butonic referenced this pull request in owncloud/3rdparty
Closed

Fix SET DEFAULT when there is no default #64

Steve Müller
Collaborator

@butonic I have no idea why @richardfullmer closed this. This PR is really old. I will try to have a look into this later this evening and supply a patch for this issue.

Jörn Friedrich Dreyer

@deeky666 I just tested the migration of OC5 to OC6 (where we start using doctrine) and with this PR applied it completes fine. We are asking the database to describe its schema and I think postgresql reports DEFAULT '' when no default is set ... or doctrine interprets whatever postgres reports as such. So this might be fixing a symptom ...

Jörn Friedrich Dreyer

looking at the code it might as well be that https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php#L387 always tries to return a default value ... but then again all other platforms also always return a default. MySql then uses CHANGE, Oracle uses MODIFY and thus they do not not have the SQL syntax issues PostgreSqlPlatform runs into. From what I see I think adding the 'default' value to the options array in the _getPortableTableColumnDefinition implementations should only be added when a default value is specified ...

Richard Fullmer

Why did you close the PR

Because it sat for over a year without being commented on or addressed. I've maintained my own fork in the mean time. I'd assumed that I was the only one who had the issue.

Jörn Friedrich Dreyer

@richardfullmer nah ... we ran into it with a vengance... it breaks the migration to OC6 for all postgresql users of owncloud. Sadly, nobody seems to have tested it ... oh well ...

Steve Müller
Collaborator

Fix included in PR #456.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 3 additions and 1 deletion.
  1. +3 −1 lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
4 lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
View
@@ -400,7 +400,9 @@ public function getAlterTableSQL(TableDiff $diff)
$sql[] = 'ALTER TABLE ' . $diff->name . ' ' . $query;
}
if ($columnDiff->hasChanged('default')) {
- $query = 'ALTER ' . $oldColumnName . ' SET ' . $this->getDefaultValueDeclarationSQL($column->toArray());
+ $query = 'ALTER ' . $oldColumnName . ((null !== $column->getDefault())
+ ? ' SET ' . $this->getDefaultValueDeclarationSQL($column->toArray())
+ : ' DROP DEFAULT');
$sql[] = 'ALTER TABLE ' . $diff->name . ' ' . $query;
}
if ($columnDiff->hasChanged('notnull')) {
Something went wrong with that request. Please try again.