lara-zeus / bolt

form builder for your users, with so many use cases
https://larazeus.com/bolt
MIT License
186 stars 32 forks source link

[Bug]: override/array_merge of zeus-bolt.php config models does not work correctly with BoltPlugin::getModel('') #291

Closed eelco2k closed 5 months ago

eelco2k commented 5 months ago

What happened?

in Configuration.php the getModel() function merges the config models array but the params should be flipped. otherwise the config models will be overriden and will not be used, and BoltPlugin::getModel('Category') will always be the package Category Model (\LaraZeus\Bolt\Models\Category::class) for example..

should be fixed when using the function like below:

public static function getModel(string $model): string
    {
        return array_merge(
            (new static())::get()->getBoltModels(),
            config('zeus-bolt.models')
        )[$model];
    }

How to reproduce the bug

override config classes in zeus-bolt.php file. and use BoltPlugin::getModel('Category') .

Package Version

3.0.39

PHP Version

8.3

Laravel Version

10.48.11

Which operating systems does with happen with?

No response

Notes

No response

atmonshi commented 5 months ago

This issue has been brought to my attention so many times, and I really don't get it!

The reason is that you can overwrite your config file from any panel. There are some use cases for that with tenants.

The current code is correct:

public static function getModel(string $model): string
    {
        return array_merge(
            config('zeus-bolt.models'),
            (new static())::get()->getBoltModels()
        )[$model];
    }

it will take the default models from the config file, and merge it with your panel models, so you can do:

->boltModels([
    'Category' => CategoryModelFromPanel::class,
])

and in your config:

'models' => [
        'Category' => CategoryFromConfig::class, // notice this
        'Collection' => \App\Models\Zeus\Collection::class,
        'Field' => \App\Models\Zeus\Field::class,
        'FieldResponse' => \App\Models\Zeus\FieldResponse::class,
        'Form' => \App\Models\Zeus\Form::class,
        'FormsStatus' => \LaraZeus\Bolt\Models\FormsStatus::class,
        'Response' => \App\Models\Zeus\Response::class,
        'Section' => \App\Models\Zeus\Section::class,
    ],

and the output will be:

Screenshot 2024-05-30 at 11 36 28 PM
eelco2k commented 5 months ago

Okay I understand the explanation of adding

->boltModels([ 'Category' => CategoryModelFromPanel::class, ])

But why is the published config file then? When I modify the models array there it does not do anything. As it still takes the models array in Configuration.php

eelco2k commented 5 months ago

I would then think the following override priority would be: Start with 1st base: 1st Configration.php model array /package Zeus-bolt.php config 2nd Zeus-bolt.php published config model array 3th $panel ->boltModels()

atmonshi commented 5 months ago

if you dont have custom models why adding panel ->boltModels? it will read from the config file which is the default for the frontend and all model relations

atmonshi commented 5 months ago

I will remove the default array in the Configration.php so it will only be the config file or the panel ->boltModels

eelco2k commented 5 months ago

That is just the problem:

I published the Zeus-bolt.php config file. And changed the Category::class to my custom model in like so App\Models\Category::class for example.

But then still the value of Boltplugin::getModel('Category') is the

\LaraZeus\Bolt\Models\Category::class

and not: App\Models\Category::class

atmonshi commented 5 months ago

can you test now the new version v3.0.45

eelco2k commented 5 months ago

👍🏻

eelco2k commented 5 months ago

I also think that Concerns\Schemata.php Line: 213

  ->getOptionLabelFromRecordUsing(fn (Category $record) => $record->name),

should be more generic as it can be overridden in config or $panel ->boltModels(). Like so:

  ->getOptionLabelFromRecordUsing(fn (Model $record) => $record->name),
eelco2k commented 5 months ago

but the change works! i Tested it.

atmonshi commented 5 months ago

I also think that Concerns\Schemata.php Line: 213

  ->getOptionLabelFromRecordUsing(fn (Category $record) => $record->name),

should be more generic as it can be overridden in config or $panel ->boltModels(). Like so:

  ->getOptionLabelFromRecordUsing(fn (Model $record) => $record->name),

your model should extend mine so it wont be any issues, also better this way in case any updates or additions to the models.

also changing to Model will make stan crazy :)

eelco2k commented 5 months ago

I tried extending the Category Model. And also created a CategoryFactory but the problem is this then:

Declaration of App\Models\Bolt\Category::newFactory(): Database\Factories\Bolt\CategoryFactory must be compatible with LaraZeus\Bolt\Models\Category::newFactory(): LaraZeus\Bolt\Database\Factories\CategoryFactory

this is because return type is the package CategoryFactory. so then this should be more general:

use Illuminate\Database\Eloquent\Factories\Factory;

 protected static function newFactory(): Factory
    {
        return CategoryFactory::new();
    }

and if this is used in all Bolt Models all the models should return Factory as newFactory() return type.

atmonshi commented 4 months ago

coming on #297 👍