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

SPIKE Look at solutions between field validation conflicting between block level and page level #1204

Closed
emteknetnz opened this issue Jun 13, 2024 · 2 comments

Comments

@emteknetnz
Copy link
Member

emteknetnz commented Jun 13, 2024

SPIKE to resolve #1179 and possibly also #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

  • Implement inline submitting elements on page/parent save

PRs

@emteknetnz emteknetnz changed the title SPIKE Look at solutions between field validation is conflicting between block level and page level SPIKE Look at solutions between field validation conflicting between block level and page level Jun 13, 2024
@emteknetnz emteknetnz self-assigned this Jun 17, 2024
@emteknetnz
Copy link
Member Author

emteknetnz commented Jun 26, 2024

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:

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.

@emteknetnz
Copy link
Member Author

Team has agreed to proceed with approach one

New issues created to implement - #1215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant