Laraconfig icon indicating copy to clipboard operation
Laraconfig copied to clipboard

Existing users are not initialized on migration

Open mralston opened this issue 2 years ago • 8 comments

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

mralston avatar Aug 30 '21 13:08 mralston

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 avatar Aug 31 '21 17:08 DarkGhostHunter

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

DarkGhostHunter avatar Aug 31 '21 17:08 DarkGhostHunter

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.

  • Matt

mralston avatar Aug 31 '21 17:08 mralston

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.

  • Matt

mralston avatar Aug 31 '21 18:08 mralston

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.

DarkGhostHunter avatar Aug 31 '21 18:08 DarkGhostHunter

Thank you, I'll keep my eye out.

mralston avatar Aug 31 '21 20:08 mralston

No ETA BTW, hands full.

DarkGhostHunter avatar Sep 01 '21 02:09 DarkGhostHunter

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.

mralston avatar Sep 02 '21 21:09 mralston