Support models which don't use auto-incrementing IDs
Opened this issue · 12 comments
Currently, things don't work too well.
getNextModelIDassumes that IDs are always numbers and just adds 1 each time- Newly created models are forced to have a new model_id assigned to them
- Inserting a new record when updating a model is done using
$query->insert($attributes)which fails if the attributes don't contain anidfield when it's not automatically set by the database
I've started a branch in my fork (https://github.com/EspadaV8/eloquent-versioned/tree/feature/non-auto-incrementing-id) that will hopefully fix some of these issues, but it's a fair way off yet.
Curious as to why you don't feel this works well? What would be a use-case where this would not be adequate?
The model_id column is pretty essential (in my approach) as it is the persistent/unique identifier for a model (and all its versions). Not sure how you would easily track versions without it, but I look forward to seeing your approach.
Also getNextModelID doesn't just +1 the current version, it gets the max version for the given model and increments that (in case your current version has been rolled back to an older version and you create a new version from that model).
In my application I'm using UUIDs instead of an auto-increminting column. The UUID is created before the model is actually saved and themodel_id in this case would be set to 1, which would then need to be used to access the model.
What I need in this case is for the model_idto be a newly generated UUID
That makes sense.. So I guess my code would need to provide a more flexible getNextModelId() method then..
Let me know what you come up with for this!
If you have chance to check that branch out it's basically working at the moment (in the tests). I've not had chance to test on real data thought. The last commit was just a dumb before the end of the day so it needs a bit of a clean up.
I'd be interested in any comments/feedback/ideas you have about it. I actually forget what I had to change but in all I don't think there was too much. It does build on the next/previous pull request which I'll change first so a lot of the diff related to that can be ignored.
Hmm, if you're taking care of generating the UUIDs maybe the easiest solution here is for the code to check if a model_id attribute exists and is even needed. In your case, it would not be needed because the version and is_current_version columns would provide all the functionality needed, since you would not be changing the UUID between models (so it works as id and model_id at the same time).
I already know I need to make my code more flexible around the timestamps (currently it assumes they are present and they might not always be) so I can look into that at the same time.
Hmm you seem to have already done pretty much what I was thinking in your branch. So the trait needs to check if model_id exists and also not make auto-incrementing assumptions about the id column and just treat it as an opaque attribute.
That's the min diff between my 2 branches with the code for non-auto-incrementing IDs.
Some of that can be ignored too once I've cleaned up the formatting and sorted out my last commit.
Cool, that's good then, at least we on the same page :)
These is a bit of messing around with some Laravel stuff where model events that don't get fired in PHPUnit (hence the need for orchestra/testbench) and with the UUID generation we need to manually fire the created event when replicating the model otherwise a new UUID isn't created for the replica.
I'm also not happy with the tests needing all models provide the model_id and id but at the moment I can't think of a way to run the asserts without having them passed in.
I had been reading about Orchestra/Testbench for a bit but I'm still a bit new to testing so I was sticking with vanilla PHPUnit for now, but I welcome ugprades! I'll look into it..
I think it would make sense to create separate tests for models that don't use the default auto-incrementing id column.
Sure, the tests can be split out. I was actually thinking of splitting out some of the changes into separate PRs (like testbench) to break down the number of changes in each PR. Would that be preferable for you?
Wow, can't believe this was a year ago! I am using a fork of this package with @EspadaV8's work merged in for my MongoDB application, and it's working well (with a few additional fixes). I was wondering if this is going to be merged in any time soon. I was looking at pulling in some of the more recent updates into my fork, but this is essential.