Problem/Motivation
In general, \Drupal\Core\Form\FormBuilder is aware of ajax api requests and needs to be adjusted so that it is similarly aware of HTMX requests.
A thorny problem with dynamic elements was encountered in #3446642: [POC] Implementing some components of the Ajax system using HTMX. The form used as our discovery and development context for this POC, \Drupal\config\Form\ConfigSingleExportForm, has <select> elements with values that dynamically change based on user input.
With form builder as it is, if the form route is called with user input that is not a submit, the form rebuilds with a new build ID and fails to validate because the options get reset to their base case, and the user input no longer matches the available options.
Steps to reproduce
Once we have dependent issues committed, I'll update ConfigSingleExportForm without changing FormBuilder so others can see the issue and explore solutions.
Proposed resolution
Follow the pattern of the existing Ajax API and dynamically replace the form build ID input when the build ID value changes using hx-swap-oob.
Remaining tasks
Refactor FormBuilder
Add appropriate tests
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3535173
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3535173-support-dynamic-forms-only-with-test
changes, plain diff MR !13132
- 3535173-support-dynamic-forms-only-with-test-fix
compare
- 3535173-htmx-aware-forms
changes, plain diff MR !12942
- 3535173-htmx-aware-forms-tests-only
changes, plain diff MR !13126
- 3535173-support-dynamic-forms-only
changes, plain diff MR !13009
- 3535173-support-dynamic-forms
changes, plain diff MR !12918
Comments
Comment #2
fathershawnComment #4
fathershawnPosting some notes and code snippets from conversations in Slack. I'm not using the issue fork as we aren't really ready for that yet as there are still dependencies that need to be committed.
I loaded up the 11.x branch locally and spent more time stepping through
FormBuilder,FormAjaxResponseBuilder::buildResponse, andFormAjaxSubscriber::onExceptionwith xdebug.FormBuilder.php:436FormAjaxSubscriber::onException. The Ajax classes copy the new build ID back to the form.So I removed the previous code to maintain build_id from my POC branch and instead added the following immediately after the
$form['form_build_id']part of the form is built inFormBuilder::prepareForm.We don't have all the similar tools available yet, but this would be adding
data-hx-swap-oob='outerHTML:input[data-drupal-selector="existing-form-build-id"]'to the build id input element on the response.And that works! Validation is working, form_build_id is changing.
Comment #5
fathershawnComment #8
nod_we need #3522597: Return only main content for selected htmx requests before this works properly
Comment #10
fathershawnI pushed up a draft MR of how I imagine this implementation. I've commented out a condition that would depend on the upstream issue, but otherwise the single export form works here. My experience is that the form builder out of band swap needs to be deeper in the build process to prevent the validation error.
Nice edge case catch on the nested case with a full form swap @nod_!
Comment #11
nod_very nice. I couldn't make the config export form work on my end. I have no idea why setting the data-hx-swap-oob earlier makes the whole thing work… There is a side effect somewhere and couldn't figure out what it is yet.
I think the changes to the form are a bit worrying. We can't expect people to make this kind of changes to switch from ajax to htmx. I guess that's a concern for once things actually work.
Just realized we're going to have troubles with inline form errors, we need to swap the whole wrapping element, not just the select when it's updated.
Comment #12
fathershawnMaybe someone will see how to turn the ajax callbacks into process callbacks? I haven't experimented with that at all. Since this form is pure ajax, the form building logic was definitely distributed into the callbacks.
Switched to the
js-form-itemclasses. Were data attributes not a thing when those were added to our markup?Comment #13
fathershawnI looked over @nod_ latest update and our approach in FormBuilder is now very similar:
Our adaptations to
\Drupal\config\Form\ConfigSingleExportFormare very different, so I'll speak for what I am trying to accomplish. I want the code to be explicit, and to show the full htmx paradigm of request, select, target, swap.I also have a helper method on
FormBaseto determine which element triggered the request.Comment #14
fathershawnNice changes from @nod_ that take advantage of existing workflows for FormBuilder. Pulled those into my branch and we are now more closely aligned:
We still approach targeting the build_id element differently. nod_ adds an id - I go with the
drupal data selectorpattern.I also have a helper method on FormBase to determine which element triggered the form build. As I noted in #13 our approaches to refactoring
\Drupal\config\Form\ConfigSingleExportFormare different. I use the helper method to detect and respond to which element caused changes. My intention is to be explicit so some future developer (myself included!) sees the dynamic flow. It also allows me to use the HTMX push url attribute to update the url when a config value is actually exported.Comment #15
nod_it's not actually postponed.
Looks like the only big difference is the configexport form, which probably shouldn't be part of this issue so we can discuss the approach on how to htmx-ify the forms without holding up the necessary pieces in this MR
Comment #16
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #18
nod_Opened a MR with only the changes we converged on, I kept the swap-oob true, I could be convinced to do it like before but I don't want to keep track of css selectors in the backend.
Added a small condition so that form_build_id doesn't get the htmx attribute when its not an htmx request.
Comment #19
fathershawnThat looks great @nod_
Comment #20
fathershawnThis is a fine place to use that since the #id is reliable here. I offered the selector as a way to make fewer changes to the class.
Comment #23
nod_let's call it to have someone take a look :)
Comment #24
fathershawn+1 from me!
Comment #25
fathershawnMatching the priority to #3524030: Provide a factory object for HTMX headers and attributes as the new functionality in that issue will not work in forms without the changes in this issue.
Comment #26
catchCouple of questions on the MR.
Comment #27
fathershawnThe MR that prompted the move away from RTBC is hidden in the issue. Returning to RTBC after leaving a comment pointing to the final MR.
Comment #28
catchArgh OK, so the MR looks a lot simpler, but now there's no test coverage or conversion?
Comment #29
fathershawn@nod_ and I had different ideas about how to convert the
ConfigSingleExportFormform I've been using as an example.My understanding is that he converted it so that all three form elements were replaced on every response so that any changes that were caused by changing a value were updated in the form. Based on comments above he is demonstrating how easily convert a form.
I wanted to demonstrate the request/select/target/swap process that is so common in HTMX usage. My solution had more code that was conditional based on which select element triggered the change.
I think we both see merit in the other's approach but he decided to defer the conversation. If you would like to see the example and test @catch, we should try to reach consensus on the approach.
Comment #30
fathershawn@catch I brought our consensus changes to FormBuilder over to the MR 12942 and improved my test. I'd welcome your guidance on how to proceed with this issue.
Comment #31
nod_We should move the changes to the config form in a new issue like we have for #3538544: Ajaxify the user interface translation forms so that we can use the new Htmx object so that we can start using the helper.
Comment #32
fathershawnI'm deferring to @catch and @nod_ to reconcile the scope differences between #28 and #31 so that we can get this feature into 11.3
Comment #33
catchI think converting the config form in a new issue is fine, however I would somewhat expect that we have a test form implementation that does something similar-ish in this issue to show that the changes work. If it's a test form, it would be less of an issue if it's not using the attributes helper, we can add a @todo pointing to that issue.
Note that doesn't mean we have to add test coverage here, but it would be good to know where it's going to be added if not here.
Comment #35
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #38
fathershawnI took the MR from @nod_ and added a test that passes to create MR !13132 (3535173-support-dynamic-forms-only-with-test). Then I took the every thing except the changes to FormBuilder and applied them to the 11.x branch to create draft MR !13126 in which the test fails.
The test uses a simple form that abstracts the dynamics of the single export form.
Comment #44
nod_Comment #45
fathershawnNew test passes on MR !13132 and fails on the test-only job.
Comment #46
nod_Back to RTBC, we have tests
Comment #47
ghost of drupal pastThanks for the great work.
Does this (or perhaps a previous one? doesn't matter) need a change notice for proxies need to be configured so that the HX-Request header gets passed to Drupal. This shouldn't block the commit of this patch just some point before 11.3.0 it would be good to document it.
Comment #48
fathershawnComment #49
larowlanConfirmed that https://www.drupal.org/node/3539472 has notes about headers per request in #47
Comment #50
larowlanJust one question on the MR
Comment #51
nod_@fathershawn was right about build id replacement :)
I removed the id match because we can actually be very specific about what we replace. This will avoid instances of people adding an element on the page with the same id as the form_build_id input just to mess with things. It targets the element the same way the Ajax framework does it in
Drupal.AjaxCommands.update_build_id.Comment #52
larowlan#3524030: Provide a factory object for HTMX headers and attributes is in so this can be re-rolled and the todo resolved.
Comment #53
nod_Comment #54
nod_I think i got them all now
Comment #55
fathershawnReviewed and confirmed. back to RTBC!
Comment #56
catchCouple of minor points on the MR. Overall it's looking great.
The main overall question I have is how we'll eventually phase out the AJAX logic in here and more clearly delineate what's done for AJAX and what's done for HTMX and what's done for both, but I didn't have any good ideas when reviewing.
Comment #57
nod_I'm hopeful we won't much more htmx specific code in the backend side of things.
What I have in mind with regard to htmx: make the form work without JS and make htmx the ajaxify-ier of that form. We're currently looking at some ways to keep the callback mechanism of the ajax framework without the whole separate processing on the PHP side. I'm almost sure we could have that type of callbacks for non ajax forms, need to look into it more, in any case if it makes form processing more complex we're not going the right way. Fun times ahead :)
Comment #60
larowlanCommitted to 11.x
Published the change record.
Thanks folks.
Comment #63
gábor hojtsy