search

sbarre / eloquent-versioned

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

Just want to give some feedback.. #21

Closed isaackearl closed 8 years ago

isaackearl commented 8 years ago

EDIT Just wanted to say after looking at it closely I realized this would cause problems because the model_id would have to be updated to reference the newest version of the model (i think)... So my idea here won't work I don't think. I'll need to think of a better way to handle my particular use case.

I think this package would be really fantastic with some tweaks to the implementation.

The decision to update the current record and make a new record for the old version is strange to me, as it seems like it would make alot more sense to create a new record for the newer version, and leave the old version as it is.

Maybe I'm wrong but here is why it makes sense in my use case:

I am creating agreements, and so I want to keep old versions of agreements. When someone agrees to an agreement I want to be able to associate that agreement to the user. When I make the new version of the agreement I don't want the user to be associated with the new version.. I want them to be associated with the version they actually agreed to.

In the current package implimentation, that means I would have to store the version and the model_id as a composite key foreign key reference to the model I want to associate.. and eloquent doesn't really support that. It would be much nicer if I could simply associate it with the ID of the model that it was originally saved as, and then if I want to reference the newer version I can reference the model_id and the scope will automatically limit that to the current version.

I think this would make the current package more flexible without losing any functionality. Then again I probably haven't given this as much thought as I need to. I'm going to try and fork this package and take a look.

EspadaV8 commented 8 years ago

The original version of this package actually worked how you mentioned. New versions would get a new row with a new ID and old versions would stay the same, however, this causes massive problems in (what I believe are) the more common use cases. If you read #15 and #16 you can see why I recommended changing this.

sbarre commented 8 years ago

@isaackearl to add to Andrew's note: in your example, you are talking about directly referencing the id column, but that's actually the wrong thing to do if you are using this package, because the whole point of our package is to use the model_id and version columns as a model's unique identifier, and for the actual id column to be a private/internal value that other code should not reference.

With regards to your edit, if you are tracking the model_id and version in your other code, you should not need to update the model_id after you save a new version. model_id will always be the same across all versions of a given model, it's only the version field that increments (and obviously the id column will change but your code should not care about that if you're accessing the model through the proper interfaces).

I appreciate the issue you're trying to solve though, you want to use an Eloquent relationship to associate something to a specific version of a model. That's an interesting use-case that I had not thought of.

If you track the source table's model_id and version values in your related table, create a composite foreign key in your schema, and then use a custom query function in your other model's Eloquent implementation (as opposed to one of the canned relationship convenience methods) that retrieves your versioned model based on those two values (and turns off versioning otherwise the is_current_version constraint will apply and you might not get anything back), that should solve your problem with minimal custom code while still making use of the indexing correctly.

Do you have specific performance concerns with this approach? It seems like it should work fine but maybe I'm missing something.

isaackearl commented 8 years ago

@sbarre I can totally see your point now. I'm going to try and use it as you described. I took a stab last night at getting it to work in my project (Laravel 5.2), and it doesn't work quite right for me. I'm going to close this question, but will probably open some issues. (when I create or new up a model and save, I always get the model_id set to 1...)