Composite primary key support

[Failure22]

For now, Laravel supports composite primary keys in migrations, but they're not completely supported in models. For example, if you have model that contains public $incrementing = false; public $primaryKey = ['fieldA', 'fieldB']; it will cause an error during createOrUpdate operaion (need to check out others). That issue solved by 3rd party packages, overriding protected setKeysForSaveQuery and getKeyForSaveQuery methods. In my opinion, that feature is important enough to pay more attention at core eloquent package, without need to require additional packages

[sisve]

There's lots to do except those two methods you mentioned; Model::getKeysForSaveQuery and Model::getKeyForSaveQuery.

Modifying the type of the primaryKey will change the accepted types for Model::getKeyName and Model::setKeyName. So, we can probably look into the usage of Model::getKeyName to figure which other places needs to be updated to support composite keys too. The Model::getKey method will now return an array, affecting more code.

1. Everything doing ->where($model->getKeyName(), $model->getKey()) will break. This includes Eloquent\Collection::find, Eloquent\Collection::fresh, Model::incrementOrDecrement and Model::fresh.
2. Lots of other methods in the Model class assumes there's only one key. These needs to be updated.
3. Everything involving relations. The HasRelationships trait calls getKeyName in several places.
4. Authenticatable::getAuthIdentifierName will now return multiple key names, which will affect Authenticatable::getAuthIdentifier which is only returning one value. This will affect every user provider.
5. Url generation and model binding needs to be updated since Model::getRouteKeyName and Model::getRouteKey may now return several values.
6. This would also affect the SerializesAndRestoresModelIdentifiers when serializing model references for the queue system.

It's absolutely possible to do. But this is way bigger than just modifying those two mentioned methods.

[Failure22]

@sisve That's why 3rd party packages can't provide complete solution, and that's why I'm talking about importancy of adding this to core, imho. About complexity. I wasn't talking about the solution is modifying theese two methods, I was talking about things I've detected for myself so far, and ye, that's need a deeper inspection. If in general this idea will be approved by maintainers, I can try to find out usages and work on this issue, but I'll need some direct feedback from someone from maintainers/contributors team while working on it. And ye, it will take much time to resolve

Btw, is feature-dependend code test-covered completely? Will test results be illustrative enough?

Thx

[sisve]

I like the idea of having discussions or work logged in something persisted as GitHub, but you'll probably get better discussions on Discord (https://discordapp.com/invite/mPZNm7A) or Slack (https://larachat.co/register).

I'm not sure what you mean with "feature-dependent code", but I know that there are several parts of the framework that aren't fully covered, so the answer is probably not the one you want to hear. ;)

[gareth-ib]

Any progress on this discussion? I'm trying to support a composite table as well. Adding a single primary key would put bloat on my tables I don't want to do. I can help with the dev but I'd like to know it's not going to get blocked on merger first :)

example:

CREATE TABLE visitor_personal_log.config_source (
    user_id            int4 NOT NULL,
    config_site_id     int4 NOT NULL,
    config_source_id   int4 NOT NULL,
    date_used          date NOT NULL DEFAULT now(),
    used_count         int2 NOT NULL,
    PRIMARY KEY (user_id, config_site_id, config_source_id, date_used),
    FOREIGN KEY (config_site_id) REFERENCES visitor_global.config_site(config_site_id),
    FOREIGN KEY (config_source_id) REFERENCES visitor_global.config_source(config_source_id)
);

[prezire]

I think convenience makes this very tedious to implement since the core is tightly coupled to just a single primary key structure. Instead of modifying the current structure, why not extend, or create methods related to composites? How about something like $model->findByComposites([...]), Model::getRouteCompositeKeys(), ->where($model->getCompositeKeyNames(), $model->getCompositeKeys()), etc?

[gareth-ib]

@prezire because then the extension will end up needing more effort to maintain as internals change. And making it be a requirement to have a special method in order to use a model based on it's structure is extra complexity. Even if it's possible to do, with how many private/protected methods exist and are necessary. Would end up basically rebuilding the model system as a copy.

[gareth-ib]

Laravel already supports creating composite primary keys... straight from the docs on https://laravel.com/docs/6.x/migrations

$table->primary(['id', 'parent_id']);

But then for some reason eloquent doesn't support actually using that key it created. I'd definitely mark this as an error, it's far beyond being considered a feature request

[palypster]

@gareth-ib Not sure if Taylor really seees this as an error (reading couple of previouse threads i.e. https://github.com/laravel/framework/issues/5355#issuecomment-115426172). Anyway, is there any progress on adding composite primary keys support for eloquent?

[gareth-ib]

@palypster implementing it would be easy... but spending the time doing it just to have it rejected from merger would be a total waste of time. Get someone to confirm they'd accept it if we properly implemented it. then we (me, whoever) can do it no problem.

[rcapp]

Hello everyone!

I would love to see this implemented. I've read threads from 5 years ago and some more recent: I agree with those saying this should be implemented because we can't always add columns to databases, nor that this should be necessary - we aren't wishing a niche feature from a niche database system to be implemented, but rather a key concept (no pun intended) on relational databases.

Also, having an ID column just for the sake of having a PK sounds a lot like a careless student doing their homework in the nick of time. At last, we can see there would be people from the community to help implementing this.

Take care, you all!

[galloaleonardo]

Hello there, same problem here! :disappointed: I would love to see this implemented too, but while this is not resolved, wich 3rd party packages resolves this?

[prezire]

I think I might have heard Taylor saying that he'll look into feature requests, based on the number of Likes they have. So try liking the original request. Hope it gets noticed :)

[Stetzon]

2021 checking in and would still be a nice feature to have ;)

[prezire]

2021 checking in and would still be a nice feature to have ;)

@Stetzon Can you think of ways to get more votes for the original post? The feature would be very helpful indeed.

[erickr]

We've seen overriding setKeysForSaveQuery() to be successful using models for tables with composite keys. Something like:

    /**
     * Added to support the composite primary key key_1+key_2
     * Notice: Type declarations for method excluded because, it
     * interfers with original declaration in Model.php
     *
     * @param Builder $query
     * @return Builder
     */
    protected function setKeysForSaveQuery($query)
    {
        $query->where('key_1', '=', $this->key_1)
            ->where('key_2', '=', $this->key_2);

        return $query;
    }

A related problem is that it is not possible to define a relationship to this table using the hasOne/hasMany for example. This seems to be solved partly by this external library[1], but a solution in the core would be preferrable. Is there anyone working on this?

Example of how the external library[1] looks:

    public function relatedModels()
    {
        return $this->hasMany('relatedModels', ['foreignKey1', 'foreignKey2'], ['localKey1', 'localKey2']);
    }

1. https://github.com/topclaudy/compoships

[akbarjimi]

Half of 2021 is passed and still nothing happened