[2.2.1] Fix getMeta returns empty Collection if attribute is casted in some cases

[bfiessinger]

regarding to #100 getMeta returns an empty collection on some casted attributes if the cast was initialized by some trait.

[siamak2]

You said getMeta returns empty collection but you modified getAttribute method. getAttribute is used for fluent access and getMeta never uses getAttribute.

[siamak2]

Can you provide a test that will fail without the modifications you made?

[bfiessinger]

You said getMeta returns empty collection but you modified getAttribute method. getAttribute is used for fluent access and getMeta never uses getAttribute.

That's true but getMeta passively makes use of getAttribute when calling $this->metas in getMetaData and somehow this returns Undefined property: App\Models\XYZ::$metas in some rare cases with casted attributes.

Can you provide a test that will fail without the modifications you made?

I added a property state which is casted to a UserCastedObject Class. The tests are failing without the edits. It seems like the reason is not a 100% the same as for the problem with spatie/laravel-model-states but without the check for hasCast it will fail.

[siamak2]

I think now I understood the problem. It happens when result of a cast is null but I'm not sure how it happens. I made some changes in your code please try it to see if it solved the problem. Instead of

if ( $this->hasCast( $key ) ) {
  return $this->castAttribute( $key, $attr );
}

I copied the code from original getAttribute method.

[bfiessinger]

@siamak2 I've just reviewed your changes and it works as expected for me.

[siamak2]

@bfiessinger What about that line of code I asked you in my previous comment?

[bfiessinger]

I think your modified check is more straight forward. But I don't know if we have to check it after

// Don't get meta data if fluent access is disabled.
if ( property_exists( $this, 'disableFluentMeta' ) && $this->disableFluentMeta ) {
    return $attr;
}

as there shouldn't be a meta key named exactly like a model's property, right?

[siamak2]

as there shouldn't be a meta key named exactly like a model's property, right?

It's possible to add meta with a name from model property using setMeta method. this is an example:

$model->setMeta('id','some thing');

If models property is not null then there is no problem but when it's null then laravel-meta thinks it's a meta. the code I copied from original method checks if this is the case, then it won't consider it a meta.

This check is already done in parent::getAttribute( $key ) call. the reason I put it after checking for disableFluentMeta is that it's pointless to check all those conditions again if the user don't want fluent meta access.

[siamak2]

@bfiessinger I'm still waiting for you to tell me how HasUserCasts trait is working. I started a review about it a few days ago: https://github.com/kodeine/laravel-meta/pull/104#review-thread-or-comment-id-797902196

[bfiessinger]

Sorry but which comment? The link brings me up to the top of the PR. Have you assigned me to the said review?

hasUserCast just adds the casted property and sets default values. It might not be the perfect implementation, i've just tried to reproduce the issue there.

[siamak2]

The link works for me in browser but not in mobile app. here is the share link from the mobile app that also works on browser : https://github.com/kodeine/laravel-meta/pull/104#discussion_r1248294462

Have you assigned me to the said review?

There is no option to assign people. I guess every can view it.

Here is a screenshot of that review:

[bfiessinger]

Mhh yeah i'm currently writing from the gh app, so that might be the problem but I did not receive a notification about that review earlier on PC too.

Actually you are right. That way it doesn't make much sense. I think I was about to make UserCastedObject an abstract Object and create another class from it that should act like the default cast. This is pretty similar to what the model-states package was doing and why I went that approach for testing.

I'll take a look on it tomorrow.

[siamak2]

OK now it make sense. @kodeine PR looks good. you may merge it and tag it as 2.2.1

[kodeine]

Thank you @siamak2 and @bfiessinger