DarkGhostHunter / Laraconfig

Per-user settings repository system for Laravel
MIT License
173 stars 49 forks source link

Existing users are not initialized on migration #19

Open mralston opened 3 years ago

mralston commented 3 years ago

I have an existing codebase with many users already in the database. When I retrieve one of these existing users, their settings are not initialised. This results in the following error when I attempt to set the value of a setting on one of these users:

use App\Models\User;
$user = User::find(1);
$user->settings->set('dark_mode', true);

// RuntimeException with message 'The setting [dark_mode] doesn't exist.'

I have created a PR which resolves this by hooking into the retrieved Eloquent event within the HasConfig trait.

PR: Initialise when loading historic user records

DarkGhostHunter commented 3 years ago

Weird. Supposedly, when you push the setting to migrate, it will create settings for all users, so there is no need to "initialize them" on retrieval: these are already initialized.

DarkGhostHunter commented 3 years ago

If not, I would rather add a command to fill the settings to the historic users, rather adding an additional query check.

mralston commented 3 years ago

Hi!

Fair enough. I will double check that. It didn't seem to create any settings records for users when I migrated, but I'll do some testing.

Side note - I included a few corrections to the readme in that PR which you might want to merge independently. The readme says $user-config() instead of $user->settings(). I presume this changed at some point during development of the package.

mralston commented 3 years ago

Hello again.

So I've just taken another look at this and found two things:

  1. My proposed solution is a bad idea. If you load a large number of historic users at once (which my site's dashboard does) then the workload of creating user_settings records for each of them as they are retrieved is very expensive (I saw high processor usage and a slow page load).
  2. php artisan settings:migrate did not create any user_settings records for my existing users.

So I think that my proposed solution should be scrapped. Your suggestion of doing it on settings:migrate (or with a dedicated command) is the better approach but it doesn't seem to be working for me.

DarkGhostHunter commented 3 years ago

Gonna check it out. AFAIK, it "scans" the models for those having the package trait, and then queries them to make a bag of settings for each of them. If you have already some users, and doesn't create settings for them, then something is not working as intended.

mralston commented 3 years ago

Thank you, I'll keep my eye out.

DarkGhostHunter commented 3 years ago

No ETA BTW, hands full.

mralston commented 3 years ago

No worries. I wrote a quick and dirty command to do it in the meantime.

    public function handle()
    {
        $this->withProgressBar(
            User::select('id')->get(),
            function ($user) {
                $user->settings()->initialize();
            }
        );

        return 0;
    }

It doesn't look through all of the models which might have the HasConfig trait as I've seen your code does, but it's good enough for now.

Side note - it seems to take rather a long time to run (2 minutes to initialise 1 setting for 2104 users). It's probably just that iterating over and initialising every user separately is a rather inefficient way of doing it.