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

Eloquent should support composite keys #5355

Closed
aebersold opened this issue Aug 6, 2014 · 84 comments
Closed

Eloquent should support composite keys #5355

aebersold opened this issue Aug 6, 2014 · 84 comments

Comments

@aebersold
Copy link

Eloquent assumes heavily that you are using surrogate keys on your tables (the id field in every table). However in real world usage, particularly older database systems not designed with web frameworks in mind, there are often tables with composite keys. That is, the primary key is over two or more columns. Often, you can't just add autoincremented id's. This is definitely a game stopper for a lot of use-cases, where you are forced to access existing RDBMS (e.g. building a frontend for an existing db). See also this reddit post about this issue.

The schema builder has already support for composite keys, however eloquent doesn't seem to support it yet.

I request the feature to use composite keys in your eloquent models and queries. The default configuration for surrogate keys is fine, just add the option.

@Marwelln
Copy link
Contributor

Marwelln commented Aug 6, 2014

I would like to have this supported too.

@fabioboris
Copy link

Me too.

@nesl247
Copy link

nesl247 commented Aug 21, 2014

I agree, this is extremely annoying at the moment. Unfortunately I'm not in charge of a lot of data I need to access.

For the moment, I'm just overriding newQuery() in my models by doing something like

function newQuery()
{
    $query = parent::newQuery();
    $query->... // Add constraints here
    return $query;
}

@aebersold
Copy link
Author

@nesl247 this is a good workaround. So did you specify primaryKey with one field and added the second field as a where-clause in the newQuery method? Can you give me some actual code?

@tsilenzio
Copy link

You could view my bug report (#5517) which has a work-around that works flawlessly for me.

@nesl247
Copy link

nesl247 commented Aug 26, 2014

@aebersold Yes, that is exactly what I did. In my case I was using a BaseModel and a bunch of child classes that extended my BaseModel, so I had a protected $secondaryKey = 'something' and a protected $type = 'someType'.

function newQuery()
{
    $query = parent::newQuery();
    $query->where($this->secondaryKey, '=', $this->type);
    return $query;
}

@pmccarren
Copy link

I would really like to have this supported as well.

@cgwyllie
Copy link

+1

@weotch
Copy link
Contributor

weotch commented Nov 19, 2014

I put this in my Eloquent Model subclass to enforce my extra column ('locale'). Maybe it'll help someone.

    protected function setKeysForSaveQuery(Builder $query)
    {
        parent::setKeysForSaveQuery($query);
        $query->where('locale', '=', $this->locale);
        return $query;
    }

@ghost
Copy link

ghost commented Dec 17, 2014

+1
I just get an common error while saving model with composite keys
PDO::lastInsertId() expects parameter 1 to be string, array given

@jonagoldman
Copy link
Contributor

+1

@GrahamCampbell GrahamCampbell changed the title [Request] Eloquent should support composite keys Eloquent should support composite keys Dec 28, 2014
@piotrchludzinski
Copy link

I want it!

@HiroKws
Copy link

HiroKws commented Jan 30, 2015

-1
This is not "should" stuff. Please keep Eloquent simple.
As you mentioned, query builder supported it, so it is enough.

Don't do every thing by Eloquent. Just use query builder, results became collections in L5, so what problem?

@GrahamCampbell
Copy link
Member

I doubt this will ever happen.

@ClickerMonkey
Copy link

Why do you doubt this will ever happen?

Composite keys are extremely common, if the SchemaBuilder supports it why wouldn't you want to have it supported by Eloquent as well?

Sometimes using surrogate keys are wasteful, especially if you're using UUIDs as identifiers... having a useless column you never search on that just takes up table & index space is silly sometimes.

Keeping Eloquent "simple" is a very silly reason. To the end-user nothing changes - but they now have the ability to specify an array for the primary key. If Laravel/Eloquent really expects to be taken seriously and provide further credit for using PHP for web development it should naturally support what every other ORM supports for alternative web development libraries.

Using pivot only is doable when you have a few extra columns on your relation... otherwise it's very awkward.

Using the query builder for saving & deleting is kind of silly, why would anyone want those inconsistencies in their code? Some places you use Eloquent for saving/deleting and some places you use QueryBuilder? This kind of attitude is poor and sounds lazy.

If Laravel is open to receiving a feature like this, I would be more than willing to supply a nice simple and efficient solution.

@Evertt
Copy link

Evertt commented Feb 23, 2015

+1

@lotestudio
Copy link

+1
Maybe in 90% of my projects I use composite primary keys. For localisation and for one to many relationships composite keys is the best practice for me... I'm very dissapointed that in Laravel 5 there is no composite keys, but a lot of complicated design patterns. That is my opinion. Don't want to argue with anyone.

@vvinhas
Copy link

vvinhas commented Mar 3, 2015

Eloquent follows the Active Record convention, so it needs a single primary key to work.
If you need composite keys in your project, maybe Eloquent isn't the best tool for it.

@ClickerMonkey
Copy link

The Active Record pattern makes no restrictions about primary keys - I don't know where you're getting that from.

Eloquent/Laravel is currently developed to work with a single primary key, there's no reason why someone shouldn't be able to make a contribution to the project which adds composite keys. What is the best way to go about this?

@vvinhas
Copy link

vvinhas commented Mar 3, 2015

All I'm saying is that this is a convention in other frameworks as well. Since Rails made it popular, the other frameworks followed the same path, that's what we see out there with most PHP frameworks (except Yii 2, for what I know).

Don't get me wrong, I would love to see Eloquent working with composite keys but I agree with Campbell, we're not going to see that happen so soon and my opinion is that I don't think it's worth the trouble. Well, only Taylor can tell :)

@ClickerMonkey
Copy link

If the current contributors aren't interested in implementing this feature, I'm more than willing to submit how I'm going to solve it (so I don't waste my time and have it be later rejected) - I just don't know who exactly to message to get the go ahead to work on it.

@vvinhas
Copy link

vvinhas commented Mar 3, 2015

Laravel developers are always willing to accept new ideas from the community, you can write to @GrahamCampbell or even @taylorotwell

@GrahamCampbell
Copy link
Member

Laravel developers are always willing to accept new ideas from the community, you can write to @GrahamCampbell or even @taylorotwell

Please pose questions to Taylor. He's the only one that can say yes/no to proposals.

@GrahamCampbell
Copy link
Member

As things stand, this issue is rejected, so it's unlikely for this to make it into the framework.

@aebersold
Copy link
Author

@taylorotwell are you willing to accept a pull request for this proposal?

@hlogeon
Copy link

hlogeon commented Sep 21, 2015

+1

3 similar comments
@warrickhill
Copy link

+1

@adrazich
Copy link

+1

@jaszczur171
Copy link

+1

@rapliandras
Copy link

I was just silently listening this thread for a year, still nothing new? I'm playing with the idea of coding an Eloquent extension specifically for supporting comp keys until this feature comes out.

@rbruinier
Copy link

+1

3 similar comments
@rogierborst
Copy link

+1

@Riesjart2
Copy link
Contributor

+1

@bgarrison25
Copy link

+1

@younes0
Copy link

younes0 commented Oct 23, 2015

this should be locked

@GrahamCampbell
Copy link
Member

lol - I don't lock issues anymore because it causes massive rage

@nguyenhu
Copy link

+1

1 similar comment
@melloc01
Copy link
Contributor

+1

@bgarrison25
Copy link

Why should this issue be locked?

@mikevrind
Copy link

Because of what Taylor said about this feature.

to be fair, it is not supported by Rails either. It is not as if every major framework supports this and Laravel is refusing to.

@bgarrison25
Copy link

So just because other frameworks don't support it means Laravel doesn't need to? In fact I would think this would be a driving force TO support it since it will be yet one more feature it has that other frameworks don't.

@mikevrind
Copy link

I (and a lot of others) totally agree with you. It's quite a common thing to use in database tables that simply don't require a 'id' field.

@ClickerMonkey
Copy link

Taylor's response is interesting. It could one of a few things.

  1. He doesn't think Laravel can/should be better than Rails.
  2. He knows this is an excuse for not wanting to do it himself. Could be lack of time or passion.
  3. He protests the idea of composite keys, because some folks actually think you should always have a surrogate key.

I don't mean to pick on him, he's done fantastic work, but with all of the support this issue has gotten I think we need a well thought out reason on why Laravel will never have composite keys OR, even better,
what can be done to get this done.

After assessing what needs to be done to get composite keys working I can understand why he would want to avoid it. I think it would lead to a very healthy refactoring of the relationship classes.

As I've said before, If he's willing to accept a patch for this feature there are many people who want to work on this. If he's hesitant because he has specific things in mind for this feature - or lines he wouldn't want crossed, he could communicate them to us on this issue.

Outside of my work in PHP, ALL ORMS I've developed or used in the real world supported composite keys (Java might've spoiled me) so I find other libraries being hesitant a little odd.

As I've said before, Laravel to me completely legitimizes developing large and well architected websites in PHP. The company I work for has used it dozens of times for small to large clients. It's definitely one of my favorite frameworks - save this one thing.

Laravel's tagline shouldn't be "At least as good as the competition".

@crynobone
Copy link
Member

@ClickerMonkey I don't think @taylorotwell would reject if you/anyone can come out with a complete Pull Request to add composite key support for Eloquent.

You can't expect someone that doesn't use composite key him/herself to create such feature. I myself would never consider making that PR because I don't have all the knowledge to cover all possible use case.

@jayeley
Copy link

jayeley commented Nov 17, 2015

I rarely use surrogate key if it's meaningless, and I use composite keys a lot in applications. I don't see a good reason to force using surrogate key. So please support composite keys in Eloquent.

@hlogeon
Copy link

hlogeon commented Nov 17, 2015

May be if we collect enough "+1" I will make the solution

@codestic
Copy link

Guys this is ridiculous, while I really enjoy working with Eloquent, the idea of adding a completely useless column just to "make it work" is unacceptable. This is such a no-brainer, Doctrine 2 does have native support for composite keys as well.

Why not Laravel?!

+1!

@altheatec
Copy link

+1

1 similar comment
@ghost
Copy link

ghost commented Nov 18, 2015

+1

@Payetus
Copy link

Payetus commented Nov 19, 2015

I would like composite keys +1

@j3j5
Copy link
Contributor

j3j5 commented Nov 29, 2015

+1 for composite primary keys, there's no good reason to force using surrogate key, is there?

In addition, the fix proposed on #5517 is ridiculously simple and works flawlessly, I don't see why you wouldn't merge it on master. A pity.

@gabidavila
Copy link

Composite keys are really useful in use cases where you have an NxN table with a High read and write throughput.

I guess one could argue in that case you don't use auto_increment, you should use a hash as primary_key instead of integer and suck it up using an UNIQUE constraint, but in my point of view this "workaround" is not the ideal scenario for a situation like this, it just creates confusion in something that should be simple.

@samuelfa
Copy link

samuelfa commented Dec 2, 2015

+1

I put here my own solution:

    /**
     * Set the keys for a save update query.
     *
     * @param  \Illuminate\Database\Eloquent\Builder  $query
     * @return \Illuminate\Database\Eloquent\Builder
     */
    protected function setKeysForSaveQuery(Builder $query)
    {
        $keys = $this->getKeyName();
        if(!is_array($keys)){
            return parent::setKeysForSaveQuery($query);
        }

        foreach($keys as $keyName){
            $query->where($keyName, '=', $this->getKeyForSaveQuery($keyName));
        }

        return $query;
    }

    /**
     * Get the primary key value for a save query.
     *
     * @param mixed $keyName
     * @return mixed
     */
    protected function getKeyForSaveQuery($keyName = null)
    {
        if(is_null($keyName)){
            $keyName = $this->getKeyName();
        }

        if (isset($this->original[$keyName])) {
            return $this->original[$keyName];
        }

        return $this->getAttribute($keyName);
    }

I'm using a Abstract Model which one final model has extended and in his private property of primaryKey I declared an array with the name of each field on my composite key.

@ivid
Copy link

ivid commented Dec 6, 2015

This is just sad. +1 for all its worth...

@laravel laravel locked and limited conversation to collaborators Dec 6, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests