emteknetnz
commented
2 months ago I recommend we proceed with approach one, at a high level:
- Approach one overall reduces tech debt
- Approach two overall increases tech debt
Approach one:
A pair of draft PRs implementing the core of this are linked in the description.
I strongly think this is the correct approach compared to approach two:
- It allows us to remove the PHP code that processes element form data, thus we're only processing page data on the page form, not related data. In my view this is reducing tech debt
- It's removing the coupling with the parent edit form, meaning we could upgrade the parent edit form to something better and not have to make any changes to the element logic
The linked PRs resolve the following two issues:
- https://github.com/silverstripe/silverstripe-elemental/issues/1177
- https://github.com/silverstripe/silverstripe-elemental/issues/1179
Approach two:
Broadly this would be implemented as:
- Updating some PHP logic so that the validation failures in PHP get prefixed with the Element that triggered them e.g. "Title" failing validation in a subclass of BaseElement would become "ElementForm_1_Title"
- Exposing those particular validation errors from PHP to javascript, e.g. add them to getClientConfig(), or another mechanism e.g. data-validation result attribution on server rendered HTML.
- Get those validation errors read by a react component
- For every elemental ID in those values, ensure the formschema is fetched for those elements. The easiest way to do this is to just simulate a click on the element to open the inline form, this makes sense as we'll need to pop open the elements to display the errors
- Hook into the formSchema response before it goes into redux and before the formbulder form is created, and manually splice in the validation errors. We may be able to make use of one of the existing "overrides" API's in the Silverstripe redux code.
- Remove the values from the client config or whatever was used to get them into javascript, to ensure they're not reused when a new page edit form is pjaxed in
I strongly dislike this approach compared to solution one:
- We're adding a second source of truth for validation state, whereas with solution one everything is done via formSchema. This is adding new tech debt in my view.
- We shouldn't have two ways to submit elemental data, it's far better from a maintenance point of view to have a single code. I view the existing two ways of submitting data as tech debt. We're adding to this existing tech debt instead of reducing it
- Our redux is already confusing and no-one really likes it. The use of overrides would be adding more to the redux layer.
SPIKE to resolve https://github.com/silverstripe/silverstripe-elemental/issues/1179 and possibly also https://github.com/silverstripe/silverstripe-elemental/issues/1177
Currently when saving a page with an elemental area on it, the element data for all rendered elemental forms will be added to the POST request. This is a completely seperate code path from inline saving which uses FormBuilder/FormSchema
There are broadly two possible approaches here:
Timebox
3 days
Approach one:
Stop adding elemental data to the page POST request, and rely exclusively on using FormBuilder/FormSchema. On page save, block it on inline validating/saving the opened elemental forms
Approach two:
Keep elemental data in the page POST request. Somehow make the field validation message show in the correct place. Ineline elemental forms do not render by default so that'd need to be dynmically rendered when required.
Personally I (Steve) lean heavily towards approach one as it reduces complexity because there is now only one code path we need to look after, rather than two
New issues created
PRs