Added refresh function to model

[kalaspuffar]

When using a model in a system where it's required that your model is up to date with database before you make your changes then a refresh is a good addition to the model class.

[emlynwest]

While this looks like it could be a useful feature I think there is more work needed than you think here. For example your code dies not take into account updating the model cache.

It would also be good if you could update the docs with the new feature.

[kalaspuffar]

Thanks for the input.

I wanted to open the request to suggest the feature to see if I'm thinking in the right direction. I think I've solved the cache updating.

Also want to move the new check to the beginning of the function. And last but not least I still have a cache related issue in php7.

I'll try to update the docs.

Again Thanks for the input.

[kalaspuffar]

Looked around for an external documentation. Or do you mean the comment of the function? I've added more information of the function and what actually is done when it runs.

[emlynwest]

I mean add to the documentation at https://github.com/fuel/docs

[kalaspuffar]

Added documentation in https://github.com/fuel/docs/pull/697

[kalaspuffar]

Sorry must have missed that. I saw that we set the original in the create method but never touch it in the update method. I thought that was an oversight.

But in this case we update the originals twice when creating but not when updating.

[kalaspuffar]

Hi Steve.

I think the feature set of this function now is fully flushed out, at it works in the project I'm currently using it.

In this latest commit I added a function that recursively adds data to this model and the related models and also sets the primary keys so all data is updated. This function might not be applicable as a public function but is needed to simplify the implementation of the refresh function.

Should this function exist or should it be a part of the from_array function? Do you think it's wrong to update the primary keys when refreshing models from database?

best regards Daniel

[emlynwest]

Given that your function does not seem to reload relations I am not sure why the recursive value update is needed. This was a nice feature but now I can't help feel that it's started to get bloated and overly complicated.

[kalaspuffar]

Hi Steve.

I understand that you don't want to bloat your code base and neither would I. So I hope we can find a solution where I can rework my PR so we still can add this function but with less code or better implementation.

Just so you understand my problem space I'll add an example. I've obfuscated the classes and kept only the identifiers for each model. The relationship tree I'm working with is 4 levels deep.

$object = Array
(
    [id] => 7322
    [an_object] => Array
        (
            [id] => 134000
        )
    [another_object] => Array
        (
            [id] => 67003
            [inner] => Array
                (
                    [id] => 1
                )
            [second_inner] => Array
                (
                    [id] => 2668
                )
            [third_inner] => Array
                (
                    [9463] => Array
                        (
                            [id] => 9463
                            [first_inner_inner] => Array
                                (
                                    [id] => 18301
                                )
                            [second_inner_inner] => Array
                                (
                                    [id] => 43203
                              )
                        )
                )
        )
)

So I really do need to handle relationship mappings.

The current from_array function will give me a model with the following structure

$object = Array
(
    [id] => 7322
    [an_object] => Array
        (
            [id] => 134000
        )

    [another_object] => Array
        (
        )
)

This could be a bug or an implementation detail that I've not understood. But with the current implementation I don't get a model back from the from_array function that is identical to the model described in the array.

The reason I've created a new function is that I didn't know if the from_array should set primary keys. Or if I should make it an param you supply to from_array to say if you want to set the keys or create new objects.

Anyway how from_array should handle multiple level deep arrays right?

I could put in the time to make the changes I just need a direction.

Best regards Daniel

[emlynwest]

I'm sorry but you've lost me now. I am not sure I see what from_array() has to do with your desired feature.

[kalaspuffar]

Hi Steve.

At the moment the refresh() function uses the to_array function to take the model retrieved from the result and extract all information. And then applies this data to the current model so this model will be refreshed with new data.

$query = Query::forge(get_called_class(), static::connection(true));
$this->add_primary_keys_to_where($query);
$result = $query->get_one();

if ($result instanceof self)
{
    $result_array = $result->to_array();
    $this->load_from_array($result_array); // This was earlier from_array.
    static::$_cached_objects[get_class($this)][static::implode_pk($this)] = $this;
}

This is exactly for the reason you mention earlier. I didn't want to bloat the codebase so I used functions already implemented in the model class. But I ran into problem when I wanted to change my data with from_array back from the to_array format. Some of the data didn't make it back.

Maybe the function to create data models from an array never where intended for this purpose?

[emlynwest]

Ah, you mean the Query's to_array() not from Model. I think you are right in that creating models from arrays has never taken into account setting relations at the same time. I am starting to feel that the v1 code base is not going to be suitable for this and instead this should go into v2.

[kalaspuffar]

Hi Steve.

Well I can change the merge request target later. And if you think this is a breaking change it should be targeted to version 2.

But to make this change I still need to know what to do with load_from_array?

Should this function remain as is? Should I merge the functionality into from_array? Is their cases I've missed in my implementation? Should from_array set primary keys or not? and if not should you be able to override that option?

Sorry for all the questions but I really want to make this feature happen and contribute in any way I can.

We need this feature in our organization so making a good implementation that later could be a part of fuel orm is of interest for us.

[emlynwest]

It's not so much that it's a breaking change, but more the fact that there should be a feature freeze on v1 now, for v2 it's not a case of changing the merge target as the v2 orm is on a separate repo and is a total rewrite. The v2 orm's architecture is totally different from v1.

I am reluctant to merge this into the core at this time, but if you need it in your organization then there's no reason you can't use a shared base model class or trait.

[kalaspuffar]

Hi Steve.

Sounds good. I've moved the functionality up to our base class which works for us.

Will close this PR now.

Been really nice chatting with you and I hope I've not taken up to much of your time. Looking forward to version 2.0 and when it's release I'll look into if we perhaps should open a PR by then.

Best regards Daniel

[emlynwest]

Not at all, it's always good to hear how people are using the code and ways we can improve it and make it more useful. Given the way that v2 is looking this feature might not even be needed. You can take a look for yourself over at http://github.com/fuelphp/orm