sbarre / eloquent-versioned

Add transparent versioning to Laravel 5's Eloquent ORM
MIT License
30 stars 11 forks source link

Problems with model_id always defaulting to 1 when being persisted. #22

Open isaackearl opened 8 years ago

isaackearl commented 8 years ago

Hello,

I experimentally started using the package last night and was attempting to proof of concept what I wanted to accomplish by using artisan tinker... unfortunately I didn't get very far because I realized whenever I create a new model it persists model_id 1 to the database the first time it is persisted.

example using a simple model in a table that has some rows already: TestModel::create(['name' => 'test', 'content' => 'ha ha']); will immediately output this as it gets created:

=> TestModel {
     id: 14,
     name: "test",
     content: "ha ha",
     model_id: 14,
     is_current_version: 1,
     created_at: "2016-05-23 22:35:04",
     updated_at: "2016-05-23 22:35:04",
     deleted_at: null,
   }

So everything looks good right? well the database row that actually got inserted looksl like this: id: 14, name: test, content: ha ha, model_id: 1, version: 1, is_current_version: 1 etc

as you can see it just inserts 1 as the model_id. I mean this makes sense to me since you wouldn't know the ID of the model until after it is persisted... so it would require a second save to update the model_id...

It works the same way when I new up a model and save it the traditional way. I'm wondering is this supposed to be how this behaves? I'm using php 7, and mariadb 10. I'm going to take a look at the code and if I can figure out a fix I'll attempt a pull request.

Please let me know if I am just trying to use it wrong or if I missed some part of setup (I looked over the readme several times to check).

Thanks!

isaackearl commented 8 years ago

So I've looked more into this and I've found that there is something strange going on when trying to update the model_id to the correct value. Here is the bit of relevant code (part of Versioned)

        // If the model is brand new, we'll insert it into our database,
        // then set the model_id to the id of the newly created record.
        else {
            $this->{static::getIsCurrentVersionColumn()} = 1;
            $saved = $this->performInsert($query, $options);
            $this->{static::getModelIdColumn()} = $this->attributes[$this->primaryKey];
            $saved = $saved && $this->performUpdate($query, $options);
        }

so It sets the model_id column to the value of the primary key column (id), and then it tries to perform the update... Well I followed the update through Model and did a var dump of the sql and bindings and this is what happens:

>>> $ag = \Aviago\Models\Agreement::create(['name' => '4', 'content' => '4']);
string(117) "insert into `agreements` (`name`, `content`, `is_current_version`, `updated_at`, `created_at`) values (?, ?, ?, ?, ?)"
array:5 [
  0 => "4"
  1 => "4"
  2 => 1
  3 => "2016-05-24 15:08:55"
  4 => "2016-05-24 15:08:55"
]
string(200) "update `agreements` set `name` = ?, `content` = ?, `is_current_version` = ?, `updated_at` = ?, `created_at` = ?, `id` = ?, `model_id` = ? where `model_id` = ? and `agreements`.`is_current_version` = ?"
array:9 [
  0 => "4"
  1 => "4"
  2 => 1
  3 => "2016-05-24 15:08:55"
  4 => "2016-05-24 15:08:55"
  5 => 45
  6 => 45
  7 => 45
  8 => 1
]

As you can see from above... the UPDATE that takes place after the initial insert is wrong... it tries to update WHERE model_id = 45... which will never happen since that is the value we are trying to update in the first place. It should be trying to use the primary key to update the model_id... I'm still not sure how to fix this yet, but I'm hoping this helps someone pinpoint how to make the perform update work we want.

sbarre commented 8 years ago

Hi there, I will look into this as soon as I can. It might take me a few days, just to set expectations. If you figure out more in the meantime, please update this issue. Patches are also welcome if you figure out a solution.

isaackearl commented 8 years ago

@sbarre I committed a fix to this issue sometime ago and was wondering if you ever had a chance to review and accept my pull request.

Thanks