Changes made in settings model's beforeSave function are lost immediately afterward

[interworks-morr]

Winter CMS Build

1.2

PHP Version

8.3

Database engine

MySQL/MariaDB

Plugins installed

No response

Issue description

While testing the upgrade from Winter CMS 1.2.3 to 1.2.7, an automated test began failing because changes to a settings model in its beforeSave function are lost immediately after the beforeSave function returns. I have traced it down to the Winter\Storm\Database\Model::bootNicerEvents function where it fires $model->fireEvent('model.beforeSave').

I added a dump at the end of the beforeSave and the attributes are what I expect. I added a dump just after this event is fired and the attributes are reverted.

After Luke's hint to look at this commit, I changed bindEventOnce to bindEvent in the bootNicerEvents function and it started working as expected, so this commit is the likely cause of the issue. However, the root cause may be something deeper inside of the bindEventOnce function specifically for settings models.

Steps to replicate

1. Create a settings model with some simple text fields.
   1. Run something like php artisan create:settings Company.Plugin TestSettings
   2. Add the following to the fields.yaml file:

fields:
    foo:
        label: Foo
        type: text
    bar:
        label: Bar
        type: text

2. Register the backend settings page as needed to point to this new settings model.

            'test'  => [
                'label'       => 'Test Settings',
                'description' => 'Test settings.',
                'icon'        => 'icon-lock',
                'category'    => 'Testing',
                'class'       => 'Company\Plugin\Models\TestSettings',
                'order'       => 30,
                'keywords'    => 'testing',
                'permissions' => ['company.plugin.*'],
            ],

3. Add a beforeSave function to the settings model and override the value attribute with something noticeable.

    public function beforeSave()
    {
        $value                     = json_decode($this->attributes['value'] ?? '[]', true);
        $value['foo']              = 'nonsense';
        $value['baz']              = 'more nonsense';
        $this->attributes['value'] = json_encode($value);
    }

4. Open the backend settings page, change the values in the text fields, and click the save button.
5. Check the system_settings table to verify that it accepted the values on the form and whether it accepted the changes from the beforeSave function.
6. Modify Winter\Storm\Database\Model::bootNicerEvents to use bindEvent instead of bindEventOnce and repeat steps 4-5 again.

Workaround

It appears that beforeValidate does not exhibit this same behavior, so it can be used instead if needed/possible.

[LukeTowers]

@interworks-morr could you provide the listener that was making changes and the test case that was failing?

[interworks-morr]

@LukeTowers the actual listener and test I was using are very bespoke and overly complicated, so I added more details on the example I used to recreate the issue under the Steps to replicate section above.

[Fl0Cri]

Just posting my two cents here as it's related to a commit I made and some code paths I xdebug'd in the past.

In this case, $this->attributes['value'] is overwritten here

winter/modules/system/behaviors/SettingsModel.php

Line 226 in 9f1dd90

$this->model->value = $this->fieldValues;

The fact that using bindEvent instead of bindEventOnce does fix this issue is because the Emitter first runs all "once" events, before running recurring events. That's something I didn't see when evaluating potential side effects.

Note that using bindEvent will probably reintroduce #1251.

Looking at the SettingsModel, fighting with it for $this->attributes['value'] may not be a good idea... @interworks-morr maybe using setSettingsValue could do the trick ?

public function beforeSave()
{
    $this->setSettingsValue('foo', 'nonsense');
    $this->setSettingsValue('baz', 'more ' . $this->getSettingsValue('foo'));
}

@LukeTowers is wintercms/storm@626409c a source of trouble ? If so maybe one of the other ways discussed in #1251 may work better ?