lunar icon indicating copy to clipboard operation
lunar copied to clipboard

Add ability to swap core models for own models

Open gavinhewitt opened this issue 3 years ago • 20 comments

Is your feature request related to a problem? Please describe. It's not easy to add functionality to models. Yes, we have macros but that feels cumbersome. Also, macroable models are discouraged by the Laravel team so it kind of feels not right to use them.

Describe the solution you'd like I would like to see that we could easily swap core models for our own. This could be done in a config (Spatie does this for medialibrary classes) or serviceprovider.

Describe alternatives you've considered I've tried to extend core models, but that did not work because in some parts of the code the core models/namespaces are expected.

gavinhewitt avatar Jul 18 '22 22:07 gavinhewitt

This is something that has been requested a few times. The macroable functionality was a stop-gap solution.

I think it's something we need to look at, but it's not at the top of our list right now.

If someone wants to create a draft PR with a suggested approach, I would be interested to see it.

glennjacobs avatar Jul 19 '22 09:07 glennjacobs

Thanks for considering this 🙏🏼

I think it could be just something like

// config.php
'models' => [
        'product_model' => MyPackage\Models\MyProduct::class,
        'cart_model' => GetCandy\Models\Cart::class,
        'collection_model' => GetCandy\Models\Collection::class,
        // ...
    ]
    
// Reference it anywhere
config('getcandy.models.product_model')::find(1);  

but maybe @adam-code-labx could also share his take on it? 😎

gavinhewitt avatar Jul 19 '22 09:07 gavinhewitt

Be good to see how some other packages do it. I feel like the must be a tidier way.

glennjacobs avatar Jul 19 '22 10:07 glennjacobs

Also, I don't want to have to list out every single model in config, only the ones a developer wants to override.

Maybe we need a little helper function/class.

glennjacobs avatar Jul 19 '22 10:07 glennjacobs

Be good to see how some other packages do it. I feel like the must be a tidier way.

I think so too yeah. But this is how Spatie does it for their medialibrary package.

gavinhewitt avatar Jul 19 '22 10:07 gavinhewitt

Yeah, they only have a single model to worry about though. We have quite a lot!

Thinking maybe something like

gc_model('Product')::find(1);

Where the helper function gc_model() looks to see if anything is set in config, if not it assumes the standard models directory.

glennjacobs avatar Jul 19 '22 10:07 glennjacobs

Also, I don't want to have to list out every single model in config, only the ones a developer wants to override.

Maybe we need a little helper function/class.

I somewhat agree. Personally I think a developer should have complete freedom to decide what needs overriding and not be limited.

gavinhewitt avatar Jul 19 '22 10:07 gavinhewitt

Maybe we need a way for addons to register their own models too, so they too can be overridden using the same technique?

glennjacobs avatar Jul 19 '22 10:07 glennjacobs

Sure @gavinhewitt I can see your point with setting the models in the config but also see @glennjacobs point having to add all models in this way will certainly cause breaking changes not to mention zero IDE completion.

One idea I have which I will look further into next week which would mean no breaking changes in GC and complete developer flexibility is.

  • Add trait to BaseModel something like hasExtendedModel This trait will check the config for the model namespace as it could be App or even an Addon with the ability to extend the model, this new trait will have it's own boot method with the ability to call the matched extended model for example App\Models\ProductExtended where you will be able to add your own methods or override existing ones

Well thats my initial idea need to make a few tests, unless of course the GC team beat me to it 😃 I would propose the same for single action classes although we don't have too many of these right now. Used this package for a few projects recently https://laravelactions.com/ 😉

adam-code-labx avatar Jul 19 '22 10:07 adam-code-labx

We've used the Laravel Actions package before, in fact it was used in v1 of GetCandy. However, we decided against it for v2.

glennjacobs avatar Jul 19 '22 10:07 glennjacobs

One idea I have which I will look further into next week which would mean no breaking changes in GC and complete developer flexibility is.

  • Add trait to BaseModel something like hasExtendedModel This trait will check the config for the model namespace as it could be App or even an Addon with the ability to extend the model, this new trait will have it's own boot method with the ability to call the matched extended model for example App\Models\ProductExtended where you will be able to add your own methods or override existing ones

Sounds quite complex in comparison. I'm not sure if this would introduce issues. The only way to find out I guess is to knock something up.

glennjacobs avatar Jul 19 '22 10:07 glennjacobs

We've used the Laravel Actions package before, in fact it was used in v1 of GetCandy. However, we decided against it for v2.

Curious why you decided against it for V2

gavinhewitt avatar Jul 19 '22 10:07 gavinhewitt

We've used the Laravel Actions package before, in fact it was used in v1 of GetCandy. However, we decided against it for v2.

Curious why you decided against it for V2

We are quite strict on the third-party packages we use for GetCandy now. Part of our mission is to approach GetCandy in the most "Laravel way" as possible. Almost like "What would Taylor do?".

So packages must provide a lot of value and be well supported for us to consider them. Laravel Actions completely changed in v2, meaning we couldn't upgrade easily and it would have been a big chore to convert, so I don't want that again.

We don't see enough value in Laravel Actions to incorporate it.

glennjacobs avatar Jul 19 '22 10:07 glennjacobs

Yeah, they only have a single model to worry about though. We have quite a lot!

Thinking maybe something like

gc_model('Product')::find(1);

Where the helper function gc_model() looks to see if anything is set in config, if not it assumes the standard models directory.

The issue for me with this one is IDE completion I suppose we can inline but prefer my solution will put something together.

/** @var \Models\Product $product */
$product = gc_model('Product')::find(1);

or even create factory class then cleaner implementation to extend or swap from the service container or even from an Addon ServiceProvider ModelFactory::make('Product')->extend(Product::class);

adam-code-labx avatar Jul 19 '22 10:07 adam-code-labx

Yeah, a factory class is what I had in my head. So you can register the model overrides from anywhere and then easily load the correct one.

glennjacobs avatar Jul 19 '22 10:07 glennjacobs

Maybe it could be that you put something like this in your AppServiceProvider...

use GetCandy\Facades\ModelFactory;
use App\Models\Product;

ModelFactory::register('Product', Product::class);

And then to use the model, something like

use GetCandy\Facades\ModelFactory;

ModelFactory::resolve('Product')::find(1);

glennjacobs avatar Jul 19 '22 10:07 glennjacobs

Could even add autoloading namespaces, e.g.

use GetCandy\Facades\ModelFactory;

ModelFactory::addNamespace('GetCandy\Core\Models');

Would just need to consider priority of loading if more than one namespace added.

glennjacobs avatar Jul 19 '22 10:07 glennjacobs

Yeah it could work but seems a lot more work for you guys to change the codebase especially all the model relations. Adding a trait on the BaseModel which will auto register will perhaps be the simplest approach.

I imagine the ModelFactory would not be high priority for GC at this stage as there would be a lot of work to swap out every model and update all the unit tests in order to resolve the new model class?

adam-code-labx avatar Jul 19 '22 10:07 adam-code-labx

That is exactly it, swapping all the models out would be a big chore, hence we're not doing it right now.

If the trait can work, it might be an option.

glennjacobs avatar Jul 19 '22 11:07 glennjacobs

@glennjacobs @gavinhewitt Will look into putting together this temporary magical model trait this weekend and create my 4th PR 😂 Have a great weekend.

adam-code-labx avatar Jul 22 '22 23:07 adam-code-labx