Fix up UUID binary16 support

[dereuromark]

Follow #1734

But doesn't quite fix cakephp/cakephp#17420 yet - also needs cakephp/cakephp#17421 then

[markstory]

What about binary(16)?

[dereuromark]

Isnt Binary always 16 in this case?
https://stackoverflow.com/questions/17277735/using-uuids-in-sqlite#:~:text=SQLite%20allows%20to%20use%20any,small%20that%20the%20difference%20matters).

[markstory]

I'm not able to find any documentation on SQLite's support for uuid_blob, but I do know that sqlite only has 5 real types, and that it will preserve but not enforce column lengths. By mapping binaryuuid to binary(16) in phinx, we would not have to update cakephp as it already detects binary(16) as binaryuuid.

[dereuromark]

What would that change look like?
What type string string should we use here to map?

You just want it to be binary with length of 16?

[dereuromark]

WAH
I accidently pushed directly instead of creating a PR: 5a78c76

We can revert if needed!

[dereuromark]

I checked, and it returns binary without length, as such this is not working out.
How can we also include the length I wonder.

[
  'type' => 'binary',
  'length' => null,
  'null' => false,
  'default' => null,
  'precision' => null,
  'comment' => null
]

[dereuromark]

I must be missing sth
On my local system, in Cake (and thus my app and tests) it always ends up as UUID_BLOB, and still seems to require a change in cakephp/cakephp directly (as in my other PR).
I cannot reproduce the CI result locally, making it hard to debug..

[dereuromark]

What I found working was to set it to binaryuuid

self::PHINX_TYPE_BINARYUUID => 'binaryuuid',

and then change the core code to at least add

        if ($col === 'binaryuuid') {
            return ['type' => TableSchemaInterface::TYPE_BINARY_UUID, 'length' => null];
        }

https://github.com/dereuromark/cakephp-sandbox/actions/runs/6864095226/job/18665132299

Also note that pgsql also has still "uuid" only here:
https://github.com/dereuromark/cakephp-sandbox/actions/runs/6864095226/job/18665133009#step:11:25
But the test doesnt fail, interesting enough.

[dereuromark]

Now I understand: You mean literally binary(16). Didnt know that works.

[dereuromark]

I can confirm that works!
Only Postgres is still a bit questionable. No errors, but it seems to be using the Uuid type instead of BinaryUuid type.

[
  'type' => 'uuid',
  'length' => null,
  'default' => null,
  'null' => false,
  'comment' => null,
  'precision' => null
]

Since it doesnt fail, maybe its OK
The entity itself contains

'uuid' => 'eb25610d-7bfa-4e34-812c-ad72b100fb26'

when reading a record.

I wonder if the actual DB column here is using binary16, it seems uuid is a native type.
So we probably dont have to care about it and all is good.

[dereuromark]

Confirmed to be working I will squash merge this then, as the current one in master is faulty.

[MasterOdin]

I don't think using binary(16) as the type works fully as we'd want for sqlite? Since that's not a real type in sqlite, it uses the rules of type affinity, which in this case I think means it'll end up having the INT affinity as it doesn't have any of the landmark strings to look for, but does have the type defined. I think we need to have blob somewhere in the string for this to have the right affinity.

[dereuromark]

@MasterOdin What value would you then consider to be working here? uuid_blob?
That would definitly need a CakePHP core adjustment on top.

Or should we go with the binaryuuid way then and a smaller adjustment in CakePHP core?

[MasterOdin]

Yeah, uuid_blob as that'd be symmetrical to the uuid_text type. binaryuuid wouldn't work for the same reason I don't think binary(16) does, it doesn't contain the word "blob" (or any of the other strings looked for) so the affinity would end up being INT.

Though, does CakePHP core need anything special for that or since it's just text, it's fine?

[dereuromark]

It wouldn't map it to "binaryuuid" or "binary" with length 16 and as such to the correct BinaryUuid type class
See https://github.com/cakephp/cakephp/pull/17421/files

Or is there a way to map it to "binary" and the length of 16 for Sqlite? I only saw these options for mysql etc.
If we can, it would auto detect it again, yes.

[MasterOdin]

Looking at that, it's already doing a bit of special logic for text_uuid:

        if (($col === 'char' && $length === 36) || $col === 'uuid') {
            return ['type' => TableSchemaInterface::TYPE_UUID, 'length' => null];
        }

where the regex gives:
[1] => 'text', [2] => 'uuid', so $col === 'uuid' matches for that. This then gives precedent to not need binary(16) in the same way we don't have char(36) for the text UUID. As such, I think that phinx should have a PR to rename the type to blob_uuid and then core has a PR that does something like:

        if ($col === 'binary' && $length === 16 || (str_contains($column, 'blob') && $col === 'uuid')) {
            return ['type' => TableSchemaInterface::TYPE_BINARY_UUID, 'length' => null];
        }
        if (($col === 'char' && $length === 36) || $col === 'uuid') {
            return ['type' => TableSchemaInterface::TYPE_UUID, 'length' => null];
        }

[dereuromark]

I opened the PRs
dereuromark/cakephp-sandbox#69 shows that it works as expected