Problem/Motivation

Recently added code does not account for the possibility that the target passed to htmx does not exist.

Steps to reproduce

Use a CSS selector for a target that does not resolve to an element in the DOM. When this is a regular swap the fetch is never sent due to a failure is our header configuration code. When it is an incoming hx-partial the swap itself fails in our behavior processing.

Proposed resolution

Add an additional condition to the guarding conditional.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3622958

Command icon 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:

About issue forks

Comments

fathershawn created an issue. See original summary.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Issue tags: +HTMX initiative
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Active » Needs work

Working on the tests

fathershawn opened merge request !17122

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Issue summary: View changes
Status: Needs work » Needs review

1 unrelated test failing - which has failed 5 times in the last 14 days

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Needs work

Haven't gone over everything in detail yet, but I do have some comments on the MR.

ironnuts’s picture

ironnuts commented

I agree with godotislate that by using Route attributes no need for the entries in routing yml.

Symfony has a relevant example (click on Attributes tab) https://symfony.com/doc/current/routing.html#parameters-validation

    #[Route('/blog/{slug}', name: 'blog_show')]
    public function show(string $slug): Response
    {
        // $slug will equal the dynamic part of the URL
        // e.g. at /blog/yay-routing, then $slug='yay-routing'

        // ...
    }

That is a simple example. So would look like:

    #[Route('/test-htmx/{target}/{swap}', name: 'build_partial')]
    public function buildPartial(string $target, string $swap)  
    // ...
ironnuts’s picture

ironnuts commented

Is that what you had in mind, godotislate?

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Needs work » Needs review

Feedback addressed. If comments can be resolved it would be super to get this bug fix in!

ironnuts’s picture

ironnuts commented
Status: Needs review » Reviewed & tested by the community

fathershawn I have reviewed the code and noted that the comments have been resolved. I have added a new comment suggesting consolidation of the 3 routes into 1, but it a suggestion and does not need resolving. Maintainer can consider my suggestion if they want to.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

Ran the test only job, and only the new testMissingSwapTarget fails. testMissingPartialTarget, ttestPartialTarget, and testPartialDelete are all passing.

I pulled the test code down locally (without the fix) and manually recreated the test steps on the test form in browser. Checking either "Target does not exist" box ( bad_swap_target and partial_missing) in separate requests does generate console errors, but checking either "Replace via partial" or "Delete via partial" does not generate any console errors. I'm not sure whether this is expected for the latter two cases.

But for testMissingPartialTarget, presumably that test should fail since I do see errors in the console manually, so I'm not sure why the automated test isn't failing. Setting to NW for that.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Needs work » Needs review

testMissingPartialTarget does not fail in test only due to an interaction between htmx and our testing code.

Htmx traps the error

} catch (error) {
  ctx.status = "error: " + error;
  this.#trigger(elt, "htmx:error", {ctx, error})
}

And so we see in console:
htmx: htmx:error: null is not an object (evaluating 'element.parentElement')

Our code traps the error event:

  window.addEventListener('error', (evt) => {
    const errors = JSON.parse(
      sessionStorage.getItem('js_testing_log_test.errors') ||
        JSON.stringify([]),
    );
    errors.push(`${window.location.pathname}|${evt.error.stack}`);
    sessionStorage.setItem(
      'js_testing_log_test.errors',
      JSON.stringify(errors),
    );
  });

Because the htmx has trapped the error and triggered a custom event, our test code does not react and the test passes without the fix in place.

Revised the test to send two partials: the first with a bad target which stops execution.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Unrelated Drupal.Tests.workspaces.FunctionalJavascript.WorkspacesMediaLibraryIntegrationTest failed. Has failed 3 times in the last 14 days.

godotislate’s picture

godotislate
he/him
commented

Changes look good. Test only fails for testMissingPartialTarget as well.

@fathershawn, can you confirm that testPartialTarget, and testPartialDelete are expected to pass test only? If so, feel free to self RTBC.

One note is that I think it might be worth looking into whether the test methods could be consolidated, so that bootstrapping doesn't have to happen 4 times, but I think that can be handled in a follow up, since this critical and the first 12.0 beta is already out.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Needs review » Reviewed & tested by the community

Confirmed!

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Issue tags: +12.0.0 release priority
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Version: main » 12.0.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: -12.0.0 release priority
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Needs review » Reviewed & tested by the community
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Issue tags: +12.0.0 release priority
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Version: 12.0.x-dev » main

  • longwave committed 2ae3db67 on 12.0.x 
    fix: #3622958 Target can be null in htmx-assets.js

By: fathershawn
By:...

  • longwave committed 0af7577c on main 
    fix: #3622958 Target can be null in htmx-assets.js

By: fathershawn
By:...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 12.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 0af7577cc6b to main and 2ae3db67a89 to 12.0.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.