MissingSettings Exception thrown

[Muffinman]

Hi,

I was looking at the code which triggers the MissingSettings exception, to see if there was any way to conditionally disable this, or provide a default.

We're using Spatie multitenancy, and In our use case, we're loading settings quite early in one of the Tenant Switch actions. Currently these are throwing exceptions if settings don't yet exist, meaning we can't even run the setting migrations without discarding MissingSettings exceptions entirely in this code block.

    public function makeCurrent(Tenant $tenant): void
    {
        try {
            $paths = $this->originalPaths;
            $theme_name = ThemeSetting::theme_name();
            $paths[] = base_path("themes/{$theme_name}/resources/views");
            config()->set('view.paths', $paths);
            $this->updateViewFinder($paths);
        }
        catch (MissingSettings $e) {
            // If running in console we may be migrating the missing settings now!
            if (!app()->runningInConsole()) {
                throw $e;
            }
        }
    }

I think it would be good if the Settings class itself could provide a default in the property to get around this.

class ThemeSettings extends Settings
{
    // <- Why could it not fall back to this default from the class property?
    public string $theme_navigation = 'navigation_horizontal';

    public static function group(): string
    {
        return 'theme';
    }
}

I can see that it's possible to inject setting values with events, but that seems a bit more complex and verbose than the above. I was just wondering if there was a specific reason the class property was not used?

[rubenvanassche]

I don't think defaults are a good solution to this problem, since you will never know if your defaults are correctly replaced and they can become real values in context where you don't want them to be used. This will make debugging quite hard.

We're also using this package with the spatie tenant package and as a rule of thumb, we're never directly injecting settings where they can be picked up by running the app in console.

In this case I would catch the exception and then provide a default or probably don't inject any view.paths at all since you're running in console.

[Muffinman]

I think not allowing defaults is a real limitation of this package unfortunately. So much so we're considering replacing our implementation with https://github.com/akaunting/laravel-setting (which I really didn't want to do).

Our particular use case is quite complex since we have MultiTenancy, and each tenant may require their own group of settings. Currently this package requires us to migrate and set defaults in the database for ALL settings for ALL tenants even if 50% of the settings may be totally irrelevant for them.

Example: Each Tenant imports data from its own data sources depending on their backend software in use. We have all the tenant-specific code and settings for this tenant contained within their own package which we can quickly add / removed as needed.

Currently we're having to put blocks like this over their tenant-specific code, before trying to use any settings:

        // Make sure all settings exist
        try {
            $migrator = app(SettingsMigrator::class);
            $migrator->add('dear.api_key');
        } catch (SettingAlreadyExists $exception) {}

This feels really dirty imho.

I can think of another few non BC work-arounds:

1. Add an event to help provide a default if missing
2. Add a global call like Settings::ignoreMissing() which stops it throwing the exceptions and let's you save into the table
3. Allow each settings class to provide it's own set of defaults before throwing the exception:

class ThemeSettings extends Settings
{
    public string $theme_navigation;

    public static function group(): string
    {
        return 'theme';
    }

    // This would also be an option?
    protected static function defaults(): array
    {
        return [
            'theme_navigation' => 'navigation_horizontal',
        ];
    }
}

Would a PR be considered for any of the above?

[rubenvanassche]

Hi @Muffinman,

I've taken a look into the options, let's go with the third one where we provide a defaults method. I would accept a fully tested and documented PR for this if you're willing to make it.

[Muffinman]

Thank you, I will make and submit a PR!

Must also send your team a postcard!

[ralphmorris]

Hey,

Just wondering if there was an update on this? We have a requirement to add a lot of alert messages as settings to our filament CMS. However, adding preset values with the migrator for so many options feels a bit tedious and without them we get the MissingSettings exception. Would it be more user friendly to just return null if the key isn't set?

Cheers

Ralph

[swaty007]

you can do like this

1. Listen load settings event
   LoadingSettings::class => [
   LoadDefaultsSpatieSettingsListener::class,
   ],

2. Add to Settings Model default

   public static function defaults(): Collection
   {
   return collect([
   'promo_enabled' => true,
   'credit_pdf_enabled_test' => false,
   ]);
   }

3. Merge properties in listener
   class LoadDefaultsSpatieSettingsListener
   {
   public function handle(LoadingSettings $event): void
   {
   if (method_exists($event->settingsClass, 'defaults')) {
   app($event->settingsClass)::defaults()->each(function ($value, $key) use ($event) {
   if (!$event->properties->has($key)) {
   $event->properties->put($key, $value);
   }
   });
   }
   }
   }