winter icon indicating copy to clipboard operation
winter copied to clipboard

v1.2.4 breaks filterFields behavior

Open msimkunas opened this issue 1 year ago • 10 comments

Winter CMS Build

Other (please specify below)

PHP Version

8.1

Database engine

MySQL/MariaDB

Plugins installed

No response

Issue description

The following PR breaks existing filterFields behavior: https://github.com/wintercms/winter/pull/927

Steps to replicate

Try to access a model attribute via $this, e.g. $this->someAttribute, in the filterFields method. Before v1.2.4 the model attributes are populated but after v1.2.4 they are all null.

Example use case:

public function filterFields($fields, $context = null)
{
    if($this->someExistingAttribute) {
        throw new \ApplicationException('some message');
    }
}

The above never evaluates to true because $this->someExistingAttribute is null.

Workaround

No known workaround.

msimkunas avatar Jan 14 '24 18:01 msimkunas

Are you saying that commenting out the call added in PR #927 fixes the problem?

mjauvin avatar Jan 14 '24 19:01 mjauvin

@mjauvin Yes it did

msimkunas avatar Jan 14 '24 19:01 msimkunas

In your filterFields method, just use the $context argument to make sure you're in update context. In create context, the model's fields have not been set yet.

mjauvin avatar Jan 15 '24 14:01 mjauvin

@mjauvin I need this to work in the create context as well. It worked pre 1.2.4.

msimkunas avatar Jan 15 '24 14:01 msimkunas

The only thing different is the call to applyFiltersFromModel() in getSaveData().

And the only thing this does is call your model's filterFields() method:

    protected function applyFiltersFromModel()
    {   
        /*
         * Standard usage
         */
        if (method_exists($this->model, 'filterFields')) {
            $this->model->filterFields((object) $this->allFields, $this->getContext());
        }

mjauvin avatar Jan 15 '24 14:01 mjauvin

Could it be that at this point the model attributes are not yet populated? Like I said, reverting this change fixes the issue

msimkunas avatar Jan 15 '24 14:01 msimkunas

In create context, the model has not been created yet, so this is expected behavior.

mjauvin avatar Jan 15 '24 14:01 mjauvin

Updating to 1.2.4 breaks use cases where the developer is checking the model attributes in the create context.

I think it’s a valid use case because you may want to have conditional logic in filterFields during both create and update flows

msimkunas avatar Jan 15 '24 14:01 msimkunas

@msimkunas could you in your listener just check to see if the relevant attributes are set yet and return early if they are not? It will still be fired where it was before, this change just added another point in the request lifecycle where it could be fired.

LukeTowers avatar Jan 15 '24 22:01 LukeTowers

That's my take as well, unless we're missing something.

mjauvin avatar Jan 15 '24 22:01 mjauvin