Closed mringler closed 1 year ago
Great work!
type="UUID_BINARY"
vs https://github.com/propelorm/Propel2/pull/1915 and type="UUID"
My main question would be: Do we need to have different type names per DB?
From a user perspective the internals (and custom decisions on the specific sub type) are not so interesting usually, so lets say you want to use UUID from a framework perspective and support 3+ DB types
The ORM ideally abstracts the internals away, that you can specify UUID
across your project definitions then and they dont have to be different per DB type.
In other words: Ideally we define
type="UUID"
and by default picks the best internal strategy by default for each ORM DB type supported.
Is that something we can achieve?
Base: 88.44% // Head: 73.25% // Decreases project coverage by -15.19%
:warning:
Coverage data is based on head (
ce0a677
) compared to base (b35fb89
). Patch coverage: 65.73% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Ok, now UUID_BINARY
is used as fallback type for UUID
in MySQL and SQLite (all other vendors have a native UUID type). So now the UUID
type can always be used (even though the result might be surprising).
I think we should keep both types, as it allows to still use UUID_BINARY
on systems where UUID
maps to the native type. This might be useful if an old version of the dbms is used or if the db already uses binary columns and cannot be changed. Also, if you know your database will use UUID_BINARY
anyway, you might find it cleaner to just specify that. And should MySQL ever support a native UUID type and we change the behavior of UUID
, users can switch to UUID_BINARY
if they want to avoid migrating their db.
@dereuromark what do you think does that make sense?
Unsafe usage of new static().
These should be in phpstan ignoreErrors list.
I have added code for automatic migration between CHAR
/VARCHAR
and BINARY
uuid columns on MySQL. If column type is changed in schema.xml, Propel will add code to the migration file, which creates a new column for the converted values and then replaces the old column with that new one.
When changing from a binary column to a CHAR
type, propel does not know that the column contains uuids, it just sees binary to char. To let Propel know that it is a conversion of uuids, the column has to be marked as such by adding the content="UUID"
to the declaration in schema.xml (we can use something else if that is better):
<column name="id" type="varchar" size="36" content="UUID" />
I had to change some overall behavior to get this working, my assumption is that these changes are useful:
ALTER TABLE
statements in migrations, I have moved that code into its own class (AlterTableStatementMerger) and you can add a statement, which tells the merger not to combine statements above with those below (AlterTableStatementMerger::NO_MERGE_ACROSS_THIS_LINE). Also the merger recognizes DML statements and will not merge across them.With the test, I am becoming more confident that this might actually work. Let me know what you think!
This allows Propel to work with UUIDs stored in a binary data column like
BINARY(16)
in MySQL.To use it, the column type in
schema.xml
has to be set toUUID_BINARY
:In models, fields of that type will always contain the UUID as string, conversion between binary value and string is done during loading (in
hydrate()
) or saving (indoInsert()
orbuildCriteria()
fromdoUpdate()
). Similar conversion happens in thefindBy
methods in the query class. This is a different approach than planned in #1914, but I found it integrates better into Propel.Per default, UUID conversion will swap some bytes around, in accordance with MySQL's
uuid_to_bin()
function. This allows better indexing for version-1 UUIDs. The default behavior can be changed inschema.xml
by setting the value ofUuidSwapFlag
in vendor information:Caveats:
UuidSwapFlag
currently does not trigger a migration at all (it should cause all existing UUIDs to be (un)swapped).select()
on the query class or manually setting a formatter likeSimpleArrayFormatter
, will leave UUIDs in their binary form and users have to convert them manually by callingUuidConverter::binToUuid($uuidData, $swapFlag)
(depending on PDO behavior for the underlying dbms, they will have to read the resource/stream into a string first by callingstream_get_contents($uuidData)
).UuidConverter::uuidToBin($uuid, $swapFlag)
.Realistically, this will take a brave user to test and give feedback.