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

uuid models support #2310

Open
wants to merge 10 commits into
base: 3.x
Choose a base branch
from
Open

uuid models support #2310

wants to merge 10 commits into from

Conversation

yamaha252
Copy link
Contributor

Hi!
As I can see there is no big reason not to support model uuid primary keys.
It's always annoying I have to use autoincrement int if I want to use the model in Twill.

@Tofandel
Copy link
Contributor

Tofandel commented Feb 27, 2024

Maybe adding a feature config option for this? And use it for the migration, there is also the model helpers that use id() that could be improved based on this option

@ifox
Copy link
Member

ifox commented Feb 27, 2024

I'm not sure about this because uuid is not necessarily great to use as a primary key, it is more recommended to use a separate column for it. I understand the primary use case is to hide the id from URLs to prevent enumeration, so I'm all for adding support to resolve Twill models through a UUID, but I don't think we need to change all our methods definitions to support that.

@Tofandel
Copy link
Contributor

Tofandel commented Feb 27, 2024

I also agree on this, it's simpler and more efficient to use id with mysql, but the one really big advantage of uuid is that you can export import content without worrying about conflicts (eg merging multiple databases) but yes in most cases it's better to put both id and uuid and change the model's route key and put id as hidden

@yamaha252
Copy link
Contributor Author

My opinion is it's better for the project to be quite flexible considering this feature not that hard to maintain. If anybody prefer to use uuid as a primary key, why not?
My case is adding existing models in twill and otherwise I have to migrate all the data in them.
Feature config option sounds good if changing subject column type can cause poor performance for the activity log.

@ifox
Copy link
Member

ifox commented Feb 29, 2024

Yes, I agree, we can add it as an optional feature.

@ifox
Copy link
Member

ifox commented May 24, 2024

Hi @yamaha252 I'm ready to accept and merge this but there are more tables that use integer columns for id references that would need to be changed (twill_mediables, twill_fileables, twill_related, twill_blocks primarily).

@yamaha252
Copy link
Contributor Author

Thanks @ifox, done
What do you think guys, column type varchar(36) is ok for every sort of keys types or strict uuid is better?

@ifox
Copy link
Member

ifox commented Jun 2, 2024

Yes, I think that's good, as it'll supprt both UUID and ULID.

@ifox ifox added this to the Twill 3.4 milestone Jun 4, 2024
@ifox ifox added the type: enhancement New feature or request label Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

3 participants