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

Saving Eloquent models with Composite Keys causes an error #5517

Closed
tsilenzio opened this issue Aug 21, 2014 · 21 comments
Closed

Saving Eloquent models with Composite Keys causes an error #5517

tsilenzio opened this issue Aug 21, 2014 · 21 comments

Comments

@tsilenzio
Copy link

I created a composite index in the a typical database schema

$table->primary(array('some_name','some_value'));

I defined the model like the following, no difference between protected or public decleration

protected $primaryKey = array('some_name','some_value');

I try to save the model

$modelobject->save()

Error:

{"error":{"type":"ErrorException","message":"Illegal offset type in isset or empty","file":"\/srv\/www\/laravel\/vendor\/laravel\/framework\/src\/Illuminate\/Database\/Eloquent\/Model.php","line":1587}}

Trace:
https://github.com/laravel/framework/blob/4.2/src/Illuminate/Database/Eloquent/Model.php#L1587
https://github.com/laravel/framework/blob/4.2/src/Illuminate/Database/Eloquent/Model.php#L1575
https://github.com/laravel/framework/blob/4.2/src/Illuminate/Database/Eloquent/Model.php#L1453
https://github.com/laravel/framework/blob/4.2/src/Illuminate/Database/Eloquent/Model.php#L1387

@RSully
Copy link
Contributor

RSully commented Aug 21, 2014

Also see #5355, #5248. Maybe related: #5251.

@tsilenzio
Copy link
Author

I was able to provide an override method that is able to save composite keys and other-wise falls back on Laravel's saving method

public function save(array $options = [])
    {
        if( ! is_array($this->getKeyName()))
        {
            return parent::save($options);
        }

        // Fire Event for others to hook
        if($this->fireModelEvent('saving') === false) return false;

        // Prepare query for inserting or updating
        $query = $this->newQueryWithoutScopes();

        // Perform Update
        if ($this->exists)
        {
            if (count($this->getDirty()) > 0)
            {
                // Fire Event for others to hook
                if ($this->fireModelEvent('updating') === false)
                {
                    return false;
                }

                // Touch the timestamps
                if ($this->timestamps)
                {
                    $this->updateTimestamps();
                }

                //
                // START FIX
                //


                // Convert primary key into an array if it's a single value
                $primary = (count($this->getKeyName()) > 1) ? $this->getKeyName() : [$this->getKeyName()];

                // Fetch the primary key(s) values before any changes
                $unique = array_intersect_key($this->original, array_flip($primary));

                // Fetch the primary key(s) values after any changes
                $unique = !empty($unique) ? $unique : array_intersect_key($this->getAttributes(), array_flip($primary));

                // Fetch the element of the array if the array contains only a single element
                //$unique = (count($unique) <> 1) ? $unique : reset($unique);

                // Apply SQL logic
                $query->where($unique);

                //
                // END FIX
                //

                // Update the records
                $query->update($this->getDirty());

                // Fire an event for hooking into
                $this->fireModelEvent('updated', false);
            }
        }
        // Insert
        else
        {
            // Fire an event for hooking into
            if ($this->fireModelEvent('creating') === false) return false;

            // Touch the timestamps
            if($this->timestamps)
            {
                $this->updateTimestamps();
            }

            // Retrieve the attributes
            $attributes = $this->attributes;

            if ($this->incrementing && !is_array($this->getKeyName()))
            {
                $this->insertAndSetId($query, $attributes);
            }
            else
            {
                $query->insert($attributes);
            }

            // Set exists to true in case someone tries to update it during an event
            $this->exists = true;

            // Fire an event for hooking into
            $this->fireModelEvent('created', false);
        }

        // Fires an event
        $this->fireModelEvent('saved', false);

        // Sync
        $this->original = $this->attributes;

        // Touches all relations
        if (array_get($options, 'touch', true)) $this->touchOwners();

        return true;
    }

@sirWill
Copy link

sirWill commented Nov 11, 2014

Thank you for solution. You saved my time.

@lukasmalkmus
Copy link

Sorry if I bring this up again... But where to add your code? My save function in the Model.php file looks different then yours. To different. Or is this outdated?

@rapliandras
Copy link

Guys, can you suggest a Laravel 5 compatible solution? This does not work.

@carsonevans
Copy link

@rapliandras, It looks like @tsilenzio took the save function in the Model.php and overrode it his own model. In the process keeping a lot of the event hooks and other stuff but then changing the area labeled "START FIX" to "END FIX" to handle composite keys. His code was for Laravel 4, but you can do the same in Laravel 5. The Laravel5 Model->save() is much cleaner. To accomplish a similar thing what I did was take the $query variable that is set at the top of Model->save() and just added some ->where()'s to it....

public function save(array $options = array())
{
  $query = $this->newQueryWithoutScopes();
  // MY MODEL SPECIFIC FIX
  $query->where('OTHER_ID', '=', $this->OTHER_ID);
  // END MY FIX

 // the rest of the standard model->save() code is here but I cut it out for brevity. 
 // Use what is in Model.php already. 
}

and to answer the previous question from @MichealMi: This is placed in your own model that extends Model to override the parent save method.

@ccuono
Copy link

ccuono commented May 3, 2015

@carsonevans thank you for sharing your findings. I did what you wound up doing but was finding that the update query that was being generated kept generating like this: UPDATE .... WHERE ... AND "Id" = NULL

I had to add the following for my model class to override the setKeysForSaveQuery function since my table does NOT have an Id column but has a composite primary key:

/**
 * Set the keys for a save update query.
 * This is a fix for tables with composite keys
 * TODO: Investigate this later on
 *
 * @param  \Illuminate\Database\Eloquent\Builder  $query
 * @return \Illuminate\Database\Eloquent\Builder
 */
protected function setKeysForSaveQuery(Builder $query)
{
    $query
        //Put appropriate values for your keys here:
        ->where('column1', '=', $this->column1)
        ->where('column2', '=', $this->column2);

    return $query;
}

Overriding this function is all I had to do and I did not have to override save or anything else.

I wonder if we could just override the $primaryKey property and that would be all that needs to be done...

@ghost
Copy link

ghost commented Jun 12, 2015

Its works!!!
I ignored the model of the relationship table and set "belongsToMany" in the two tables with a little details: set the two foreign key of two tables and set the name of the relationship table. The whole foreign keys will works like a composite key.

Ex:
//--------- Model "Table1": ------------//
class Table1 extends Model {
...
public function table2()
{
return $this->belongsToMany('App\Table2', 'table_relashionship', 'id_table2', 'id_table1');
}
}

//--------- Model "Table2": ------------//
class Table2 extends Model {
...
public function table1()
{
return $this->belongsToMany('App\Table1', 'table_relashionship', 'id_table1', 'id_table2');
}

}

Now the Eloquent undestand that you have a 'table_relashionship' in your database with columns 'id_table1' and 'id_table2' and you can access the data with the comand:
$table1 = Table2::find(1);
dd($table1->table2)

Its solve my problem!

@cmnardi
Copy link

cmnardi commented Jun 19, 2015

@ccuono
I used your method, but without fix the field names ...
and with a "if".
This way I expect to be able to use in a extension of the Model class!
But in the save method I get an Exception :
PDO::lastInsertId() expects parameter 1 to be string, array given

I'll try to fix ... until then I'm using the save method of like @tsilenzio suggest.

Hope this helps someone else

Thanks!

/**
 * Set the keys for a save update query.
 * This is a fix for tables with composite keys
 * TODO: Investigate this later on
 *
 * @param  \Illuminate\Database\Eloquent\Builder  $query
 * @return \Illuminate\Database\Eloquent\Builder
 */
 protected function setKeysForSaveQuery(\Illuminate\Database\Eloquent\Builder $query) {
        if (is_array($this->primaryKey)) {
            foreach ($this->primaryKey as $pk) {
                $query->where($pk, '=', $this->original[$pk]);
            }
            return $query;
        }else{
            return parent::setKeysForSaveQuery($query);
        }
    }

@andrewtweber
Copy link

This should definitely not have been closed. It's still an issue in 5.1. For example, if you do:

$item = Item::firstOrNew(['pk_col_1' => $x, 'pk_col_2' => $y]);
$item->col_3 = 'content';
$item->save();

It gives the "Illegal offset type in isset or empty"

Thanks @cmnardi for the fix. I have all of my "models" extending a BaseModel which extends Eloquent Model, so it was easy to stick in. This should really be merged into master

@rap2hpoutre
Copy link
Contributor

Thanks. Not sure to understand why it's not merged :'(

@turgutsaricam
Copy link

Thanks for the fix @cmnardi. Why is the issue closed although the error still exists?

@cmnardi
Copy link

cmnardi commented Sep 7, 2015

Maybe is because they don't see this like an error.
You should create a field like a primary key in every table

@cmoralesweb
Copy link

@cmnardi

You should create a field like a primary key in every table

Only when it makes sense, right? If they expect everyone to always put a single field ID, composite primary keys shouldn't be supported. There is no sense to support them partially.

@andrewtweber
Copy link

@cmnardi the database shouldn't be structured in a certain way to support the framework, it should be the other way around.

@cmnardi
Copy link

cmnardi commented Oct 27, 2015

I agree @cmoralesweb and @andrewtweber the database must be independent of the framework
in some cases the database already exists and you need to build the application 'around'

@devilcius
Copy link

@cmnardi, to prevent the exception PDO::lastInsertId() expects parameter 1 to be string, array given, you should set to false $incrementing

public $incrementing = false;

Also note that, if the the relationships are set properly, there's no need to override the save function to have the inserts working. Just the setKeysForSaveQuery for the updates

@jonagoldman
Copy link
Contributor

Any reason why this issue is closed?
I also noticed discussion on the main issue #5355 have been disabled...

@punchi
Copy link

punchi commented Dec 15, 2015

People are trying to find how to make work composite keys with Eloquent, issues like #5355 are trying to help. If Laravel don't want to support the requested feature, at least do not close the thread that are trying to help.

+1 to Composite Keys

@leonaves
Copy link

leonaves commented Jan 8, 2016

+1 Composite keys

@laravel laravel locked and limited conversation to collaborators Jan 8, 2016
@GrahamCampbell
Copy link
Member

These will never be supported because it makes no sense in the context of routing, and our interface for generating routes.

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