-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add Support to ULID
column type
#3699
base: main
Are you sure you want to change the base?
Add Support to ULID
column type
#3699
Conversation
Hey, @freekmurze ! I'd like to know your opinion on this before I continue with the PR :D There doesn't seem to be any breaking changes, but I'd like your opinion on whether I should move forward because it's something interesting in your eyes. |
@devajmeireles please feel free to continue with this PR. I'm happy to review it once it's ready 👍 Thank you! |
Hey, @patinthehat ! Thanks for your feedback. I think the PR is ready for review. Feel free to review it and point out anything misaligned or incorrect so I can adjust it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the docs to explain the new configuration setting/environment variable. Other than that and one other minor change, looks good!
Hey, @patinthehat ! I applied all your suggestions. I also tried searching for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devajmeireles Thanks for the updates! Would you mind adding an additional markdown file in "advanced usage" that explains the configuration option and what it does when true
and false
?
@@ -14,6 +14,11 @@ | |||
*/ | |||
'max_file_size' => 1024 * 1024 * 10, // 10MB | |||
|
|||
/* | |||
* Determiner if the media table should use ULID instead of UUID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Typo in "Determine" 😄
@@ -51,6 +51,11 @@ return [ | |||
* Adding a larger file will result in an exception. | |||
*/ | |||
'max_file_size' => 1024 * 1024 * 10, | |||
|
|||
/* | |||
* Determiner if the media table should use ULID instead of UUID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: typo
Based on several studies and personal tests, we can prove the effectiveness of
ulid
columns instead ofuuid
when talking about a large volume of data.A package like this tends to persist a thousand of data in the database since all the media of an entire application is persisted in a table. Some public studies point to abstracts where ULID is advantageous in these cases.
A good read is here: https://medium.com/@sammaingi5/uuid-vs-ulid-how-ulid-improves-write-speeds-d16b23505458
This PR is an attempt to implement a configuration that allows ULID to be used instead of UUID.