Extending base shop model vs moving functions to a trait

[awebartisan]

Hi,
I am using this package and extending the BaseShop, I need to use another package which requires model to extend their base class too. How we are going to deal with such situation? Can we move the code in Shop.php model of this package to a trait?

Please have a look into it. thank you 👍

[gnikyt]

I can move the code to a trait yes and leave the default model still there including the trait for backwards compatibility.

[awebartisan]

So if someone use only trait, what will happen to ShopObserver? I can change to the model to \App\Shop in the config. ShopObserver will not fire and it will cause trouble. What are your thoughts on this one?

[gnikyt]

The Observer grabs the class defined in the config. It will be fine.

…

On Wed., Nov. 7, 2018, 14:15 Zubair Mohsin ***@***.*** wrote: So if someone use only trait, what will happen to ShopObserver? I can change to the model to \App\Shop in the config. ShopObserver will not fire and it will cause trouble. What are your thoughts on this one? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#137 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACTuOhtits-wMkICsq8Wj8MrZCneF36Oks5usxw2gaJpZM4YS2gu> .

[awebartisan]

use OhMyBrew\ShopifyApp\Models\Shop;
Observer has this use statement on its top and it uses this in creating function. I have tried moving all the code from BaseShop model of package and moved to my own Shop model and removed extends keyword then i faced ShopObserver problem.

[gnikyt]

I addressed this a couple versions back I believe already, someone made a PR to use the config version.

…

On Wed., Nov. 7, 2018, 14:18 Zubair Mohsin ***@***.*** wrote: use OhMyBrew\ShopifyApp\Models\Shop; Observer has this use statement on its top and it uses this in creating function. I have tried moving all the code from BaseShop model of package and moved to my own Shop model and removed extends keyword then i faced ShopObserver problem. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#137 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACTuOnVyFIt9tm8q705cWvoe1gXAxgHdks5usx0AgaJpZM4YS2gu> .

[awebartisan]

        $shopModel = config('shopify-app.shop_model');
        $shopModel::observe(ShopObserver::class);

Yes they did this in ShopifyAppProvider.php , but ShopObserver.php class still uses package base model in use statement and creating function.

[gnikyt]

Hmm it just states it has to be an instance of Shop, not use it. It'll have to be killed for the trait yes.

…

On Wed., Nov. 7, 2018, 14:22 Zubair Mohsin ***@***.*** wrote: $shopModel = config('shopify-app.shop_model'); $shopModel::observe(ShopObserver::class); Yes they did this in ShopifyAppProvider.php , but ShopObserver.php class still uses package base model in use statement and creating function. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#137 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACTuOukBmygplAra7YLKJUI58E2ccM8Fks5usx30gaJpZM4YS2gu> .

[gnikyt]

It turns out this may not be possible.

I can not have $fillable, $casts, $dates, etc properties inside the trait. Because the base Eloquent defines them, the trait can not override it. Moving the properties and defining them inside the constructor of the model does not work, I get "fillable" errors on a few tests.

There does not seem to be a work around for this unfortunately. Only work around I see is to define all methods in the trait, and keep the properties inside the model class.

This is not ideal, but it would mean if you use the trait directly (without extending the shop model), you would need to also manually add the properties to your own class to ensure it works.

On the fence about this one...

---

Example of above (not working, cant work):

class ShopModel extends BaseModel {
    use ShopModelTrait;
}

Example of above (which could work), but I'm not sure its a clean way...

class ShopModel extends BaseModel {
   /* define properties here for $fillable, etc */

  use ShopModelTrait;
}

// Your own model...
class MyOwnShopModel {
  /* define the same properties */

 use ShopModelTrait;
}

This is an issue because if the core properties of a model changes (additions, deletions, changes), then your own model would fail without updating it, which could lead to issues, but of course, I could just doucment this in the changelogs if such a change should happen.

[awebartisan]

I am also searching, will let you know if I get some idea from anywhere. Looking at other packages specifically which require extending their models for overriding methods and properties.

[awebartisan]

https://gist.github.com/JordanDalton/f952b053ef188e8750177bf0260ce166

you probably stumbled upon this also?

[gnikyt]

I have but this involves overwriting how eloquent does fillables and such. Which would depend on me updating the code should that change. Doesn't seem like a clean solution can happen besides the one I mentioned above which is still not totally clean but at least helps anyone wanting to extend

[awebartisan]

Okay... can you add the trait including only functions and mention in documentation to use it on your own risk? and add the properties to your own model which will be using the trait.

[gnikyt]

Yup that should work

[awebartisan]

Any timeline about this feature?

[awebartisan]

https://www.archybold.com/blog/post/booting-eloquent-model-traits

Does this help in any way to solve above problem?

[gnikyt]

Sadly the above still causes complaints by Eloquent, it just wont work nicely.

I've gone the trait + model for properties. Its now available through v4.2.0 release. All tests passed, code coverage remained at 100.

See the wiki article on override and extending for more information; essentially use the trait and paste in the same properties from the base model.

[gnikyt]

Observer will now accept any object yes.

…

On Thu., Nov. 15, 2018, 01:52 Zubair Mohsin ***@***.*** wrote: This is great. I haven't tested it yet but quick question that Observer is also taken care of? Because it will fire off as its being resolved from config file in ShopifyAppServiceProvider.php — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#137 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACTuOl1K37d2J_AydgRqs8FgIh82R4LOks5uvPoXgaJpZM4YS2gu> .