Fix json-enc DELETE implementation

[alexpetros]

This PR breaks this test: https://github.com/bigskysoftware/htmx/blob/master/test/ext/json-enc.js#L112

I'm not exactly sure what the best way to handle this is—I think it's probably to have the json-enc extension override this PR's change and send its payload in the body (since that's almost certainly what the user expects). That probably requires an additional core change though.

Originally posted by @alexpetros in https://github.com/bigskysoftware/htmx/issues/1513#issuecomment-1605837195

[Telroshan]

Probably a naive question, but should've the test been there in the first place? I see there is no "GET" version of those tests for the json-enc extension, adding one quickly also raises the same error. Which isn't surprising, because the RFC for the GET method says the same thing as for the DELETE method ( see #1513 )

A payload within a GET request message has no defined semantics; sending a payload body on a GET request might cause some existing implementations to reject the request.

So if you're going to add support for JSON body anyway for DELETE, you might as well want to add it for GET (which also doesn't seem to work atm)

Or am I missing something there?

[alexpetros]

No it's a fair question! That's why I made the issue for it.

My take on this is that while it's generally considered terrible practice to add payload bodies to GET requests, I don't know if the same cultural norm applies to DELETE and I expect that people making use of the json-enc extension will expect that to work.

As you correctly point out, the semantics of both are undefined, so there's no reason to allow DELETE bodies but not GET bodies. Since the json-enc extension is purely additive (and applied selectively), I would cautiously be in favor of also adding it to GET, since, if you want to define that way for your application, why not?

[Telroshan]

Yeah I would be in favor of adding to both of them in that case, for the sake of consistency and homogeneity

[andryyy]

I never really thought about that. But I seem to misuse DELETE with data bodies. I did it recently when I wanted to drop a bunch of ids from a db while not looping every record.

[alexpetros]

@1cg are you fine with me opening up GET and DELETE requests to allow for bodies, if there is an extension present on the element with encodeParameters defined?

[1cg]

yeah, seems like we should make it possible to override including things in the body via the htmx:configRequest event to me

[1cg]

hey @alexpetros thinking about this more, I think we should keep the existing DELETE functionality by default. I would hate to break someone depending on the current behavior, and the bulk delete case seems like a situation where we could get in trouble (URL parameters aren't handled the same as body parameters in every server side framework, you can only encode so much data in a URL, etc.)

I think what would be idea is to make things configurable so that you can have DELETE use URL prams if you like (and have GET use a body, if you like), probably via an event.

Does that sound reasonable?

[Telroshan]

While I understand the retro-compatibility issue, I would still argue that not following the RFC looks more like a bug than a feature to me, and a "bugged state" shouldn't be the default behaviour but an optional one, letting the user move away from the RFC recommandations if they want to

[1cg]

i agree, but I'd also call this a breaking change.

maybe we can flip the behavior (and document it) for 2.0?

[Telroshan]

i agree, but I'd also call this a breaking change.

A breaking change indeed, but of what is by definition already a broken state.

The problem I see with that approach is that, currently for a golang backend (maybe for other languages as well?), a hx-delete simply does not work at all, by default. It's not that it doesn't send parameters the way that's more convenient to you and you can do otherway on your backend or whatever, you simply do not get any parameter at all on your backend. As a dev discovering htmx, this would clearly look like a bug (and really, it is). Assuming that dev would then check the docs, they would see that "it's not working because the lib is deliberately not following the RFC specification by default, so I have to add this extra config / event / other thing, to be compliant and make it work"

Whereas, breaking (though I really see it as "fixing") the current default state, will have the htmx devs that were also not following the RFC specification, suddenly get their params in the URL instead of the body, but at least get something, something to work with. (Also if you'll allow me some nitpicking, some backends handle gracefully parameters and check for both URL encoded params and body params, so in some languages you wouldn't actually have to change anything on your backend). From there, they could either change their backend parameters parsing to follow the RFC or add the extra htmx config to keep a behaviour that was not supposed to be the default one by definition.

I'll concede that I'm exaggerating a bit here, but this gives me some vibes of that xkcd comic

[alexpetros]

a "bugged state" shouldn't be the default behaviour but an optional one

You would love the discussion we're having about SQLite Foreign Keys in the discord.

"it's not working because the lib is deliberately not following the RFC specification by default, so I have to add this extra config / event / other thing, to be compliant and make it work"

If we're going to reference the RFC, the RFC says that the behavior is undefined. So it does seem like a framework problem that golang errors out on the presence of a body, rather than gracefully ignoring the thing that would be undefined. I'm not minimizing the frustration of it (very fair) but we are implementing additional behavior on top of HTML, so there really isn't a "default" way to handle DELETE requests from a form because it's (absurdly) impossible to make DELETE requests from a form—one of the problems that htmx seeks to solve.

Backwards compatibility comes with tradeoffs. I do think it's possible to minimize the impact of this default with a cleverly chosen config, but don't really have strong feelings about it one way or another. I might also consider for changing the default behavior here in htmx2, assuming the changelog is small enough that we can really emphasize it (EDIT: 1cg suggested this too, cool). Backwards compatibility is really important to the mission of this particular software though, IMO.

I think what would be idea is to make things configurable so that you can have DELETE use URL prams if you like (and have GET use a body, if you like), probably via an event.

@1cg It's perfectly reasonable. What that would mean for my existing PR is that I just remove DELETE from the default and PR a separate config mechanism at a later date. You let me know if that's the approach to take.

[Telroshan]

It's not only about forms, it applies to every request, including those made with htmx.ajax, it doesn't only apply to something that doesn't exist. Besides, it still resolves to simple HTTP request, I'd argue that it doesn't have anything to do with being from a form at all. Forms send params in URL for GET, and in body for POST, as per the RFC. I would agree with you if forms also were sending params in body for GET but they aren't, they're simply following the RFC about HTTP requests payloads

Undefined, exactly, unlike URL params which is a defined behaviour, thus standard

[Telroshan]

To expand on my previous point, see this JSFiddle It features 2 calls to htmx.ajax, one using the GET method, one using DELETE. See in the inspector how the DELETE request sends parameters in the body, not as URL encoded parameters.

This isn't just about forms. It's about HTTP requests in general. Whether they come from a form, a link, any HTML element that htmx gives the ability to send requests to, or from a very JS script, a manual ajax call that htmx itselfs resolves to XMLHttpRequest. As htmx itselfs states on its homepage

Why should only <a> and <form> be able to make HTTP requests?

This is about HTTP requests, not forms. As such, I suggest we should follow the HTTP requests RFC, and use the defined behaviours instead of undefined ones.

Sorry if I look stubborn about this topic, but I'm really concerned about the decision here

[alexpetros]

It is true that query parameters are a defined part of URI scheme and DELETE body payloads are not. It's also true that sending DELETE payloads should not break your backend server framework just because its semantics are undefined. I'm not passing the buck, I basically agree with you, just pointing out that framing this as a "bug" when HTTP implementations can and do define DELETE payloas (as @andryyy pointed out) is not correct either. At present, htmx is one of those implementations.

Because this absolutely would be a breaking change for every htmx user that makes use of hx-delete on form elements, I don't think it should be be a minor version change. I know this is frustrating because you already merged the PR, but my recommendation would be to revert to the present behavior and PR a config that lets you set the form to use URL params instead of body params. I'd also support potentially switching the default for htmx 2 with a clear migration path, which I think is reasonable.

@1cg's decision ultimately.

[andryyy]

Sounds fine to me to change that in 2.0.

Even more when it is not disallowed by RFC but "just not defined".

How about introducing deprecation warnings?

[Telroshan]

I had missed it but the RFC has been updated as of June 2022 In RFC 9110: First, just a change in the formulation so that the next quote will be meaningful:

The terms "payload" and "payload body" have been replaced with "content", to better align with its usage elsewhere (e.g., in field names) and to avoid confusion with frame payloads in HTTP/2 and HTTP/3. (Section 6.4)

Now, in the DELETE method section (and also in the GET one), the RFC says the following:

Although request message framing is independent of the method used, content received in a DELETE request has no generally defined semantics, cannot alter the meaning or target of the request, and might lead some implementations to reject the request and close the connection because of its potential as a request smuggling attack (Section 11.2 of [HTTP/1.1]). A client SHOULD NOT generate content in a DELETE request unless it is made directly to an origin server that has previously indicated, in or out of band, that such a request has a purpose and will be adequately supported. An origin server SHOULD NOT rely on private agreements to receive content, since participants in HTTP communication are often unaware of intermediaries along the request chain.

As htmx is a generic client lib and can't make assumptions on the backend as it could be anything,

A client SHOULD NOT generate content in a DELETE request unless it is made directly to an origin server that has previously indicated, in or out of band, that such a request has a purpose and will be adequately supported.

This applies to htmx here in my opinion, which according to the RFC SHOULD NOT send a body. While I agree that it's a SHOULD NOT and not a MUST NOT, it has now made been clearer in the RFC that it is at least a discouraged behaviour, not just "undefined"

Again, I understand the breaking change / retro-compatibility part, but here it seems to me like it's a choice for retro-compatibility and past htmx users that would be using hx-delete and depend on that body, to the detriment of future htmx users that are not using it yet. Having a default behaviour that simply doesn't work on every backend seems worse to me than introducing a breaking change on a discouraged workflow (let me refer the comic above again)

[alexpetros]

it's a choice for retro-compatibility and past htmx users that would be using hx-delete and depend on that body, to the detriment of future htmx users that are not using it yet.

Correct, but a little more precisely: either make a breaking change for existing users, or introduce a config for new users whose backend frameworks break on DELETE body payloads. Hopefully you understand why the latter is a valid choice for a library. I don't disagree with your interpretation of the RFCs, but htmx absolutely has users who rely on DELETE body payloads, and that behavior is a couple steps more reasonable and widespread than emacs spacebar heating.

[Telroshan]

Honestly I would agree on that choice with you, if the behaviour to keep retro compatible, was an intended/expected behaviour, not a behavior that SHOULD NOT (quoting the RFC) be the default one. I think it really comes down to:

• I see this as a literal "bug", while you see this more as an inconvenience, if that makes sense ?
• I understand we agree about the RFC and how things should be for htmx 2, we just don't agree on the resolution of that issue in htmx 1
• You would make it the new default for htmx 2 since it's going to have breaking changes anyway, but keep the current behaviour to be retro-compatible, while I would advocate for making it the default for both versions for consistency, to the detriment of old users that would have to either not upgrade htmx, or refactor their code

So, I guess, let's agree to disagree

Just to make sure btw, no hard feelings there! And as you said, it's 1cg's decision ultimately anyway

[andryyy]

Would be cool for v2 by default, because people will have to expect breaking changes. Doing it in v1 just before v2 is released doesn’t sound good for me.

[alexpetros]

Plan from @1cg, per discord:

• Make it possible to use query parameters in deletes via two mechanisms: expose this as an option in the htmx:configRequest event and also introduce a config option. Question: is a body acceptable in all the other request types? So crazy that they would pick DELETE as well as GET for this...

• In htmx 2.0, flip DELETE to not use the body by default and then document the heck out of it as part of the upgrade step

I updated #1525 so that DELETE bodies will not use URL params, and I'll make a follow-up issue to address these two proposed solutions and link it to this one.

[1cg]

resolved by https://github.com/bigskysoftware/htmx/issues/1539 thank u @alexpetros!