@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.
@nod_ and I had different ideas about how to convert the ConfigSingleExportForm
form 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.
The 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.
Responded to feedback from @catch. I left his MR threads open so that it is easy for someone else to re-check with a goal of returning to RTBC?
✅ ✅ ✅ Indeed!
Merging
Thank you for working on this fix.
I generated a conflict by merging the admin theme issue. Can you resolve and update the MR - this looks awesome! I'll tag 1.5.0 as soon as this is merged.
Discussed, resolved questions and merged in remaining from #31 📌 DX object to collect and manage HTMX behaviors Active .
All tests passing.
fathershawn → changed the visibility of the branch 3524030-facade-simplified to hidden.
I was surprised at the performance similarity as the original approach will have less branching since we know the type to use. I then realized that the issue is in this code block in Attribute
// If the value is already an AttributeValueBase object,
// return a new instance of the same class, but with the new name.
if ($value instanceof AttributeValueBase) {
$class = get_class($value);
return new $class($name, $value->value());
}
Recreating the object takes time. I don't find any instances of renaming created AttributeValue objects in core and the whole topic is out of scope for this issue, but moving the type check up like so:
/**
* {@inheritdoc}
*/
public function offsetSet($name, $value): void {
// If the value is not an AttributeValueBase object, then create one.
if (!($value instanceof AttributeValueBase)) {
$value = $this->createAttributeValue($name, $value);
}
$this->storage[$name] = $value;
}
improved performance of the original code by 40%.
@nod_ Thanks for the ideas represented in your branch. I read through the changes and have some notes.
I removed the previously requested library optimization.
I profiled our two branches setting 10,000 string and 10,000 boolean attributes. In a single comparison they are within milliseconds of each other so there is no real performance difference.
The most important audience for the code, once it accomplishes its purpose, are humans. Since Attribute
is written to accept values of AttributeValueBase
I think explicitly creating attribute values of the correct type in an attribute factory clearly communicates what kind of attribute we are creating to the reader. That's valuable for immediate comprehension. If we need to go with your approach to get this committed, I'll support the pivot however I prefer the original code for these reasons.
Your change to the hx-name syntax from the data-hx-syntax reverses the decision in
🌱
[policy, no patch] Choose a markup strategy for HTMX POC
Fixed
. I don't have a strong opinion either way but it surprised me. Should we re-open and document the change?
I've been thinking about the power and flexibility HTMX gives through using CSS selectors and looking again at this MR. I think we can adapt what's here because we can use a selector to get all the children of a message type wrapper or all the children of the outer message wrapper.
We don't have to target the existing Ajax API, so we don't have to fix any tests that are using the current Ajax API.
Option 1: append new messages as new children of the element with `data-drupal-messages`. This is the simplest but the markup is not as nice. Each batch of messages would repeat the message type wrappers. But nothing needs to exist on the page except the element with `data-drupal-messages`.
Option 2: add a data attribute to the message type wrapper element, such as `data-drupal-message-type="error"`. This would allow us to select messages of each type and insert them into the existing wrappers. The downside is the wrapper elements need to exist even if the are empty.
You all have been thinking about this longer though. So here is the core question: If you could use CSS to select the messages from a new HTML response and then use CSS to select the element to recieve those messages, how would you all set up the markup?
Matching the priority to 📌 DX object to collect and manage HTMX behaviors Active as the new functionality in that issue will not work in forms without the changes in this issue.
Updated the CR to match the revised approach.
All tests passing. Most of this MR is code moved from the the two prior RTBC issues. Comments on the MR indicate the overlap.
There you go!
fathershawn → created an issue.
fathershawn → changed the visibility of the branch 3522597-wrapper-format-template-assets to hidden.
+1 from me!
Discussed in Slack with @nod_ (See comment on the attributes issue 📌 DX object to manage htmx attributes Active ) and we are moving this work into 📌 DX object to collect and manage HTMX behaviors Active
The empty URL behavior discussed in #48 & #49 is in the issueAjaxRequest
function of htmx.js
As of this note this can be seen at https://github.com/bigskysoftware/htmx/blob/master/src/htmx.js#L4426
Turns out this goes back to RFC 2396 on the URI specification co-authored by Tim Berners-Lee in 1998 no less!
4.2. Same-document References
A URI reference that does not contain a URI is a reference to the current document...
Thinking about #44 brought a tickle in the back of my mind to full thought. I think we've gotten down a wrong path on this issue again. I started thinking about the use case where what the developer needed was to replace the entire body tag as it would be rendered by the active theme. Then I started thinking about refreshing a dynamic block. Which leads me to these parameters:
- We launch in core with the routes that core provides.
- Developers need to be able to request and select any markup.
The ajax api was able to return partials via the callback system. We have the tools to add routes that render entities in specified view modes, render blocks, even render individual fields from entities. We are there yet and even then those will be routes.
I wondering if the route option is the best way to craft a minimal response? Or should we build the wrapper format such that it is not expected but available to be added in individual cases?
All tests passing
Made all five http verb methods operate the same way.
Re: #48 - okay. It's not documented at
- https://htmx.org/attributes/hx-post/
- https://htmx.org/attributes/hx-delete/
- https://htmx.org/attributes/hx-patch/
- https://htmx.org/attributes/hx-put/
as it is for get. Did you find support for all five in the code @nod_? It will make our code simpler so I'm going ahead. It would be good to document in a comment here for future reference with a link to their code or other documentation.
Required tests passing. The php 8.5 test seems to be failing in general due to widespread deprecations.
Unrelated toolbar tests failing. Rebasing onto HEAD.
few cosmetic feedbacks
Suggested changes made.
I also offer this regular expression: https://regex101.com/r/WMznxt/3 that leverages the template markup I'm offering if we don't want to depend on html parsing to get the asset markup.
I'm happy to write a test for the renderer when we come to consensus on the output.
Here's the output from the templated assets version for our test page response
<!doctype html>
<html>
<head>
<title>Ajax Content</title>
</head>
<body>
<div class="dialog-off-canvas-main-canvas" data-off-canvas-main-canvas>
<div class="layout-container">
<header role="banner">
</header>
<main role="main">
<a id="main-content" tabindex="-1"></a>
<div class="layout-content">
<div data-drupal-messages-fallback class="hidden"></div>
<h1>Ajax Content</h1>
<div class="ajax-content">Initial Content</div>
</div>
</main>
</div>
</div>
<template data-drupal-htmx-assets>
<link rel="stylesheet" media="all" href="/core/modules/system/tests/modules/test_htmx/css/style.css?t13305" />
<script type="application/json" data-drupal-selector="drupal-settings-json">{"path":{"baseUrl":"\/","pathPrefix":"","currentPath":"htmx-test-attachments\/replace","currentPathIsAdmin":false,"isFront":false,"currentLanguage":"en","currentQuery":{"_wrapper_format":"drupal_htmx","ajax_page_state":{"theme":"stark","theme_token":"null","libraries":"core\/drupal.htmx,system\/base"},"replace":""}},"pluralDelimiter":"\u0003","ajaxPageState":{"libraries":"eJxLzi9K1U8pKi1IzNHLKMmt0CmuLC5JzdVPSixO1SlJLS6JB4nqJxYXp5YUAwCThhG5"},"user":{"uid":0,"permissionsHash":"063738de691cc2450c1e8bd886e26fe4403ae290bb2b7031ca3342ee6b15822c"}}</script>
<script src="/core/assets/vendor/once/once.min.js?v=1.0.1"></script>
<script src="/core/modules/system/tests/modules/test_htmx/js/behavior.js?v=11.3-dev"></script>
</template>
</body>
</html>
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.
This 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.
That looks great @nod_
nicxvan → credited fathershawn → .
Nice 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 selector
pattern.
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\ConfigSingleExportForm
are 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.
I looked over @nod_ latest update and our approach in FormBuilder is now very similar:
- I have helper methods to check the request if it's from HTMX and to check for the HTMX trigger value which also makes our code differ in a few places but we are checking the same things.
- We approached targeting the build_id element differently. nod_ adds an id - I go with the existing input[name="form_build_id"][value="' . $old_build_id . '"]' selector for the build id element.
- I extend elementTriggeredScriptedSubmission using the detected trigger from HTMX since any element type can be the trigger.
Our adaptations to \Drupal\config\Form\ConfigSingleExportForm
are 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 FormBase
to determine which element triggered the request.
The use of a template wrapper inspired me to make the markup group the assets that we are looking for. I'm also thinking that with a bit more work on asset processing we can use the same template approach in the refactored bigPipe.
I left off template wrapping the content so that our approach stays consistent with HTMX documentation (https://htmx.org/docs/#troublesome-tables) . We also can document the need for parsable HTML. For direct select/swap a developer can wrap the content that they really want in the needed structure and use an appropriate CSS selector to get the elements that they want.
Thank you for offering your contribution!
We have two forms using this deprecated service. The other is \Drupal\dynamic_yield\Form\FeedSettingsForm
.
Also, the recommended replacement service has a different cache clear method, so that will also need to be changed in the form.
Tests passed
fathershawn → created an issue.
fathershawn → created an issue.
fathershawn → created an issue.
seantwalsh → credited fathershawn → .
This is a great idea and a great start!
This looks correct - there are linting issues to be fixed.
This is a good idea! I think we need this in two places: The exposed form, if it is used, and the mini-pager.
I'm seeing double escaping in the output from our test page. I have a refactoring idea that could also benefit our big pipe needs. I'll post some code soon.
Nice! We need a test and to clean up some lint
Thanks! That is clearly a bug. The debug JS is here and not in core.
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.
Maybe 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.
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.
Switched to the js-form-item
classes. Were data attributes not a thing when those were added to our markup?
I 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_!
What if we moved these lines and the associated ::processAssetLibraries
method from HtmlResponseAttachmentsProcessor
into an asset processor service?
$ajax_page_state = $this->requestStack->getCurrentRequest()->get('ajax_page_state');
$assets->setAlreadyLoadedLibraries(isset($ajax_page_state) ? explode(',', $ajax_page_state['libraries']) : []);
$variables = $this->processAssetLibraries($assets, $attachment_placeholders);
We now have two situations where we would like to get just the asset markup: here and in BigPipe.
As a side note, we can also prevent direct access to this renderer by checking for the HX-Request
header.
Discussed with @nod_ yesterday. He pointed out that we can communicate in a CR that themes need to apply the data-drupal-messages
attribute to their wrapper element.
This is really good @nod_! Setting to "needs work" for a test for the new renderer. I'm also recommending that we hold FormBuilder changes for 📌 Support dynamic forms using HTMX Active
Thanks @nod_ !!
Removed the twig function from this issue's MR and the change record. It is extra.
CR is updated with the full picture of how all 4 issue changes work together.
@catch @larowlan @nod_ A CacheableMetada parameter was added to methods that take a URL based on feedback. This complicates calling these methods within Twig as the caching has already bubbled.
Does this change the utility of a providing the attribute object within Twig? Should we drop that function from this MR?
@nicxvan First, thank you for so carefully checking the tedious mundane details! Suggestions committed and test updated for the one method name fix.
Sanitization happens in the attribute value object's ::__toString
method. They all extend AttributeValueBase which sanitizes the name property. We add AttributeJson
in this MR, and follow this pattern.
Thank you @phenaproxima for improving the code with your questions and suggestions. I've resolved all comments with either updates or answers except those related to return types (see #30). All tests are passing.
@catch can you give guidance about adding return types to \Drupal\Core\Template\Attribute
?
I hit a name collision on the BC policy with the Attribute name (As you and @nicxvan noted that was for PHP attributes). But it also seems to be permitted with these:
PHP and JavaScript classes →
In general, only interfaces can be relied on as the public API. With the exception of base classes, developers should generally assume that implementations of classes will change, and that they should not inherit from classes in the system, or be prepared to update code for changes if they do. See also more specific notes below.
Public methods not specified in an interface →
Public methods on a class that aren't on a corresponding interface are considered @internal.
We are adding an interface in this issue, but there no interface for this class now. @larowan seemed to think it was okay here: https://git.drupalcode.org/project/drupal/-/merge_requests/12097#note_51...
I brought the summary up to date with the code. We have 5 unresolved comment threads on the MR. Most open threads have a response.
Following a comment from @catch I updated the code in \Drupal\Core\Template\HtmxAttribute
for:
- ::get
- ::post
- ::put
- ::patch
- ::delete
- ::pushUrl
- ::replaceUrl
to pass in and instance of CacheableDependencyInterface
and collect the cacheable metadata emitted by the Url object.
There is an open question about the advisability of adding return types in \Drupal\Core\Template\Attribute
. I understand this class to be internal by policy but maybe that's not right?
The other 3 open threads all have answers.
I have time to move this project forward to let's get this to RTBC so it can be committed and work can start on the next layer!!
I very much affirm HTML as the data format.
In my last draft I was sending individual messages as HTML but I now think that all we need to do is render the messages normally using '#theme' => 'status_messages'
. If the theme provides the selector for the wrapper element, with fallback to core div[data-drupal-messages]
we can start get the innerHTML of the incoming wrapper and swap it into the existing wrapper in the DOM using the HTMX beforeend
strategy.
Reading through the governance doc I am marking this for framework manager review since it is blocking other work.
We are also on a learning journey about the breadth of interactive hypermedia that HTMX can allow. I think we are going to enjoy how much interactivity we can create without the weight of an SPA framework.