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

Added nativeuuid type for databases with a native uuid type #786

Merged
merged 21 commits into from
Dec 27, 2024

Conversation

nicosp
Copy link

@nicosp nicosp commented Dec 10, 2024

This is to allow the use of the native uuid for mariadb 10.7+ users. For other databases it's simply an alias to the existing uuid type. Sqlite is explicitly not supported because it doesnt have a native type.

Related Issue: cakephp/phinx#2256

Note: To test this you need an to add 'nativeuuid' to Cake\Database\TypeFactory $_types.

Once this is accepted I can also do the change in CakePHP.

MariaDB in CI

To actually test this, MariaDB was added to the CI and some MariaDB-specific issues were uncovered:

  • GIS types are not working for default values (The tests are skipped for MariadB)
  • on update for timestamps was not properly detected from the schema. FIXED

@ADmad
Copy link
Member

ADmad commented Dec 10, 2024

What wonderful consistency exists between db engines that we need 3 constants/types for UUID fields :)

@nicosp
Copy link
Author

nicosp commented Dec 10, 2024

What wonderful consistency exists between db engines that we need 3 constants/types for UUID fields :)

The data migration for this is also going to be fun because uuids are used for primary keys etc.

@nicosp
Copy link
Author

nicosp commented Dec 11, 2024

The PR is ready. The only thing I would have liked was to have phpunit also test MariaDB 10.7. If this is something we want I can try to adjust the ci pipeline to also have "mariadb" in the db matrix.

@ADmad
Copy link
Member

ADmad commented Dec 11, 2024

If this is something we want I can try to adjust the ci pipeline to also have "mariadb" in the db matrix.

Having mariadb in the CI matrix sounds good. We do have it for the core repo.

@nicosp
Copy link
Author

nicosp commented Dec 11, 2024

Looks like I opened a can of worms with MariaDB testing :)
GIS defaults are broken for sure and some tests need some massaging to pass.

@nicosp
Copy link
Author

nicosp commented Dec 11, 2024

The PR is ready.

@@ -51,6 +51,7 @@ interface AdapterInterface
public const PHINX_TYPE_JSON = 'json';
public const PHINX_TYPE_JSONB = 'jsonb';
public const PHINX_TYPE_UUID = 'uuid';
public const PHINX_TYPE_NATIVEUUID = 'nativeuuid';
Copy link
Member

Choose a reason for hiding this comment

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

I'm hoping to remove all of these constants in the future and use the abstract types defined in CakePHP. We'll have to make sure that the same abstract type names are used so that we don't break compatibility.

@LordSimal
Copy link
Contributor

I don't think this should go into 4.x but rather 4.next since this is a new feature, not a bugix.

@markstory markstory changed the base branch from 4.x to 4.next December 27, 2024 18:36
@markstory markstory merged commit 490f20a into cakephp:4.next Dec 27, 2024
22 checks passed
@markstory
Copy link
Member

Thank you 🎉

markstory added a commit that referenced this pull request Dec 27, 2024
@nicosp nicosp deleted the nativeuuid branch December 30, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants