Model's beforeSave method is called before Nullable trait is run

[Fl0Cri]

Winter CMS Build

1.2

PHP Version

8.3

Database engine

MySQL/MariaDB

Plugins installed

No response

Issue description

When a beforeSave method is defined on a Model, it is called before anything that may be bound to the internal "model.beforeSave" event. This is especially apparent with the Winter\Storm\Database\Traits\Nullable trait, where it is counterintuitive to receive an empty string for a nullable property in beforeSave.

This behavior did change in wintercms/storm#192. When doing this change, I thought that the only difference between bindEvent and bindEventOnce would be the fact that the later is removed after being run.
It turns out that they have a very different behavior regarding priority: events bound "once" are always triggered before any other event and don't support priority at all. Here is the code doing this.

I see that @mjauvin did some work about this in wintercms/storm#200. The PR was closed automatically, not sure if it is still planned but it could help to fix this issue.

Alternatively, the first fix in #1251 (comment) could work better than the current one. Its drawbacks at the time were the following:

1. beforeValidate methods added using $model->addDynamicMethod() would not be called. I may be wrong, but I think it sould be quite uncommon: (a) it only works for some trait methods, most other ones have an empty implementation in the base Model class that cannot be overridden using addDynamicMethod. (b) This could conflict with another plugin doing the same or if the extended model is updated. (c)The model.beforeValidate event can be used instead and doesn't have these problems.
2. It changes the order in which events are triggered. That's what happened with the current fix, but this one would allow to set a priority to mitigate the issue without touching the Event Emitter.

Note: issue #1360 has the same root cause.

Steps to replicate

Take a model that uses the Nullable trait and has a beforeSave metod, for example:

class Test extends Model
{
    public $table = 'tests';

    use Winter\Storm\Database\Traits\Nullable;
    public array $nullable = ['foo'];

    public function beforeSave()
    {
        $this->foo ??= 'bar';
    }
}

Add foo as a text field to the fields.yaml.

In the backend, try to leave foo empty and save. Because the Nullable trait has not run yet, foo === '' instead of null and the value stays '' instead of defaulting to 'bar'.

Workaround

Manually calling the nullableBeforeSave() of the Nullable trait when needed can help. The null checks will still occur a second time later, but it shouldn't be an issue.

function beforeSave()
{
    $this->nullableBeforeSave();

    $this->foo ??= 'bar';
}

Reverting wintercms/storm@6afe7a0 would also work, but will reintroduce the bug it fixed.

[bennothommo]

@Fl0Cri I'm hesitant to get into the debate around event prioritization (I think it's an absolute minefield, and I'm glad Laravel got rid of it).

However, I feel like the example you've provided here doesn't really demonstrate an issue that's really major. For one, I wouldn't necessarily agree that the Nullable trait running after the model's beforeSave is counter-intuitive - if anything, it probably makes more sense for it to run afterwards so that people have the opportunity to "empty" attributes in beforeSave before they are converted to null. The intent of the Nullable trait is to ensure that the values are stored as null if they are empty.

If you want the value of foo to be stored as bar if it is null, it would defeat the purpose of the trait, because the value would never be null and thus never stored as null.

I would argue that the original fix solved a bigger problem, and if anyone really wants to enforce an order for a set of actions, they should be using a queue instead. But if you really wanted to use a beforeSave in this fashion, you can always define it in the boot process of the model and set the priority to after the trait has run.

[Fl0Cri]

Well I agree that playing with priorities could quickly get messy, especially if the core needs to rely on arbitrary hardcoded values. In fact, something like wintercms/storm#200 could remove the hidden priority bindEventOnce gives compared to bindEvent and result in a more natural "first in first out" behavior. This may fix some issues like this one and #1360 (even if it seems to have worked by chance).

To be clear, I don't think it's a major issue either and wouldn't have expected it to work if I didn't see it work in the past. Feel free to close it if you think it is not worth "fixing". But I still wanted to make this feedback, as the change I submitted has consequences I didn't expect and point to alternatives in case you are seeing many issues like this one.

I maybe wrongly assumed that you might be wanting to revert the original fix when reading the following statement from #1360.

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.

About my code example, well I over-simplified it to something that makes no sense... In reality, I have a tri-state "force some option" field made using a dropdown (true, false, or empty/null to indicate an automatic choice). In beforeSave, I used to do something like $this->some_value = ($this->force_value ?? $this->shouldHaveValue()) ? $this->calculateValue() : null. The Nullable trait was previously doing the null if empty but false if zero check for me, but I can do it myself if you think it's not its job.

[LukeTowers]

I'll look into this today.