Error in HasBinaryUuid trait...?

[spatie]

Error in HasBinaryUuid trait...?

[isocroft]

@brendt i don't think it has anything to do with #49 . @telkins you can modify the what getKeyName() returns by overriding the method in your model (if you don't want to use uuid which is returned by the getKeyName() method defined in the HasBinaryUuid trait). Which i have done in my Laravel 5.6 project which uses this package already.

[brendt]

@isocroft

if that's okay with @brendt

Definitely!

[telkins]

Of course, I've just noticed that HasUuidPrimaryKey has been deprecated and that getKeyName was once in that trait.

I'm not sure if the docs are wrong, the code is wrong, or I'm wrong in how I've understood things. It seems like the docs allow for a uuid primary key with any name, but the code doesn't.

For what it's worth, I prefer the flexibility reflected in the docs and, without being too familiar with the package or its various features yet, would prefer a solution that allows one to craft the solution one wants for individual models. That would include the following:

• allowing for uuid primary keys called uuid or otherwise
• allowing for uuid keys (that are not primary) that are called uuid or otherwise
• allowing for multiple uuid keys where one/none of them could be the primary key and where one/none of them could be called uuid

I'm sure there's more, but like I said, I've not yet begun to use this package...much. ;-)

Thanks for your time. :-)

[brendt]

@isocroft this has probably to do with #49

[telkins]

@brendt I'm not sure that I see the connection. I followed the link to #49 , but I'm not seeing any changes to HasUuidPrimary key or any changes having to do with HasBinaryUuid's getKeyName() method.

Am I missing something...?

[isocroft]

@telkins from what you said, you haven't started using this package so there isn't an actual error case here or am i wrong ?

In all, i think the README isn't explanatory enough. I can volunteer later on to include better customization examples in the README maybe later this month (if that's okay with @brendt ) via a fresh PR.

[telkins]

@isocroft Thanks for the info/help. I'm not sure why, but I didn't even consider overriding the getKeyName() method in my model(s). My bad...! <-- insert embarrassed emoji here -->

No, I really haven't begun to use it just yet. I'm in the early stages of a new project. I'm wanting to use laravel-event-projector and then this package for (binary) UUIDs. I was also hoping to use Nova for administration, but I'm a little wary about how nicely Nova and an event sourced system will play together. So....things have stalled a bit while I wait for Nova and I'm now focusing a bit more on some other things.

So...I'd consider this a non-issue now and I'll close it. Your help's been appreciated. :-)

[lonnylot]

This is the commit that broke the documented functionality.

By deprecating the HasUuidPrimaryKey trait and merging that functionality into the HasBinaryUuid trait we lost the ability to set protected $primaryKey.

I think overriding two methods on each model using a binary UUID is annoying and would prefer the old functionality. I'm curious as to why the deprecation was done. I can't find a related PR so it's hard to tell...

@brendt could you provide some insight into your commit?

[brendt]

This is the related issue: #34

Unfortunately, I don't have the time this and next week to look further into this 😞However, feel free to suggest a solution!

[lonnylot]

@brendt I think I'd need to understand why HasUuidPrimaryKey was deprecated/merged into HasBinaryUuid. I don't see any clear reason for that and it would be hard to come up with a new solution without understanding why HasUuidPrimaryKey had to go...