Skip to content
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

Start adding binaryuuid support. #1734

Merged
merged 4 commits into from
Apr 11, 2020
Merged

Start adding binaryuuid support. #1734

merged 4 commits into from
Apr 11, 2020

Conversation

dereuromark
Copy link
Member

First stab at #1732

I might need some help, as I am not too familiar with the lower levels here.

@dereuromark dereuromark added this to the 0.12 (PHP 7.2+) milestone Apr 10, 2020
@dereuromark
Copy link
Member Author

This now works for MySQL already:

CREATE TABLE `uuid16_test_rows` (
  `id` binary(16) NOT NULL,
  `name` varchar(100) COLLATE utf8_unicode_ci NOT NULL DEFAULT '',
  `foreign_key_without_index` binary(16) DEFAULT NULL,
  `foreign_key_with_index` binary(16) DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;

ALTER TABLE `uuid16_test_rows`
  ADD KEY `foreign_key_with_index` (`foreign_key_with_index`);
COMMIT;

@dereuromark
Copy link
Member Author

dereuromark commented Apr 10, 2020

Tests are green so far except SQLite

SQlite has some issues still
"General error: 1 near "user_id": syntax error"

uuid

CREATE TABLE `tmp_ztable` 
(`id` UUID_TEXT NOT NULL, PRIMARY KEY (`id`), `user_id` INTEGER NOT NULL)

binaryuuid

CREATE TABLE `tmp_ztable` 
(`id` BINARY_BLOB NOT NULL, PRIMARY KEY (`id`), `user_id` INTEGER NOT NULL)

@dereuromark
Copy link
Member Author

The way SQLite creates those primary keys seems to be the issue

CREATE TABLE tmp_ztable
( id BINARY_BLOB NOT NULL PRIMARY KEY,
  user_id INTEGER NOT NULL
);

would be better

but that is a different topic and not directly related to the changes here.
So all good it seems.

I tested it also with CakePHP and migrations plugin, which now works correctly for all 3 db types: cakephp/migrations#331

@MasterOdin
Copy link
Member

For SQLServer, binaryuuid would probably map to the same type as static::PHINX_TYPE_UUID, namely uniqueidentifier, though like the postgresql adapter, you would not be able to do a comparison against static::PHINX_TYPE_BINARYUUID on saved columns in a table.

@codecov
Copy link

codecov bot commented Apr 11, 2020

Codecov Report

Merging #1734 into master will increase coverage by 0.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1734      +/-   ##
============================================
+ Coverage     76.78%   76.79%   +0.01%     
- Complexity     2026     2030       +4     
============================================
  Files            57       57              
  Lines          5902     5909       +7     
============================================
+ Hits           4532     4538       +6     
- Misses         1370     1371       +1     
Impacted Files Coverage Δ Complexity Δ
src/Phinx/Db/Adapter/SQLiteAdapter.php 97.74% <ø> (ø) 197.00 <0.00> (ø)
src/Phinx/Db/Adapter/MysqlAdapter.php 90.81% <80.00%> (-0.10%) 231.00 <0.00> (+3.00) ⬇️
src/Phinx/Db/Adapter/PostgresAdapter.php 93.97% <100.00%> (+0.01%) 229.00 <0.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbe74fb...0e953b5. Read the comment docs.

@dereuromark
Copy link
Member Author

@MasterOdin For SqlServer it seems like UNIQUEIDENTIFIER could be the one to be used.
Do you want to look into this?
I think for the main DBs this support is sufficient for now.

@@ -56,6 +56,7 @@ class SQLiteAdapter extends PdoAdapter
self::PHINX_TYPE_TEXT => 'text',
self::PHINX_TYPE_TIME => 'time_text',
self::PHINX_TYPE_UUID => 'uuid_text',
self::PHINX_TYPE_BINARYUUID => 'binary_blob',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems to be the problem/wrong. See https://github.com/cakephp/migrations/issues/660

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uuid_blob is the correct one it seems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants