Problem/Motivation

It's not super friendly to use the drupal_htmx wrapper format, add a few things to help DX with the Htmx object.
The way to get the URL of the current page is to use Url::fromRoute('<current>'), Then you need to remember that it doesn't give you the query parameters of the current url. If you're working with a GET request, your form state is gone. So you also need to add \Drupal::request()->query->all(); but that comes with extra values like form_build_id, form_token, op that you don't want in your URL since the values are coming from your actual form.

Steps to reproduce

Proposed resolution

Automatically add the wrapper format parameters to Urls when using Htmx->get/post/patch/delete, leave an option to opt-out and request a full page with all the regions and blocks surrounding the content if necessary.

Automatically figure out the current url and remove unwanted query string parameters from other form submissions and history state management.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#4 3546105-nr-bot_8apakq9t.txt3.86 KBneeds-review-queue-bot

Issue fork drupal-3546105

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

nod_ created an issue. See original summary.

nod_ opened merge request !13202

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Active » Needs review

depends on #3522597: Return only main content for selected htmx requests

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3546105-nr-bot_8apakq9t.txt3.86 KB

The 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.

fathershawn made their first commit to this issue’s fork.

fathershawn changed the visibility of the branch 11.x to hidden.

fathershawn opened merge request !13205

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

I put up MR!13205. I discussed this idea with @nod_ in Slack. The static method makes creating or modifying the Url object relatively simple and the changes are fewer and easier to test.

Here's an example code block:

$url = Htmx::mainContentOnly(
   Url::fromRoute('test_htmx.attachments.replace', [], $options)
);

$htmx = new Htmx();
$htmx->get($url);
nod_’s picture

nod_
Primary language French
Location Lille
commented

I'm not happy with that static method being the only way to add the wrapper format.

We can add the static method but I want to add the htmx wrapper format by default to the Urls the Htmx class handles, and provide a way to opt-out when necessary. The wrapper format is an implementation detail of Drupal, people using Htmx in Drupal shouldn't have to know about this.

Making a test harder to write, and in exchange we won't have to explain anything about wrapper formats to users is a good compromise to me.

fathershawn’s picture

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

Made a couple of suggestions. I'll check back later to see if the unit test is running.

fathershawn opened merge request !13218

fathershawn closed merge request !13218

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Opened an MR with a working kernel test to your issue branch @nod_

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Gitlab CI doesn't like MRs between issue branches: 

  Invalid version string "3546105-improve-url-management-dev"

but the kernel test passes locally.

fathershawn closed merge request !13205

fathershawn changed the visibility of the branch 3546105-static-method to hidden.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Refactored the test to use a data provider.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

That looks good, thanks!

fathershawn’s picture

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

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

I have a concern with current url and query parameters.

If the form is using get, won’t the url for a new request contain the prior request parameters?

nicxvan’s picture

nicxvan commented

Re 14 yes tests won't run in MR between branches without a lot of work.

It's still helpful sometimes to see the diff!

nod_’s picture

nod_
Primary language French
Location Lille
commented

@fathershwan: it sure will, that's the point and that's why we filter out the Drupal specific pieces out of the get parameters. We need that for TranslateFilterForm at least.

If we have another method in the Url object or somewhere else that does this I'd be happy to remove this chunk of code from the Htmx object, but I don't have a good place for it somewhere else

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

@nod_ My concern is about a conflict with the state stored in the form and the state stored in the request URL. Consider a form with an input A that responds to user interaction and sends the values in a get request.

Focusing only on values for clarity

  1. User sets A to 1
  2. The input regenerates, containing data-hx-get="/path?A=1

Now if I set the value to 2 I am explicitly declaring that I will send 1. Isn't that confusing? It seems like the state encoded in the form should be the source of truth.

nod_’s picture

nod_
Primary language French
Location Lille
commented

should be good to go, maybe comments needs some work? not good at that.

fathershawn’s picture

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

This looks good to go to me.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

I found myself wondering tonight if we should be building a clean url using URL::fromRouteMatch if we are going to build them for people as a convenience?

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

started to go another way in #3538544: Ajaxify the user interface translation forms

Instead of handling that in the backend, the query string is added from the front and we have an extra drupal-specific attribute that controls it

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Related issues: +#3549286: Htmx reusing existing URLs, avoid duplicating url handling
nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes
nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: Improve url management of the Htmx object » Automatically add _wrapper_format for HTMX requests
Status: Needs work » Needs review

Back to green

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

I know that predicting the future is hard. I'm wondering if #3544632: Return only main content for endpoints designed to service htmx requests lands how many dedicated routes we will build. Will most requests use such dedicated routes? If we think that is likely, should we reverse the polarity of the data attribute added here and make the default to be no wrapper format parameter?

nod_’s picture

nod_
Primary language French
Location Lille
commented

My main objective with htmx and forms is to make sure htmx and regular requests follow the same code path, unlike what's happening with Ajax today. I'm thinking most uses of htmx will be in the context of a form (for now at least) where it makes sense to add the wrapping format by default. Even if we query an htmx route, adding the wrapper format parameter shouldn't be a problem.

All that to say I think we should keep the default as adding the wrapper format.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: Automatically add _wrapper_format for HTMX requests » Automatically manage _wrapper_format for HTMX requests
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Issue tags: +Needs change record

I think this needs a CR. Adding the tag - remove if I'm off base!

godotislate’s picture

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

Some comments on the MR. For the comments on the tests, I realized after making my suggestions that the test code was largely lifted from the other existing test class. A lot of suggestions are nits or style, though I think the missing commas should at least be fixed.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review

thanks, updated.

godotislate’s picture

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

PHPCS issues in the build.

godotislate’s picture

godotislate
he/him
commented
Issue tags: -Needs change record

Added a CR from what I understand of the issue: https://www.drupal.org/node/3558614. Please review. Otherwise I think this is good for RTBC.

godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Needs review
papagrande’s picture

papagrande
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

I reviewed the CR and while my HTMX experience is still low, the explanation itself is clear to me.

fathershawn’s picture

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

The CR should also describe the effect of the data attribute on the wrapper format.

fathershawn’s picture

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

I added the detail I think is needed to the CR and ready for another review.

godotislate’s picture

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

Nice, CR lgtm.

mxh’s picture

mxh commented

I have tried out the MR within the scope of #3560641: HTMX hx-boost does not work with core/drupal.htmx library and it turns out this has one good change and one bad change when using hx-boost on the <body> element:

  • The good change: When clicking on a link, the URL history now only shows the URL path of the link without the ajax_page_state query arguments. Before the URL history has been flooded with these query arguments.
  • The bad change: Only main page content is being returned from the response and thus only main page content is rendered on the whole page. Before the response returned everything. Using hx-boost, everything is needed, not just the main page content.

Not setting to NW as I'm not into the details of this issue, but at least posting what I found out from my scope.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Once again should have listened to @fathershawn earlier 😂 in #31.

Reversing the attribute default. This should behave more as expected.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

If this works out in for @mxh in their testing, then I'll adjust the CR

nod_’s picture

nod_
Primary language French
Location Lille
commented

For default I went with only main response when using ->get/post/patch/etc. and the full page when using ->boost() not sure it's a great idea to have different default but that seemed reasonable.

nod_’s picture

nod_
Primary language French
Location Lille
commented

changed my mind, keeping the full page by defaut on all requests, to use the special drupal output you need to opt-in, hopefully that will prevent folks from being tripped up by this and we can just use that in core when we need it.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: Automatically manage _wrapper_format for HTMX requests » Automatically manage _wrapper_format for HTMX requests and clean up ajax_page_state in urls
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Reviewed recent changes and I affirm this is still RTBC.

Thanks for the refactor @nod_ !!

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Nice re-write on the CR. I changed the title to HTMX requests may be configured to use the drupal_htmx wrapper format.

mxh’s picture

mxh commented

I can confirm the recent changes make usage of hx-boost more straightforward. Now, without specifying anything else, the whole response is returned as expected. Network logs show the attached ajax_page_state query arguments, whereas the URL history contains the link's href only (without ajax_page_state). This is what I consider to be expected. It's nice to see the attachments being loaded only once, which the main reason why I want to use hx-boost. Great to see this working, thank you!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Wondering why it needs to be specified in the factory method when it could just be a separate method in the chain? Boolean arguments are harder to reason about and less readable (though named arguments helps here). As this feature is opt-in and disabled by default it makes more sense to me to move the opt-in to its own explicit method call, ie.

    (new Htmx())
      ->post($form_url, onlyMainContent: TRUE)
      ->select('[data-export-wrapper]')
      ->target('[data-export-wrapper]')

could be

    (new Htmx())
      ->post($form_url)
      ->onlyMainContent()
      ->select('[data-export-wrapper]')
      ->target('[data-export-wrapper]')

?

idiaz.roncero’s picture

idiaz.roncero
he/him
Primary language Spanish
Location Madrid
commented
Title: Automatically manage _wrapper_format for HTMX requests and clean up ajax_page_state in urls » Automatically manage _wrapper_format for HTMX requests

I came here from #3560641: HTMX hx-boost does not work with core/drupal.htmx library: in my case, I wasn't using hx-boost but a more specific usage of data-hx-attributes on some links to reload only certain parts of the page while keeping brower history up to date.

<a href="/" data-hx-get="/" data-hx-target="#htmx-main-content" data-hx-selector="#htmx-main-content" data-hx-swap="outerHTML ignoreTitle:true" data-hx-push-url="true">
            Inicio
</a>

Kind of a "hx-boost" manually constructed.

I had the same problem with the ajax_page_state being pushed to they query URL parameters, and with the patch applied I can confirm that they are gone.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: Automatically manage _wrapper_format for HTMX requests » Automatically manage _wrapper_format for HTMX requests and clean up ajax_page_state in urls
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

The revisions requested by @longwave look good. This really improved the feature. All tests passing. Still RTBC

longwave’s picture

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

Thanks for the last minute change. This is going to be super helpful so good to get it into 11.3.x.

Committed and pushed ca4031b9136 to 11.x and 62362fa40d1 to 11.3.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.

longwave closed merge request !13202

  • longwave committed 62362fa4 on 11.3.x 
    task: #3546105 Automatically manage _wrapper_format for HTMX requests...

  • longwave committed ca4031b9 on 11.x 
    task: #3546105 Automatically manage _wrapper_format for HTMX requests...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.