-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle default values for datetime/datetimetz columns in PostgreSQL #785
Conversation
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DBAL-1131 We use Jira to track the state of pull requests and the versions they got |
@Ocramius re-submitted as requested; thanks and sorry for the problems! |
What problems? ;-) |
@@ -378,6 +378,12 @@ protected function _getPortableTableColumnDefinition($tableColumn) | |||
case 'year': | |||
$length = null; | |||
break; | |||
case 'timestamp': | |||
case 'timestamptz': | |||
if ($tableColumn['default'] == 'now()') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see now()
in the test cases: is it a missing test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is kind of subtle; the tests will create a new table using the mapping of CURRENT_TIMESTAMP
, and then the schema manager will call listTableDetails()
on the database itself, which will report the column as having a default of now()
(e.g. not null default now()
). So the actual change in this PR is recognizing that "now()" is the same as the platform's getCurrentTimestampSQL()
.
The reason the tests must use CURRENT_TIMESTAMP
and not NOW()
is that it must match what is in AbstractPlatform::getCurrentTimestampSQL()
.
Just wanted to check up on this to see if there was anything else needed of me on it -- it's on my open to do list. :) |
09cf942
to
c9b6384
Compare
@deeky666 I have rebased this to the current master, and I think it is ready for merging. Note that I did need to add an explicit During testing the rebase I also logged DBAL-1274 as I realized that all of the Travis builds seemed to be testing against PostgreSQL 9.4. I proposed a fix in there, but not sure of the impact. |
Closing as stale. |
(This was originally in #670 but am re-submitting due to a busted merge)
When dealing with legacy schema it would be nice to be able to map default values and not have the schema spit out ALTER statements each time. This works correctly today for basic integer and string columns, but does not handle columns with special types such as boolean or datetime.
For example, the following statement:
ALTER TABLE test_table ALTER test_column SET DEFAULT CURRENT_TIMESTAMP;
Results in a column definition like:
test_column | timestamp with time zone | not null default now()
However, repeating the same schema generation will result in the same ALTER statement each time, because it will always detect that the default value has changed. The same is true for boolean columns.
This simple change prevents this situation from happening and correctly detects that the column default has not changed. It is specific to PostgreSQL.