-
Notifications
You must be signed in to change notification settings - Fork 398
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
Add support for native UUID column type #1915
Add support for native UUID column type #1915
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1915 +/- ##
============================================
+ Coverage 88.41% 88.44% +0.02%
- Complexity 7849 7854 +5
============================================
Files 224 224
Lines 20981 20988 +7
============================================
+ Hits 18551 18562 +11
+ Misses 2430 2426 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
e7273d1
to
1dad2e8
Compare
Do we have a way to update docs here accordingly, so it is clear what to expect and what support across the DB types has been achieved yet? |
Not sure, the only doc for Propel 2 that I know of is at http://propelorm.org/documentation/, but I am not really familiar with it |
Right, we probably want to add UUID info at https://github.com/propelorm/propelorm.github.com then to be published after we merged. |
if ($fromSqlType === 'DATE' && $toSqlType === 'TIME') { | ||
return ' USING NULL'; | ||
} |
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.
This DATE and TIME handling is dropped, why?
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.
The function returns USING NULL
per default, so this was just an extra step to get to the same result.
Small conflict then we should be good to go |
Oh, hadn't realized, thank you for the notice! |
46dc313
to
1db89c3
Compare
As discussed in #1914, this allows to use
UUID
as column type in schema.xml. Underlying columns are set to the native uuid type of the database system (uuid
in Postgres and Oracle,UNIQUEIDENTIFIER
in mssql). In Mysql/MariaDB and SQLite, an error is thrown. Inside the model and query classes, the uuid columns use string values.This is pretty straight forward, a new type literal is registered and associated with strings, and register the type in the vendor adapters with the corresponding type. For Postgres, I added cast statements used during migration. Tests check generated
create
andalter
statements and internal mapping of uuid to string.I have played around with it on Postgres, and it seems to work there.
I also added a commit with two fixes for the test setup scripts:
setup.mysql.sh
always used "test" as database name, even though the fixtures are build using an environment variable ($DB_NAME
) with "test" only as fallbacksetup.pgsql.sh
some statements failed when a variable was empty, leading to empty string parameters