Skip to content
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

Standardising the saving of form widgets #1222

Open
bennothommo opened this issue Oct 8, 2024 · 0 comments
Open

Standardising the saving of form widgets #1222

bennothommo opened this issue Oct 8, 2024 · 0 comments

Comments

@bennothommo
Copy link
Member

bennothommo commented Oct 8, 2024

We've had a number of commits and PRs change the behavior of saving form widgets to fix little issues that pop up in certain form widgets. Unfortunately, I feel this has cascaded into using band-aids to keep the boat afloat - instead of fixing the root problem of having inconsistent saving behavior in the widgets themselves, we've been applying more and more fixes to the getSaveData method in the Form widget to try and account for these little issues and it's creating more and more complexity and edge cases.

At this point, we should try and standardise the whole process of loading and saving data from a form and ensuring the form widgets all behave in a predictable and consistent manner.

The following are the rules I believe should apply:

Loading fields

  • Fields are loaded in the order they are specified in the fields.yaml config, under the tabs specified.
  • If a field is marked as hidden, either in the config or through a callback or event such as filterFields, it does not display on the form and its data is not included in the final form data.
  • If a field is marked as disabled, either in the config or through a callback or event such as filterFields, it is displayed on the form in a disabled state, but its data is not included in the form data.
  • All form widgets and fields have the raw loaded data fed through its getLoadValue method, transforming the data if necessary into a format that allows the field or widget to function in the interface.
  • Only in the create context, and only when no form save has taken place, will a form widget or field use a default value if provided. This value is pass through the getLoadValue method as with raw loaded data.

Saving fields

  • All active fields (ie. fields not marked as disabled or hidden) must send POST data, even if empty. The POST data should reflect an empty state in a way that the field or widget can process an empty state correctly.
  • Any fields not included in the POST data are to be ignored and not saved.
  • The POST data will be fed to each field and widget through its getSaveValue method, transforming the data if necessary into a format that allows the field to be saved.
  • The getSaveValue method must return any value except FormField::NO_SAVE_DATA (which should be changed to a unicode null character), which is used to determine if a field is not to be saved at all. This means that an empty string, 0, -1, false and null are all considered valid values.

References:

LukeTowers pushed a commit to wintercms/wn-sitemap-plugin that referenced this issue Oct 9, 2024
LukeTowers pushed a commit that referenced this issue Nov 13, 2024
Reverts changes to original behavior - too many problems.

Related: #1222

#927 was originally intended to solve the issue logged in #950 where if you hide a field by default, and then want to unhide it dynamically in filterFields() it does not get saved because disabled and hidden fields are skipped by default in Form->getSaveData(); so the proposed fix was to apply model filtering logic immediately before processing the save data; however that caused issues (#1036) because the model was no longer populated the same way when calling filterFields() at that point in the request and the filterFields() method could be called multiple times.

The solution to the original issue would have been to use the client side Trigger API instead; and potentially we could have solved the other issue by looking at how we're populating the model before calling filterFields() but as this functionality isn't well tested we'll just revert for now.

The second change being reverted is #1099 which was an attempt to solve an issue encountered by someone that was caused by the method being called multiple times.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant