wintercms / wintercms/winter

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

Open
#1,375 3 comments 0 reactions 1 assignee View on GitHub

@LukeTowers is already working on this.

Since Jun 25, 2025.

needs response
Dominant language
PHP
Stars
1.5k
Forks
246
Avg merge
19h 2m
Merged PRs (30d)
7

Description

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 https://github.com/wintercms/storm/pull/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 https://github.com/wintercms/storm/pull/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 https://github.com/wintercms/winter/issues/1251#issuecomment-2484209350 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 https://github.com/wintercms/winter/issues/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 https://github.com/wintercms/storm/commit/6afe7a0385a2b9c275dbe1a7f9b033257b680299 would also work, but will reintroduce the bug it fixed.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.