laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.62k stars 11.03k forks source link

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

Closed chrisflorin closed 1 day ago

chrisflorin commented 2 days ago

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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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 commented 16 hours ago

@Jubeki

Jubeki commented 14 hours ago

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).