-
-
Notifications
You must be signed in to change notification settings - Fork 191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert applyFiltersFromModel change #1126
base: develop
Are you sure you want to change the base?
Conversation
@mjauvin was there another issue that popped up? I thought we dealt with all of them now |
I thought so as well, but this is really hard to pinpoint where the problem comes from. Seems there is always a new case. That's why I think what we were trying to fix vs what we are breaking is not worth it, makes the Form widget unreliable. |
@mjauvin seems a bit like throwing the baby out with the bathwater. If there's no current issues with the code I don't see why we should revert it. |
There IS an issue, just didn't create it yet! 😝 |
Well then let's hear it! 😂 |
@mjauvin let me know if there's still an issue. |
@LukeTowers yes, this is still an issue. When using |
@LukeTowers I would not release 1.2.7 without reverting this. |
@LukeTowers I am adding a PR to the Winter.Test plugin to demonstrate the issue. |
The bug can be seen when using Winter.Test plugin under the Galleries model by creating/updating an gallery entry and selecting "active" status. This PR is needed to see the issue: wintercms/wn-test-plugin#20 |
This PR resolves the issue: #1170 |
When This is introducing more problems than it ever solved. |
@mjauvin what's the latest on this? |
It's not needed anymore, been replaced |
@mjauvin would you be able to clarify the state of where we're at with this problem? Your comment last week mentioned this PR was not needed, but now it has been re-opened. |
Yeah, I just don't know how to fix this anymore. I have tried many different ways, but everytime I eventually find a situation where it breaks. My best advice for now is to apply this PR to revert the initial changes. The case we were trying to solve in the first place is minor compared to all the other cases where it breaks it. |
@mjauvin does the same issues occur when you use the form events / callbacks inside the controller, as opposed to those in the model? I think I've been fairly open with my dislike for the modification of the form within the model, so it'll be interesting to hear the difference. |
What do you mean exactly ? |
Unless I was mistaken, the original issues were from using the EDIT: https://wintercms.com/docs/v1.2/docs/backend/forms#extending-form-fields |
@bennothommo you can use those methods, but they wouldn't be triggered by the |
I don't know myself. I tend to avoid this sort of magic and apply the rules to the models themselves through |
I guess your forms requirements are not as complex as mine then... |
Perhaps, and until someone presents a form configuration so complex as a test case, I'll remain blissfully unaware. |
All I'm saying is we should revert those changes as they break BC. FYI: I was able to do what I want using events in the controller like this: public function relationExtendManageWidget($widget, $field, $model)
{
if ($field != 'tasks') {
return;
}
$widget->bindEvent('form.refreshFields', function ($allFields) use ($widget) {
$fields = (object)$allFields;
// manipulate fields here as it it were in filterFields
});
} |
Revert changes to original behavior - too many problems.
Related: #927 #950 #1036 #1099