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

[9.x] Option to set UUID/ULID as primary key on migration and model files #45350

Closed
wants to merge 9 commits into from

Conversation

morcen
Copy link

@morcen morcen commented Dec 18, 2022

Since #44146 we now have options to use UUID and ULID as the table's primary key. This PR aims to assist developers in setting up UUID or ULID during migration and model generation, following the instructions of the official documentation.

How to use:

Use the option --primary (or shortcut -P) to define the type of primary key of the migration file or model being created.

Screenshot 2022-12-19 at 11 17 03 pm

For example:

  • To generate a migration file that creates the table books with primary key type UUID:
    php artisan make:migration create_books_table -Puuid
    Screenshot 2022-12-19 at 11 33 44 pm
    Screenshot 2022-12-19 at 11 36 43 pm

  • To generate a Customer model that also creates the migration file with primary key type ULID:
    php artisan make:model Customer -mPulid
    Screenshot 2022-12-19 at 11 34 28 pm
    Screenshot 2022-12-19 at 11 35 43 pm
    Screenshot 2022-12-19 at 11 35 56 pm

More examples

  • Generate the model Student with -a (all) options, and set the primary key to UUID:
    php artisan make:model Student --all --primary=uuid
    Screenshot 2022-12-19 at 11 42 07 pm
    Screenshot 2022-12-19 at 11 42 30 pm
    Screenshot 2022-12-19 at 11 43 07 pm

Changes in this PR will not affect those who wish to use the default auto-incrementing integer as the primary key, as simply omitting the primary option (or leaving it empty) will result in the default PK.

@morcen morcen changed the title [9.x] Option to set UUID as primary key on migration and model files [9.x] Option to set UUID/ULID as primary key on migration and model files Dec 18, 2022
return 'ulid(\'id\')';
}

return 'id()';
Copy link
Member

Choose a reason for hiding this comment

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

why not just utilise Illuminate\Database\Schema\Blueprint::$defaultMorphKeyType

Copy link
Author

Choose a reason for hiding this comment

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

thanks @crynobone , I will look into utilising that instead

Copy link
Author

@morcen morcen Dec 19, 2022

Choose a reason for hiding this comment

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

@crynobone, how do you suggest the Blueprint::$defaultMorphKeyType be used in this section? I used the Blueprint class to simplify all other approaches I initially did (thanks to you!), but I'm out of idea on how to make use of the said class in the line you commented on.

Copy link
Member

Choose a reason for hiding this comment

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

I believe it should 100% rely on Blueprint::$defaultMorphKeyType without adding new --primary or --key options.

@@ -18,7 +19,8 @@ class MigrateMakeCommand extends BaseCommand
{--table= : The table to migrate}
{--path= : The location where the migration file should be created}
{--realpath : Indicate any provided migration file paths are pre-resolved absolute paths}
{--fullpath : Output the full path of the migration}';
{--fullpath : Output the full path of the migration}
{--P|primary=int : Set default primary key type for the table <info>[int, uuid, ulid]</info>}';
Copy link
Author

@morcen morcen Dec 19, 2022

Choose a reason for hiding this comment

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

--key can also be used and shorter but I don't find it as descriptive as --primary. Thoughts?

@morcen morcen marked this pull request as ready for review December 19, 2022 16:00
@@ -72,6 +74,8 @@ public function handle()

$create = $this->input->getOption('create') ?: false;

Builder::defaultMorphKeyType($this->option('primary') ?: 'int');
Copy link
Member

Choose a reason for hiding this comment

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

No, the command shouldn't change application defaults. The command should only use the default settings

return 'ulid(\'id\')';
}

return 'id()';
Copy link
Member

Choose a reason for hiding this comment

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

I believe it should 100% rely on Blueprint::$defaultMorphKeyType without adding new --primary or --key options.

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants