search

fuel / orm

Fuel PHP Framework - Fuel v1.x ORM
http://fuelphp.com/docs/packages/orm/intro.html
151 stars 95 forks source link

Own encoding, lacking keys on primary key? Perhaps a standard method would be more appropriate #398

Closed xploSEoF closed 6 years ago

xploSEoF commented 7 years ago

Currently looking into re-basing some old work onto the 1.9 branch, only to notice this function that does something similar. https://github.com/fuel/orm/blob/1.9/develop/classes/model.php#L268

However, it's flawed in two ways:

I recommend switching to using a standard, such as JSON encoding or serialising an associative array., but at the very least, put in something to handle this.

WanWizard commented 7 years ago

As far as I can see it does use the values, otherwise it won't be able to generate a unique string per record. And it was never intended to be reversible, it just creates a unique string to be used internally as a lookup key. So square brackets in a key value is not a problem.

Changing it might have quite a big BC impact for people that (like yourself) have an external requirement for something like this, and now use this method.

xploSEoF commented 7 years ago

Sorry, I wasn't terribly specific there - it doesn't take into consideration the key that goes with each value. For example if I have

[
    'user_id' => 1,
    'group_id' => 2,
]

And both were part of the PK, the output value would be [1],[2] or [2],[1] dependant upon the order the fields are defined in $_primary_key, but the relation between the keys and the values isn't included.

However, if BC is a potential issue here, then maybe we should avoid touching it too much.

WanWizard commented 7 years ago

That is correct, but for what the function was designed it is irrelevant, as [1],[2] is a unique representation for that record.

Internally in the ORM, the return value is only used as index for the cache array, and as index for the relation array, any only used for matching. So any return value is ok, as long as it is a unique representation of the PK values per model, and it doesn't take up too much space.

I can't say if there are people that have used this method for other purposes (we haven't), but given the fact that it is public, chances are.

I saw your PR #399, I don't have a problem with it. Ok to merge, @stevewest?

xploSEoF commented 7 years ago

:+1: for explaining the use.

OK, could we maybe add some bits to the doc block to make it clear that it's not for any purpose beyond run-time? Alternatively, add @internal to the doc block?

WanWizard commented 7 years ago

Possible, but that doesn't solve the BC issue for people that have already used the method. I assume Jelmer made it public because it is used in several relation classes. That could have been solved differently, but that is all in hindsight, there is a lot more code from the beginning of the framework that is public, but shouldn't be. Having said that, feel free to update the Docblock. ;-)