Problem/Motivation

See #956186-189: Allow AJAX to use GET requests.

'method' has been in use for a long time. The original patch used 'type', but maybe we could change that to 'httpMethod'. If we make the change before 10.1.x, it won't be an API change.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 interdiff.txt594 byteslauriii
#14 3352984-14.patch2.95 KBlauriii
#12 interdiff.txt903 byteslauriii
#12 3352984-12.patch2.86 KBlauriii
#3 3352984.patch2.55 KBcatch

Comments

catch created an issue. See original summary.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

+1. Way more intuitive.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 3352984.patch2.55 KB

Untested patch. Couple of things I noticed:

- type was probably originally chosen because jQuery.ajax used 'type' for GET/POST.

- Since jQuery 1.9, it allows method, which type is a direct alias/bc for, although ajax.js relies on 'type' still, likely just because it's never been updated.

- as lauriii points out in the other issue, we can't use 'method' because we already use that for replacements.

So... trying httpMethod, I changed the one reference to jQuery AJAX 'type' to 'method' too here since that makes things slightly more consistent.

Passed through my mind that we should consider renaming 'method' to 'replaceMethod' in the AJAX API too, different issue if we wanted to do that though.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Throwing my hat into the bikeshed: these are commonly referred to as HTTP verbs, so if we can't use method what about verb?

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

> so if we can't use method what about verb?
Good point. I would've had to scratch my head what this means though. So while technically more correct, maybe not so intuitive for some.

Also, i'd prefer httpMethod over method (and verb), because it includes the context, so its meaning is not dependent on additional context.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

+1 for httpMethod over verb. I appreciate the verbosity

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +DX (Developer Experience)

“HTTP method” is more explicit than “method” or “verb” to my ears too.

Plus, it’s consistent with how Symfony refers to it in its route definitions.

smustgrave’s picture

smustgrave commented

If I got a vote I like httpMethod.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented 
+++ b/core/misc/ajax.js
@@ -325,12 +325,12 @@
+      const httpMethod = $linkElement.data('ajax-httpMethod');

As a data attribute wouldn't this become ajax-http-method? Data attributes usually use kebab-case instead of camelCase; automatic conversion is also done in dataset so this could also be referred to as $linkElement.dataset.ajaxHttpMethod.

I also don't see any test coverage for this :(

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Just to keep it moving, sending to NW for test coverage.

catch’s picture

catch
he/him
Primary language English
commented

I don't think we should add test coverage for a direct rename, there is a lot of implicit coverage of the AJAX API via views but not much explicit. Leaving needs work for #9 though.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
StatusFileSize
new 3352984-12.patch2.86 KB
new interdiff.txt903 bytes

Addressed the feedback and updated the comments.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work
Issue tags: +Needs followup 
+++ b/core/misc/ajax.js
@@ -325,12 +325,12 @@
        * In case of setting custom ajax type for link we rewrite ajax.type.

I think this comment needs updating a bit given one part of it is no longer called "type". This comment didn't really add much anyway, not sure it is needed, although it is perhaps the only documentation for this feature.

I am still concerned there is no test coverage for data-ajax-type / data-ajax-http-method but I suppose that shouldn't hold this up otherwise we have to do a deprecation dance and so that can be deferred to a followup.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
StatusFileSize
new 3352984-14.patch2.95 KB
new interdiff.txt594 bytes

This should address #13.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup +Needs Review Queue Initiative
Related issues: +#3356611: Add test coverage for data-ajax-http-method attribute

Opened #3356611: Add test coverage for data-ajax-http-method attribute as a follow up.

  • longwave committed cb778841 on 10.1.x 
    Issue #3352984 by lauriii, catch, longwave, smustgrave, geek-merlin,...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committing this now so it can get in before the alpha release.

Committed and pushed cb77884135 to 10.1.x. Thanks!

Will update the change record to mention the new property name.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Updated https://www.drupal.org/node/3193798 and linked it to this issue.

Status: Fixed » Closed (fixed)

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