🇫🇷France @nod_

We already have a test with multiple body tag, that's why I added the check to the value of the hitTheFloor variable, see test-only CI run: https://git.drupalcode.org/issue/drupal-3533698/-/jobs/5753271

Thanks for keeping things in check!

@fathershawn you didn't misunderstand, I think I didn't explained it clearly enough.

My initial plan was quick and dirty: do the replacement of the ajax commands only so we can get that done quickly and move on. When you added your MR that went with a more HTMX way of doing it, it looked interesting: changing ajax commands for a meta header and raw HTML looked like a great improvement, and you were really involved in making it work so I though it'd be good to try it out. It started to get hard with asset handling and messages. To make it work in that new way the changes and compromises started to become too much for comfort. After chatting with larolwan and catch we figured the less user-disrupting way to go about it was to do the initial quick and dirty implementation. That way if something breaks, we know it comes from a mismatch between what we expect and how htmx does things, because it's the only thing we changed.

Bigpipe is not broken, if we change the data format, how the data is sent or processed we risk breaking it in ways that are not obvious. Check out the history on the big_pipe.js file, there has been many fixes after we moved to the mutation observer handling, it took many years to find the bugs and fix them. I don't want to add more work on this area when there are other things broken we could work on.

Dealing with how messages are handled is going to be easier and less stressful when the change is by itself, not blocking a big feature MR. I second @godotislate assessment the smaller the diff the more likely it is to get committed quickly.

nod_ → created an issue.

+1, see also 🌱 [Meta] Make Drupal the first "design-system native" CMS + Unify & simplify render & theme systems Active

xjm → credited nod_ → .

fix is updating to the new size of assets

There are some unusual things in the test code, needs some clean-up

Committed and pushed e1c818c662f to 11.x and 03a3cf72cc4 to 10.6.x. Thanks!

Committed and pushed 27ad23e01d2 to 11.x and 4a461032dec to 11.2.x and 076888031a9 to 11.1.x and 4f50dadbcb8 to 10.6.x and cb18f9b4fdb to 10.5.x. Thanks!

Much better thanks! the two loops and requestAnimationframes is a good call

Seems like we lost the ability to open closed details elements during search though.

For the class, it could even be a webcomponent now that I think about it. Some previous webcomponent work can be seen here: 📌 Use webcomponents for dropbutton Needs review

I don't feel strongly about the naming, so list_filter is fine with me if that's what people want.

credit

I like how much things we delete, really nice to see!

It's a good start and I have some more comments.

1. Speaking of comments there are not enough useful comments, the places where I need comments to understand what the code does, there are none.

   Declaring a function then an object member with a similar name creates 2 comments and makes the object member comment useless since we can't get the function signature (defaultSearchMethod, and listFilter.searchMethod for example) we don't know the function signature to implement if we want to override this. More on overriding below

2. In the docs inside the JS file I'd like a list of all the data attributes used and the events triggered.
3. The name is not great, we call that list_filter but the code and varibles names are all about table. This is not actually the list it's a seach input with filter an existing list. Something like search_filter_list list can't be the first thing in the name, the render element is not a list.
4. There are a bunch of extension points, but they're not used. For example the only relevant one (in core) would be to override defaultSearchMethod in system.modules.js with the regex, but it's built-in the default function and controlled by an option.

   The point here is that we don't need the extensions points "just in case", this will create BC headaches down the line. This is an internal library, not a package we publish in npm so we can make the api surface as small as possible, so let's do that. The features we want to add to the library and the API surface we want to expose in Drupal core is a problem we already ran into with the autocomplete replacement work, the time it took to figure out the different priorities between the lib and core everyone was burnt-out on it and that killed the effort, let's not replicate that this time.

   So we should only add the extension points we actually need in core. If the regex is built-in the default search method, don't make it extensible. If we want to keep it extensible, let's remove the fromWordStart option.

5. It's really old school JS :) I'd still like to separate the library code and the initialization code. Can you create a subfolder in misc and add that
6. Seems like if there are multiple instances of the filter on the page, things gets shared. Like listFilter.matchedRows, or the two filters can't have different methods overridden, if one overrides the search it would impact the other (another reason to limit extension points). I don't see how we can have different searchingFinished callbacks on the same page. Easier with an event to deal with that.
7. If you don't want to make a class, keep track of the various created instances on the page and all that, save the state in the DOM in some data attributes, for example matchedRows can be a data attribute, makes things simpler.
8. I'm not saying to do it, just asking: couldn't that be a SDC?

few comments in the MR

pdureau → credited nod_ → .

I'll close my branch that replace commands execution code with a htmx-powered code. This issue is way too late for 11.2 so let's try to do it properly for 11.3 :)

AI powered release note

That could be a good idea to only have the "modern" stuff, that would definitely help with BC headaches later on

Yes, but it's a proof of concept, not a roadmap item :)

This is probably going nowhere because our current Ajax frontend implementation is deeply tied to jQuery ajax processing, core and contrib extend it I think we need something new as to not worry about backwards compatibility

Happy to help ! Thanks for being understanding

swirt → credited nod_ → .

nod_ → created an issue.

let's go with removing debounce, the dependency needs to be removed as well.

Committed and pushed 6bdfd060b13 to 11.x and f6ae1909c44 to 11.2.x. Thanks!

I was thinking a contrib module would be a good place to do that, yes.

Is there a security track for Drupal CMS? if there is and there is already a module being used for it might be a good idea to submit a patch to implement those few lines. Or make a module and ask for inclusion in Drupal CMS. Core and CMS have different audiences so it might be more appropriate there

griffynh → credited nod_ → .

griffynh → credited nod_ → .

This could be fixed from contrib, you can change the version from a hook_js_alter:

  #[Hook('js_alter')]
  public function jsAlter(&$javascript, AttachedAssetsInterface $assets, LanguageInterface $language): void {
    foreach ($javascript as &$item) {
        if (!empty($item['version']) && $item['version'] !== -1) {
          $item['version'] = hash('xxh32', $item['version']);
        }
    }

would achieve the same and would keep people off your back :)

Thanks for all the work on the issue, after a discussion with @catch and @longwave we're not going to make this change.

We don't consider the security scanner report as relevant here. Additionally it seems it's only concerned about jQuery version specifically. We have a history of backporting some security fixes to an older jQuery version to prevent core from being exposed (so a site would use a "vulnerable" version, but with our backport it wouldn't be possible to exploit that particular vulnerability). We're usually on top of it regarding jQuery.

Closing and crediting since you've been a great contributor on this issue :)

I spent quite some time working on making just 1 conversion, I agree with #153, this is not the way to go about it. 📌 Slowly, very slowly start OOPifying the render system Needs review is a much nicer solution.

porting credit

Was able to cherry pick after backporting the other issue

backported the issue, and rebased the MR

backported to 11.2.x for 📌 Update prettier/PostCSS/stylelint for 11.2 Active

that looks reasonable

Committed and pushed 34f55547a4e to 10.6.x and 7d1886dab79 to 10.5.x. Thanks!

This changes the query parameter from v= to hv=, it shouldn't impact much things but I'd like an other review to make sure we don't have too much things depending on the query string name.

Committed and pushed 7cf46468f46 to 11.x and 103f4c829e5 to 11.2.x. Thanks!

yep, thanks!

Committed and pushed 5aa1596bdc3 to 11.x and 19df4f1774c to 11.2.x. Thanks!

somehow doesn't cherry pick to 11.2.x

small nit about the placement of a comment

I think there is a timing issue sometimes. contextual.js declares Drupal.contextual.ContextualModelView as an empty object and it get ovewritten in contextualModelView.js with the class.

Just trying to put everything in one file and see if that'll fix the problem.

nod_ → made their first commit to this issue’s fork.

It's green:)

I profiled the change and this is a terrible, terrible idea. it's 10x slower and uses 10x the memory.

Adding just in case

nod_ → made their first commit to this issue’s fork.

nod_ → created an issue.

Made things a bit clearer

Small nitpick for the selector, fix look appropriate

There are not that many failures in the daily build. This is breaking something else somewhere.

nod_ → made their first commit to this issue’s fork.

I don't understand why we need the single quote. If you add a JSON::encode(), to the process assets MR it just works.

    $build['replace'] = [
      '#type' => 'html_tag',
      '#tag' => 'button',
      '#attributes' => [
        'type' => 'button',
        'name' => 'replace',
        'data-hx-get' => $url->toString(),
        'data-hx-select' => 'div.ajax-content',
        'data-hx-target' => '[data-drupal-htmx-target]',
// add this part
        'data-hx-vals' => Json::encode(['test' => 'ok']),
      ],
      '#value' => 'Click this',
      '#attached' => [
        'library' => [
          'core/drupal.htmx',
        ],
      ],
    ];

It doesn't matter that the source html doesn't look pretty. Once it's in the JS it's the correct value.

I agree with tom konda :)

Committed 89b4900 and pushed to 11.x. Thanks!

nod_ → made their first commit to this issue’s fork.

Committed 293fe02 and pushed to 11.x. Thanks!

Committed and pushed a6ad55b2765 to 11.x and 110c8468c25 to 11.2.x. Thanks!

This is a feature, not a bug.

Check out the 3 lines of comments just before the code change:

// Preview the machine name in realtime when the human-readable name
// changes, but only if there is no machine name yet; i.e., only upon
// initial creation, not when editing.

You want to be able to change the label without changing the machine name. Happens all the time that some vocabulary is renamed to be clearer to end-users but you want to keep the machine name to not mess with config export and custom code.

Thanks, there are comments about IE6, any plan to get rid of that too? :)

Committed 24dcc8d and pushed to 11.x. Thanks!

g4mbini → credited nod_ → .

Don't mind me, just fixing the version.

I like the idea. Hopefully we don't have too many forms where content and configs is mixed up.

Might need UX folks for how this should look like and behave

Voting for:
- Marine Gandy (Mupsi)
- Joris Vercammen (borisson_)

umm but then we get duplication, I think it's fine, details is not really a heavily customized template

We should be able to do that from twig only, no php changes needed to form.inc

FrankenPhp is going to the PHP GitHub org:
https://externals.io/message/127347 with some of the doc moving to PHP.net

This one can still go in 11.2, if we don't have this one, the htmx lib is useless which is unfortunate

added tests for CSS files and JS behaviors

Fixed a bug where ajaxpagestate was not added to the page when using only htmx, when core/drupal.htmx library is detected on the page we add all the ajax-related infos, otherwise we end up with JS/CSS files being loaded twice.

Should be good to go but since I added a bit of logic for drupalSettings stuff, putting as NR

I think that's what finnsky had in mind too

@supriyagupta: I don't understand why you created a fork and the content of your comment, could you clarify please?

@alex.skrypnyk: When I opened the ticket I did not understand you meant exemples yet, so the retitle is inline with what I had in mind, which are the use cases explained by Pierre above.

We need to add ajaxpagestate on all pages that have drupal/htmx library otherwise things will break in various use cases. working on it and adding tests for adding css file, js module, attach and detach behaviors

There is suggestion in the MR

Committed 7dc9bf7 and pushed to 11.x. Thanks!

Committed ab9823e and pushed to 11.x. Thanks!

small feedback, unnecessary nesting. You can RTBC once it's fixed

Committed 5e22a55 and pushed to 11.x. Thanks!

nod_ → made their first commit to this issue’s fork.

Committed and pushed 2f17d640524 to 11.x and dd94411ba01 to 11.1.x and 21bc1da3e70 to 10.5.x. Thanks!

Committed d381523 and pushed to 11.x. Thanks!

Tried to simplify some things.

For the credential forms we don't need to change too much logic, we need to reverse some conditions and it works as expected.

+1 for the two landmarks

We need a change record with code examples to show when the condition in #38 is likely to occur and code sample to show how to fix it.

updated the code, can you check it solves the problem?

that's a good idea, follow-up though

The lock file is correct, latest version is 2.0.4: https://www.npmjs.com/package/htmx.org?activeTab=versions

The debug extension is in the package because of how they used to do things: https://github.com/bigskysoftware/htmx/blob/master/dist/ext/README.md

let's remove the debug extension, it's not used yet and there is a built-in way of doing the same thing (htmx.logAll())

as for the folder that's my bad, I removed the "folder" key and we needed it