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

Set default column name to 'id' for all Increments functions #53590

Closed
wants to merge 1 commit into from

Conversation

chrisflorin
Copy link
Contributor

Set the default column name for all Increments functions to 'id.'

The benefit of this pull request to end users are:

  1. The default of 'id' of an increment column is encouraged throughout the project.
  2. Users no longer need to specify the column name when using an increments function other than the id() function.

This does not break any existing features because there is currently no default, so any existing implementations will already specify what the application needs.

@Jubeki
Copy link
Contributor

Jubeki commented Nov 20, 2024

What exactly is your purpose of your PR? Because I don't really see a need for the default value, as they are not a sensible default in my opinion.

Like you said, there already is an id() method and if you need a foreign key, you always have a prefix: e.g. user_id.

I never used any other method than id() for creating an id column. Though some people use uuid for an id, than these changes make even less sense to me.

@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 applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions!

@chrisflorin
Copy link
Contributor Author

chrisflorin commented Nov 20, 2024

The idea is that sometimes users may not want a big integer for their primary id column. I created a table where I wanted a tinyint for my primary column. This pull request isn't to modify user_id, or other foreign keys. It has to do with the primary key of a table.

The default create table migration basically contains the following:

        Schema::create('table_name', function (Blueprint $table) {
            $table->id();
            $table->timestamps();
        });

The id() function creates an auto incrementing big integer column named "id" in the table. If I don't want a big integer, but rather a tinyint, or mediumint, I need to change to the following:

        Schema::create('table_name', function (Blueprint $table) {
            $table->tinyIncrements('id');
            $table->timestamps();
        });

where I have to specify the column name "id" because it's not default there. My changes would allow the following:

        Schema::create('table_name', function (Blueprint $table) {
            $table->tinyIncrements();
            $table->timestamps();
        });

without specifying the "id" column name. The "id" column name is a Laravel default convention and for all users/applications that are specifying a column name will not see a change in behavior and can continue to do so if they're not following the default convention of using "id" as the auto incrementing column in their application.

     * Create a new auto-incrementing big integer (8-byte) column on the table.
     *
     * @param  string  $column
     * @return \Illuminate\Database\Schema\ColumnDefinition
     */
    public function id($column = 'id')
    {
        return $this->bigIncrements($column);
    }

This is the default for the id() function; and maybe you use "user_id" as the primary key in your users table, but the default Laravel convention is that the primary key is "id."

I never used any other method than id() for creating an id column.

uuid exception noted, but if Laravel is based on only your use case, why not remove the other increment functions?

This Pull Request has nothing to do with foreign keys.

@chrisflorin
Copy link
Contributor Author

@Jubeki

@Jubeki
Copy link
Contributor

Jubeki commented Nov 21, 2024

Sorry, you are right, i doesn't make sense to use autoincrement columns in the case of foreign keys.

In that case your changes make a lot more sense. Though I would still recommend using the "normal" id() method, because you don't know what changes may occur (Though it can be a little more performant / less memory if you reference the smaller column as a foreign key).

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