v1.2.4 breaks filterFields behavior
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.
Are you saying that commenting out the call added in PR #927 fixes the problem?
@mjauvin Yes it did
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 I need this to work in the create context as well. It worked pre 1.2.4.
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());
}
Could it be that at this point the model attributes are not yet populated? Like I said, reverting this change fixes the issue
In create context, the model has not been created yet, so this is expected behavior.
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 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.
That's my take as well, unless we're missing something.