cakephp / phinx

PHP Database Migrations for Everyone
https://phinx.org
MIT License
4.46k stars 892 forks source link

Fix up UUID binary16 support #2239

Closed dereuromark closed 10 months ago

dereuromark commented 10 months ago

Follow https://github.com/cakephp/phinx/pull/1734/

But doesn't quite fix https://github.com/cakephp/cakephp/issues/17420 yet - also needs https://github.com/cakephp/cakephp/pull/17421 then

dereuromark commented 10 months ago

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

MasterOdin commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

I opened the PRs https://github.com/dereuromark/cakephp-sandbox/pull/69 shows that it works as expected