Problem summary
AJAX requests are always made using POST, which disables render caching.
However most AJAX requests could be cached safely, and the POST requirement has been due to the large amount of information sent with the request - mainly ajax_page_state containing css/js and html IDs.
ajax_page_state now contains library names instead of individual files, and html IDs are no longer sent at all, resulting in a much shorter URL if requests are switched to GET.
We may be able to reduce the size of the GET request further by compressing ajax_page_state, see #3303067: Compress aggregate URL query strings and #3279206: Dynamically determine ajaxPageState based on libraries.
Proposed solution
Allow AJAX requests to use GET. Continue to default to POST and make this opt-in for backwards compatibility. Forms will continue to have to use POST because they're forms.
Convert Views AJAX (pagers etc.) to use GET - since this is the primary non-form AJAX use-case in core and validates that everything works.
Follow-ups
Comment | File | Size | Author |
---|---|---|---|
#188 | 956186-188-10.1.x.patch | 16.45 KB | lauriii |
| |||
#186 | interdiff_184-185.txt | 2.6 KB | anairamzap |
#186 | 956186-174-reroll-95-185.patch | 18.06 KB | anairamzap |
#177 | 956186-174-reroll.patch | 16.36 KB | lauriii |
| |||
#174 | 956186-174.patch | 16.37 KB | sker101 |
|
Comments
Comment #1
yhahn CreditAttribution: yhahn commentedComment #2
effulgentsia CreditAttribution: effulgentsia commentedThanks for the work on this. Subscribing, and intend to review next week.
Note that while #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework introduced the ajax_page_state variables, it was #384992: drupal_html_id does not work correctly in AJAX context with multiple forms on page that originally introduced the ajax_html_ids variables, and that alone prevents using a GET request for AJAX. Also note that ajax.js never supported GET requests out of the box. But all that aside, I agree there's value to reducing the payload both for performance, and to enable contrib to change some AJAX requests from POST to GET if it wants to.
Comment #4
alex_b CreditAttribution: alex_b commentedSub - without HTTP GET AJAX no reverse proxy caching for ajax pagers.
Comment #5
rfayThanks for the good work on this. Subscribing.
Comment #6
yhahn CreditAttribution: yhahn commentedUpdated patch fixes HTML ID test.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedSorry I haven't had time to review #6 yet. I would still like to at some point, but RC1 is approaching fast, and I don't know if I'll be able to give this the attention it deserves before then.
My gut feeling is that #6 is too much for D7 core at this late hour. I'm open to being overruled on this though, so I'm leaving it at "needs review".
But, I do think it is critical that a contrib solution be possible. I suspect it already is, between hook_ajax_render_alter(), the ability for JS functions to be overridden, drupal_html_id() using a drupal_static() that can be hacked into from another function, and PHP allowing $_POST to be written to even if it's a GET request. But, it would be great if people familiar with the inner workings of the ajax system could double check the internals to verify that nothing in the system is so hard-coded as to make such a contrib module impossible. @yhahn: if you have time to try your approach out as a contrib module, that would be awesome.
Once we're confident that such a contrib module is possible without hacking core, I think ctools would be the right home for the implementation, and for this issue to move to.
The reason I think this is critical is that the web paradigm is shifting from page centricity to application like UIs, and if Drupal doesn't provide a way for high performance AJAX, then cutting edge developers will look to other frameworks. And that would be a shame, since we'd rather that Drupal be the platform that attracts and retains cutting edge developers.
Comment #8
mikeytown2 CreditAttribution: mikeytown2 commentedSome ideas I've had in terms of keeping track of what is inside the aggregated css/js files
http://groups.drupal.org/node/53973#comment-279159
Comment #9
JeremyFrench CreditAttribution: JeremyFrench commentedI like this feature, however I am not sure if it qualifies as a critical as it may be able to go in a post 7.0 release of core.
As I understand this bug is for using the Drupal API to make ajax requests, this provides a very tight integration with Drupal. However using it outside of the FAPI is not straightforward (or well documented).
I don't think this bug would stop somone from making standard ajax requests to a custom callack without the overhead. What may be needed (and can be done in contrib), is a fast_ajax delivery method, for times when the drupal ajax stack is not needed for a request.
I don’t claim to be an expert of the Drupal Ajax innards, but have had some experience of trying to use them.
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedI think you're right. I'm just not 100% positive.
To re-iterate from #7:
...
In other words, my inclination is to move this issue to the ctools queue, and I'll probably do so in a few days barring further comments. I'm leaving it in the core critical queue for now, just to make sure people have a chance to flag any simple core API change that may be required if my assumption above is wrong.
Comment #11
yhahn CreditAttribution: yhahn commentedThis doesn't need to be a D7 critical as it's a change that can go in with a point release (e.g. 7.1).
That said, I don't think it makes sense to move into contrib as it touches a very important assumption in
drupal_html_id()
and there is really no way to get the behavior of this function (which is called widely throughout core, Form API, etc.) to change from a contrib module.Comment #12
sunI understand and basically agree with the outlined problem. I especially agree with the idea that the AJAX framework currently prevents to implement true REST callbacks, and also kills proxy/reverse-proxy caching, since every request is a HTTP POST.
However, both solution proposals make the system more fragile:
We still have lots of theme functions and templates that output arbitrary IDs, sadly. Also, third-party/non-Drupal scripts may inject further HTML IDs that are unknown to Drupal. Lastly, even though it's less likely, a random hash can lead to duplicate IDs.
Right now, the entire system has a single defined state only. The actual client/browser holds and defines that state. Drupal only reacts accordingly to that state, which means that we purposively do not try to synchronize the state information between client and server. Exactly that intentionally missing synchronization is what makes this part of the current AJAX framework rock solid.
When discussing this problem space, then we also need to take into account that the AJAX framework doesn't provide means to remove loaded files currently. While I couldn't believe it myself, I've heard various rumors that there are some contrib projects for D6, which implement a third-party library that actually supports full unloading of JS and CSS files (it's the JS part that baffled me). Given that (not verified) info, I suspect that a contrib module could relatively easily add an AJAX framework command to unload JS/CSS files from the page. Due to the current design, the AJAX framework would simply send those removed files for "reloading" if they happen to be needed in a later request again.
Food for thought.
If all fails, then we should merely make sure that a contributed module is able to replace resp. plug in to the affected functionality without having to hack core.
Comment #13
chx CreditAttribution: chx commentedIs this going to break any existing Drupal 7 theme that happens to theme by an ID? If so, then how is this D7 material?
Comment #14
rfayAll theming by HTML ID was already completely and permanently broken by the rework of drupal_html_id() a very long time ago. So that's not a happy thing, but it did happen like 8 months ago, so any theme that uses ID is already up the creek without a paddle.
Comment #15
chx CreditAttribution: chx commentedIf I understand what you are saying then a theme created in the last eight months could use IDs because they were last broken eight months ago?
Comment #16
rfayIn Drupal (IMO), CSS cannot target IDs at all, as they are unique (and can be volatile). At least IDs that are generated by drupal_html_id(). So every time AJAX generates some new code it comes in with a new ID. If CSS is targeting IDs, #fail.
Comment #17
dmitrig01 CreditAttribution: dmitrig01 commentedWell, then what's the point of using IDs, or having the in markup at all?
Comment #18
sunIDs are the fastest way to select elements in JavaScript, so IDs are still helpful for operations like AJAX. It's true Drupal's auto-generated IDs should not be used for theming - for that sake I already discussed with eff some time ago that we should actually make drupal_html_id() generate much shorter and completely random machine IDs, because the current human-readable output sends the wrong signal and makes people believe it is safe to rely on them.
We already changed lots of CSS and also JS code in D7 to no longer target IDs, but CSS classes instead - and to aid facilitating that, CSS classes have been heavily improved, too.
However, the impact of Drupal 7's HTML IDs on themers is not the point here. Regardless of auto-generated, human-readable, cryptic, or not, the AJAX framework must not generate potentially duplicate IDs. Currently, that is ensured by making the client send all known IDs to the server.
The real problem we're facing here is that AJAX always performs POST requests, which auto-disables all kind of built-in caches. In order to appropriately use GET requests, we need to find a way to decrease the HTML ID and page state information that is sent to the server in order to prevent duplicate IDs and lazy-load asset files.
Comment #19
Paul Natsuo Kishimoto CreditAttribution: Paul Natsuo Kishimoto commentedThis is an interesting problem.
ajax_html_ids
: Following on sun's point 1 in #12, why not have the client generate the hash/salt, check it and send it with the AJAX request?If I read the documentation correctly,
drupal_html_id()
generatesexample--1
,example--2
, etc. for$id = 'example'
. If the client sent$salt = 'qZ6'
, then the request could returnexample--qZ6--1
,example--qZ6--2
etc. without risk of collision. On the next request,$salt='54k'
or something else. Am I missing something?ajax_page_state
: If the original cache idea will not work per sun's point 2, I don't have any good ideas about this. To reduce the size of the payload, maybe convert:'a/b/c/d1.css', 'a/b/c/d2.css', 'a/b/c/d/e1.css', 'a/b/c/d/e2.css', 'a/b/f/g.css'
into JSON:
{'a/b': {'c': {'d1.css': 0, 'd2.css': 0, 'd': ['e1.css', 'e2.css']}, 'f/g.css': 0}}
…on the face of it this doesn't look like it saves any space, but since real path components are more than one character each, it would. One could also omit ".css" and ".js" or get creative in other ways.
Comment #20
yhahn CreditAttribution: yhahn commentedSome quick notes on Sun's comments:
This is true but I'm not sure there is anything core can really do about this. If a contrib module chooses not to use
drupal_html_id()
for the initial page response it's likely it won't be used for the AJAX response, in which case whether the client sends back all the HTML IDs or not makes no difference. Basically, if you aren't using the core AJAX framework you're on your own (with or without this patch).Yes. A cache for looking up sets of JS/CSS files via hash would be equivalent/similar in many ways to {cache_form}. If we were to establish such a cache I would not be very concerned about fragility - if the site successfully sent the page then it will be able successfully respond via AJAX for that page.
It would be nice to see some discussion on this issue around the actual patch as it does let you test these assertions and behaviors against a real site : )
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedI re-read through all the AJAX code related to ajax_html_ids and ajax_page_state, and I agree with #11 that the only thing not easily overridable by a contrib module is the implementation of drupal_html_id(). Everything else is totally straightforward to solve in contrib once the details are worked out. In other words, working out a server-side CSS/JS file list cache along the lines of #6 will still take some work, but once worked out, implementing it in contrib, with existing core hooks, should be possible, because on the JS side, Drupal.ajax.prototype.beforeSerialize() can be overridden, as can the
type: 'POST'
declaration in Drupal.ajax(), since that's a per-object default that can be changed on a per-element basis pretty much anywhere in the behavior/AJAX pipeline. On the PHP side, we have hook_js_alter() and hook_ajax_render_alter() for changing the used CSS/JS file list from persisted client-side to persisted server-side. And ajax_base_page_theme() can be overridden via hook_menu_alter(), so it hard-coding $_POST is not a problem.But as per #12, #13, #19, and #20.1, I don't think we should change the base implementation of drupal_html_id() this late in D7, but I do think we need to make it alterable. So, here's a tiny patch to do that.
That's an API addition, not a BC breaking change, so removing the "API Change" tag. For that reason, it can be committed after RC1, so downgrading from critical to major.
Comment #22
chx CreditAttribution: chx commentedThat's a good move for sure. *everything* needs a hook_alter :D
Comment #23
webchickI'm really not comfortable committing a patch that adds +function hook_html_id_alter(&$id, $original_id, $seen_ids) {. What a gigantic hack on top of a pile of fail. :( (Can't target IDs with CSS? We call ourselves a framework??)
Leaving for Dries to make a call on. IMO, we should attempt to fix this in contrib (I guess? sigh.) and only come back here to add this if this is actually, truly, absolutely necessary.
Comment #24
effulgentsia CreditAttribution: effulgentsia commented@webchick: it's certainly within your right to not like this, and to leave it for Dries.
But I'd like to point out that it is already the case that you shouldn't target IDs (except ones you really know to be unique on a page, like ones that are in page.tpl.php) from CSS, and precisely *because* Drupal is a framework. Even the D6 precursor to drupal_html_id(), form_clean_id(), made it so that if you have two form elements on a page, that both would have otherwise had the ID 'foo', that instead the second one is changed to 'foo-2'. It was correct that in D7, we changed the function name from form_clean_id() to drupal_html_id(), and started using it from contexts other than forms.
That we didn't finish the job and still have this in node.tpl.php:
is really a shame, but unaffected by this patch. This *should* be changed to call drupal_html_id(). But until it is, is unaffected by drupal_html_id() potentially returning a different ID than what was passed. In the meantime, what this means is that a page that renders the same node in two different places on the page will fail HTML validation.
So, we have this dilemma. CSS that targets #node-42 currently works, but a page that outputs the same node twice on a page fails validation. If we fix this to use drupal_html_id(), then we fix HTML validation, but the CSS only targets the first one and then fails to target the other ones, since they're changed to "node-42--2", "node-42--3", etc.
All this is true with or without this patch, and with or without AJAX. What AJAX introduces is the idea that "uniqueness on a page" needs to be tracked across page requests.
What the patch introduces is a way for a contrib module to implement uniqueness in some other way than the way done by core, because the way done by core, while working, comes with a performance and caching limitation that a contrib module may want to solve. A well-written contrib module could still implement a strategy by which the first time an ID is used, it is unmodified, so that CSS that targets an ID only breaks precisely in the exact same situation in which it already would have broken anyway even without that contrib module.
So yes, we're a framework: one that allows (and encourages) pieces to be combined in flexible ways. But IDs must be unique per page (blame the W3C for that one). So, the price of this is to not rely on a hard-coded ID of a piece that can appear more than once on a page.
Would more docs in hook_html_id_alter(), saying something like "WARNING: don't break CSS and JS for no good reason: consider the implications of altering Drupal's logic for outputting unique IDs!" help?
Comment #25
sunThis is the best we can do for D7 and for now. It doesn't fix the underlying bug, but it allows contrib to fix it. Without this patch, AJAX GET requests are impossible.
Comment #26
webchickI don't understand how not committing this patch means that AJAX GET requests are impossible. I understand that this is the only part of the rendering chain that's not alterable, but the idea that this needs to happen in order to support GET requests is currently a theory, not a proven rule, since no such code exercising these alters exists. And if it did, we'd be considering it for core inclusion. Or am I mistaken?
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedyhahn's #6 patch includes this hunk:
This replaces the current drupal_html_id() counter-based suffix with a random suffix. But I don't think it's good for core inclusion this late in D7, since I'm not sure randomizing IDs is the best of all possible solutions.
#19 proposes an alternate approach which is to have the client provide a salt as part of the AJAX request. This is appealing to me, and what I included as the example in #21, though this too may warrant more thought and in-the-wild experience. For example, does requiring the client to provide a salt minimize the effectiveness (i.e., hit ratio) of Varnish caching, which is one of the main benefits we want to get out of using a GET request?
It is also possible that a server-side page state solution can be invented in contrib. This is what I allude to in this comment within drupal_html_id():
I was hoping to have added a page state API to D7 core, but wasn't able to. A precursor to it is the D6 contrib module, AHAH Page Storage, except that module has problems making it unfit for core in its current state. The idea is simple, just like we have a $form_build_id from which the server can recover $form_state, it would be great if we could have a $page_build_id from which the server could recover $page_state, and this $page_state could contain everything the server would need to know about the state of the page, including drupal_html_id()'s $seen_ids static. But, a challenge with implementing this properly is that the database table holding this info would fill up too fast on a busy site. We already run into this with {cache_form}, but that only gets populated for multistep forms, and entries expire every 6 hours. But a {page_state} table would fill up faster, and since the purpose of this issue is to support Varnish caching, 6 hour expiration might be too aggressive.
So, already we have three possible approaches:
It's not clear which of these will end up being best, or maybe yet another idea will surface. Adding the alter hook will allow all of these to be explored in contrib and tested in the wild. Then, perhaps, we can implement the best one in D8 core.
Comment #28
rfayHas to go into 8.x first.
Comment #29
sun#21: ajax-html-id-956186-21.patch queued for re-testing.
Comment #31
Steven Jones CreditAttribution: Steven Jones commentedI'm going to resurrect this issue and attempt to move discussion along a bit, it is still a problem for Drupal 7 and 8.
The issue summary is actually really good, but I'd like to propose some alternative solutions or clarify existing ones:
To solve the unique ID issue, I reckon that the client can simply generate a unique 'salt' and send that to the server,
drupal_html_id
then just needs to append that salt if it exists. It can do so by suffixing any Drupal ID with say: ':.:SALT' which is still a valid HTML ID I believe, we then disallow the ':.:' from IDs to avoid a collision. This salt can simply be a monotonically incrementing integer, so clients send 1 on the first ajax request they make, and then 2 and three and so on. This way these requests can be cached, and those cache entries will actually get hits, unlike just adding some random strings.To solve the large arrays of CSS and JS filenames, we really don't want to go down the line of a server side cache really. cache_form is just nasty, and it gets really big, really fast!
All we need to do here is be a bit smarter, we don't need to send the whole file path to the client to have it sent back to the server. What we can do is send an array of hashes of the file paths to the client, and then get them back, and do our checks as before, but hashing at the right moment. Note that we could get away with using a git style six digit hash, rather than a full 40 characters per file.
It should be noted that this array is delivered to the client on every single page request, not just AJAX ones. Go look at the Drupal settings array on your D7 site, I bet it's massive!
We can further optimise this by removing entries in this array that are assured to be delivered to the client on every single request, i.e. those defined in info files.
Actually the biggest problem in both of these issues is that we are sending this data as a massive array, so if you look at the POST request we have the names of our array variables repeated over and over and over. This seems extremely wasteful. We could add a little processing to each side to implode and explode the arrays into strings, thus only sending the name of the variable once. I'm not sure if this suggestion would be good from a security point of view though.
Comment #32
nod_Wow, working on ctools and views that ajax ids and css made me crazy too.
I think it comes from a biais of the "typical Drupal developer", everything in done in PHP. I'd like to talk about CSS files, there is already lots of good ideas about how to handle ids.
From what I understand, css/js files are sent to avoid adding them again if they are already present in the page, and only for this purpose. In ajax.inc there is
// Ensure that the page doesn't reload what it already has.
Now why is this done on the PHP side? there is already an ajaxPageState variable with what's already there, we should add a couple of ajax command that would load CSS and JS files where we can do this check and forget about posting all CSS and JS files in the request. The diff would have to be made before the commands potentially changing ajaxPageState are processed obviously, that's pretty easy to do.
Actually with a few tweaks it would even be possible to get css files from the head and contents of the aggregated css files directly with a regex targeting a specially formated comment containing the filename, i'm not for this particular solution but just to point out there is a lot more that can be done on the js side of things.
Comment #33
sunPlease read up on #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework first, which introduced the current AJAX lazy-loading mechanism. Its architectural design was discussed in detail - there have been multiple, entirely different technical implementation approaches, and the current is the one that resolved all needs and edge-cases.
Comment #34
nod_Not sure what you're talking about, in the other issue, the architectural discussion ended at #24 after merlinofchaos said xLazyLoader will not work because it doesn't look for duplicate (to which I said it's our job to worry about that) and it wouldn't work for aggregated CSS files (to which I gave a solution, a bad one, but still), but more importantly I'm not even talking about some kind of loader. The rest of the thread is focused on how to implement merlinofchaos approach best.
The thing is, it's this patch that introduced ajaxPageState, and I'm talking about using precisely that to check for dups. It's a different issue because the context here is different 1 year after. That was an interesting read but that was no architectural discussion.
I'm not saying ajaxPageState should be remove, I'm saying is should only be posted back from PHP not to PHP. Instead of fat ajax requests and responses we'll only have fat ajax responses. Which is not ideal but better and will make it possible to use GET request which is what it's all about here.
To avoid fat responses we'd have to look into aggregated files to know what's going on which is dirty but possible (and I'm not saying we should do that just yet).
Comment #35
nod_Here you go, css and js files removed from
ajax_page_state
during ajax request. And it's still working fine. Does this makes things clearer?Stuff should be moved around, I don't think the settings command is the best place to do this kind of thing but that's good enough for a proof of concept.
It's only a patch for files, the is nothing to remove ids in this patch. It's probably going to break tests but meh.
Comment #37
Steven Jones CreditAttribution: Steven Jones commented@nod_ https://drupal.org/node/561858#comment-3404404 was the comment that nicely summarises why the current approach is in core.
Comment #38
nod_Thanks for pointing it out, I didn't give enough attention to the linked post it seems. That makes things simpler, the html_id part of this thread which is fixable can be ported to another issue and this one closed as it works as designed. There are a few solutions to limit the impact but nothing will solve this issue properly unless the whole stack is revisited.
To get small ajax http request, you just can't use Drupal AJAX framework. It should be possible to refactor the ajax commands system to be used easily by third party but that's an other issue as well. It depends on what op wanted to do. Or am I once again wrong?
Comment #39
Steven Jones CreditAttribution: Steven Jones commented@nod_ I don't see whyt we need to run away to another issue, let's just stay here!
Patches to follow...
Comment #40
davidseth CreditAttribution: davidseth commentedHello @Steven Jones, have you had any progress on this? I am very much for GET ajax calls. That is the architecture of the web, GET to retrieve content (and for caching) and POST to create new content. If Drupal wants to be a player on the modern web it needs to respect the architecture of the web... So a nice skinny GET request would be wonderful!
Comment #41
Steven Jones CreditAttribution: Steven Jones commentedSorry I implemented some proof of concepts for my comments in #31 , but nothing further than that really.
There is realtively little code to change though, these are not major refactorings.
Comment #42
bibo CreditAttribution: bibo commentedThis problem is forcing us to do a lot of custom callbacks just so we can use views + ajax + varnish.
It must be rather common, so I wonder if anyone knows of a D7 module or recipe which would allow all views to use cacheable GETs for Ajax in a simple way?
Comment #43
andypostAlso we should send ajax_html_ids as string not as array. This will reduce a bit requiest size see #1414510-37: Remove drupal_reset_static for drupal_html_id() when form validation fails
Comment #44
andypostFiled separate issue about serialization of ajax_html_ids #1575060: ajax_html_ids are broken for forms with file element (encoding=multipart/form-data)
Currently this functionality broken for forms with file element
Comment #45
scottatdrake CreditAttribution: scottatdrake commentedI'd love to know this. I'm looking into it now, but there is a ~lot~ to wade through as somebody who hasn't dug into Views' or Drupal's ajax systems in-depth before. Suggestions welcome.
Comment #46
scottatdrake CreditAttribution: scottatdrake commentedEDIT (thought comments were threaded)
Comment #47
sun#1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests has been filed and duplicates this issue to some extent, but I really appreciate the much more limited focus in that issue. The large scope of this issue here is what got us stuck, IMO.
So I'm taking this chance to turn this into a meta issue, which we can use to discuss the overall plan and to track the progress of individual spin-off issues.
Comment #48
sokrplare CreditAttribution: sokrplare commentedWorking with @scottatdrake and using the patches in comment #1 and comment #15 on #591302, I was able to get Views AJAX paging working with GET requests. I checked out these revisions for reference:
The patches applied cleanly, setup a normal View and voila - GET request!
However, the whole reason we needed this was for Varnish caching (as well explained by someone here http://agaric.com/blogs/better-performance-dont-use-ajax-views). And unfortunately, I don't think this will do it as there are three items that change per request/user - these are the ajax_page_state values for token, css and js.
Granted, that likely can vary based on what is on the page etc. I'm clearly in over my head here so just trying to record observations in hopes that it helps someone else (or myself when I come back to it!)
I guess the question for caching is if we can strip/skip/eliminate those unique state values in some fashion for specific requests like our slideshow Views AJAX pager. Going to look into that next - pointers welcome.
Screenshot attached of working implementation doing AJAX GET requests!
Comment #49
sokrplare CreditAttribution: sokrplare commentedUpdate: Just realized all this would take is a two-line change to a file in Views if I only need it for AJAX paging with no additional CSS/JS loading - no core changes, etc. etc. Posted that here #591302-23: Rewrite ajax_view.js based on ctools AJAX
I've been working on this a bit more, as it is still very high priority internally so we can use caching on slideshows (pager with 1 item per "slide"). I've ported the patch from #956186-1: Allow AJAX to use GET requests to apply against 7.16 - which likely means it will apply cleanly or very close to cleanly against 7.x. Patch attached! I know this isn't going into D8 or anything, but in case someone else absolutely needs this like we do in the meantime. This works in conjunction with the patch I'm posting to #591302-22: Rewrite ajax_view.js based on ctools AJAX shortly.
The one rather unfortunate issue with this is this core patch breaks the Views UI - best I can tell it is because AJAX is used and therefore the hash is applied to element IDs, which Views javascript isn't expecting. For example, the submit button on the add Filter/etc. pop-up expects to be
#edit-submit
, but with this patch being used in core, it instead comes out#edit-submit-[hash]
- so Views UI javascript has to be turned off for it to function.Two notes for other Varnish cache folks:
Drupal.ajax.prototype.beforeSerialize
and at least for AJAX Views paging it seems to still work fine, but BEWARE as if you have any JS/CSS files that should be added on AJAX loads I'm pretty sure this will break that, aaaand it may break a whole lot else as I'm over my head here so it isn't exactly recommended :)P.S. @sun, would it be best if I spun off another issue or just edited these existing postings if I have updates? I don't mean to hijack this into a 7.x workarounds thread, just wasn't sure it justified it's own issue initially.
Comment #50
j0rd CreditAttribution: j0rd commentedI've been talking with _nod in another issue and me made me aware of the work going on here. Here's that issue:
#1565704: Core interfaces can go over max_input_vars
One thing to keep in mind when developing this patch is suhosin & php's post/get variable limits. Here are some suhosin vars which you guys need to keep in mind, and try not to overflow, or at the very least, must document that defaults need to be increased to work with Drupal.
A tricky thing with suhosin, is that it's error log is added to syslog instead of apache logs, so you'll need to look at /var/log/syslog to see if things are failing (silently truncation of get vars, not outright errors).
Get defaults on debian lenny:
Post defaults on debian lenny:
PHP after 5.3.9 also has this variable defaulted to 1000.
It seems suhosin limits gets arguments to 100 by default. Also values of each get can only be 512. For forms with a combination of more than 100 unique HTML ids, Drupal's ajax will not be able to get the current Drupal.settings.ajax_page_state.js files, as they will be truncated by suhosin and will re-include them in the response.
For me, I ran into this when using display suite, manage display on a node with 50 fields. Drupal Ajax was passing 2000+ variables via POST...so you'll also have to avoid this happening with GET if possible.
#1870014: Admin Interface Fail: PHP max_input_vars & suhosin post variable limit with a node with lots of fields (50+).
Comment #51
jamix CreditAttribution: jamix commentedWhen considering a solution for minimizing the size of Ajax requests (to make GET requests and therefore caching possible), we also have to consider the implications on the expected cache hit rate. Passing some kind of a hash instead of the full list of loaded CSS and JS files will help bring the request size down, but it won't address the problem of potentially ending up with copies of the same Ajax response in the cache that only differ by the additional CSS and JS files to be loaded.
What I mean is this: Suppose an Ajax request is performed on a page that has a certain set of CSS and JS files loaded. The request will carry a hash for that particular set of files so that the server can return the list of additional files to load. Now, this same Ajax request is performed on a different page with a different set of loaded CSS and JS. A cache miss would occur simply because the hash (i.e. the CSS/JS state) will be different.
This may be painful, but I think that for a truly versatile solution to be possible, it has to become the job of the client-side JavaScript to determine which extra files to load, based on the server's response. That way, the client won't have to pass its CSS/JS state to the server and the server will always return the full list of CSS and JS that are necessary for the returned content to function. Again, it will be the job of the client-side JavaScript to determine what files are new and have to be loaded.
So to return to the #125 vs. #132 discussion over here, I think that as convenient as it may be to leave the heavy lifting to the server side, it's not going to work.
Comment #52
jamix CreditAttribution: jamix commentedAs a follow up to my last message, here's a link my sandbox project that makes a persistent audio player possible by reloading full pages via Ajax: Ajax pages. (Demos: Mediazoic, New Canadian Music).
The Ajax requests are made via GET and are fully cacheable. To achieve this, I leave it to the client to figure out what extra CSS and JS files need to be loaded instead of POSTing
ajax_page_state
to the server and letting the server do that. We do miss out on the aggregation because the extra files are loaded one by one but I'd rather have the caching (with decent hit rates - see my previous message). And since we're dealing with reloading full pages, we don't have to bother withajax_html_ids
at all.Hopefully that code can contribute to the discussion.
Comment #53
Steven Jones CreditAttribution: Steven Jones commentedAdd #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests as a related issue, as they are trying to reduce the amount of data sent around.
Comment #54
Steven Jones CreditAttribution: Steven Jones commentedComment #55
catchComment #56
Leon Kessler CreditAttribution: Leon Kessler commentedI've come up with a bit of a different approach, removing the ajax_html_ids and ajax_page_state from the request altogether.
I've done this in my own module, which monkey patches Drupal.ajax() to make the request run through GET.
I'm currently only trying this out on Views, I don't want to override the core ajax framework for everything.
Still not sure if this is a good idea or not.
Here's the module: https://drupal.org/sandbox/leonnk/2232463 if anyone is interested.
Comment #57
nod_That's the idea, in realted issue there is a patch to remove ajax_html_ids, next we need to transfer the logic around knowing which additional assets to load from the PHP to JavaScript, that we we maintain the state of CSS/JS on the client side instead of the PHP Side.
Comment #60
effulgentsia CreditAttribution: effulgentsia commentedThis should become much easier once #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests is done.
Comment #61
Wim Leers#2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests landed. Hence the IS now outdated, since the size of
ajax_page_state
is no longer prohibitively large.AFAICT this is now sort of blocked on #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests, which will remove the
ajax_html_ids
from AJAX requests and hence allow us to do what this issue is about: change Drupal's AJAX requests to use GET instead of POST requests.EDIT: fix typo.
Comment #62
Wim Leers#1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests landed. Which AFAICT means we can now actually finally do this!
Comment #63
geerlingguy CreditAttribution: geerlingguy commentedCan we get an issue summary update? It seems the IS is quite a bit out of date (and only mentions D7)—it's not very clear what the next steps are in this particular issue (especially in light of #60+.
Comment #65
catchHere's very initial draft. If you set the ajax request method to GET we send $ajax_page_state as a GET param, but the attachments processor looks in $request->request which for whatever reason doesn't have it. Explicitly checking the query if it's not a POST request finds it.
Comment #66
dawehnerI was wondering whether we could gzip the page state. Maybe this would help us for big ajax page states.
Comment #68
catchSlightly more patch.
gzipping the page state is interesting.
Comment #69
catchThat's going to crash and burn.
Comment #72
dawehnerSeriously catch, 14 failures. You disappoint these days!
Don't we need some code in ajax.js to also use query parameters?
Comment #73
catchComment #74
catch@dawehner jquery already handles sending the data as query params if the request method is GET and ajax.js uses that - it just always sends as POST. So if this is green, then theoretically things work.
Most obvious use case in core I can think of is ajax-enabled views, which already has an issue #2500313: Add views render caching on views ajax requests.
Comment #75
dpolant CreditAttribution: dpolant commentedI've done a couple things:
1) Rerolled the diff from ajax-956186-65_1.patch so that it applies to latest dev.
2) Added support for type option in /misc/ajax.js
ajax-956186-65_1_8-1-8.patch should work with 8.1 dev/stable.
ajax-956186-65_2.patch should be tested against 8.2
To go along with this, I have a diff ready for views that makes AJAX paging work.
Comment #79
dpolant CreditAttribution: dpolant commentedThose last patches had a mistake in the ajax.js. These should be better and should fix the test failures.
I changed the names to make more sense (1x is 8.1.x, 2x is 8.2.x).
Comment #80
catchComment #81
dawehnerIt would be great to have some prove to show that GET ajax requests actually work. You know, there could be more hidden gems out there.
Will we opt into that behaviour?
Comment #82
dpolant CreditAttribution: dpolant commentedIf you apply the patch from https://www.drupal.org/node/2500313 you can switch views to use GET.
Comment #83
catchFor now I think we should opt-in, we can have a separate issue to make it the default - depends whether we consider that a proper API change or not.
I think combining this issue with #2500313: Add views render caching on views ajax requests would be good to have an example in core and also handles test coverage.
Comment #85
dpolant CreditAttribution: dpolant commentedHere's another version of the 2.x patch with another $ajax_page_state call changed.
Comment #87
slasher13unrelated fail -> retested
Comment #90
pjcdawkins CreditAttribution: pjcdawkins commentedThe title was totally confusing me ("use AJAX with GET requests" vs "use GET with AJAX requests"). Also I'm not sure of the state of this - the patch looks helpful but it's from many months ago, and I'm sure there's more work to do with other components.
Comment #91
Wim Leers@pjcdawkins That's a better name indeed, thanks!
Comment #92
braindrift CreditAttribution: braindrift commentedHi @all,
in general the patch from #85 works fine for me but there seems to be a problem with
drupalSettings.ajaxPageState.libraries
. If a library is added during an ajax request, the ajaxPageState.libraries contains duplicate values. This is not a big problem when using POST but becomes an major issue if the ajax is made with GET because after each ajax request the ajax URI becomes larger and at some point it becomes larger than allowed and I get a "414 Request-URI Too Large".Comment #93
Pere OrgaHi @braindrift
I understand that this is an existing bug, not specific to adding support for GET in AJAX. Please can you create a separate issue?
URLs too large may be a problem/limitation of using GET. But I don't think this can be addressed in Drupal without making the request shorter. You can override these limits in your server configuration (In Apache, look for LimitRequestLine)
Comment #94
braindrift CreditAttribution: braindrift commentedCreated a separate issue.
Comment #95
dpolant CreditAttribution: dpolant commentedComment #96
dpolant CreditAttribution: dpolant commentedConfirmed same patch passes for 8.3.x.
Do we need tests for this? Take a look at #2500313: Add views render caching on views ajax requests - there are a few tests in that patch that show this working but they're for Views.
If other tests are needed, can someone give me some guidance on how to structure them?
Comment #98
Denis Danielyan CreditAttribution: Denis Danielyan commentedAFAIK there is one more piece of the puzzle missing.
in D8, the AjaxRenderer is not caching content.
To address this issue we have developed the following module that adds a cached ajax renderer:
https://www.drupal.org/project/ajax_cached_get
As described in the module, the url needs to be changed to use the correct renderer but that should basically be it.
Comment #99
dawehnerI really think this needs tests, which would have caught that problem @Denis Danielyan just described.
Comment #101
anavarreComment #103
kaythay CreditAttribution: kaythay at Aten Design Group commentedA reroll. Still needs tests.
Comment #105
kaythay CreditAttribution: kaythay at Aten Design Group commentedSorry, had been working against 8.6.x instead of 8.7.x
Comment #106
kaythay CreditAttribution: kaythay at Aten Design Group commentedComment #107
tim.plunkettThis (and the others in the patch) could be changed to
$ajax_page_state = $request->get('ajax_page_state');
See
\Symfony\Component\HttpFoundation\Request::get()
for more, but it supports this order of precedence by defaultComment #108
cchoe1 CreditAttribution: cchoe1 commentedAm I misunderstanding something? I don't see how any of the provided changes allows for us to specify whether or not we want to use a GET or POST request. ajax.type is always undefined so it always sets as POST
Comment #110
geek-merlin#107: Yeah
so the patch should shrink down to this tiny change.(only one code fragment already does that) (but see below)#108: Here it is. Note that this is opt-in and surely needs a change record to inform developers. If i get this right,
type: 'GET'
must be added to the settings parameter of Drupal.ajax.Also, digging through the source, it looks like defaulting ajax.type should be done in the defaults variable of Drupal.Ajax (note the big A!).
Comment #111
geek-merlinPatch flying in with these changes.
Comment #113
super_romeo CreditAttribution: super_romeo as a volunteer commented+ 1 for Needs review :)
Comment #114
SteffenRIn case of using links with .use-ajax, it's not possible to add type for the ajax request.
Right now it defaults to POST.
While looking at the current patch, the es6 files should be handled also - these are missing right now.
I'll rework the patch fixing those issues.
Comment #115
SteffenRAttached the patch fixing the issues mentioned above.
Comment #116
agiraud CreditAttribution: agiraud commentedIs there any way to make the ajax requests cacheable when using .use-ajax ?
Why is it not possible to add type for this case ?
Comment #117
owenpm3 CreditAttribution: owenpm3 at Mobomo commentedI was able to use the patch in #115 successfully. I was able to change the ajax view to use GET and it showed up correctly on the frontend.
Comment #118
catchThe patch itself looks OK, but this still needs an issue summary update - it's listing blockers that were fixed several years ago at this point (css/js list was replaced with a list of libraries, and ajax_html_ids is gone). Could also use a change record.
Also needs test coverage for a GET AJAX request.
Comment #120
SteffenRAttached an updated patch against latest 8.9.x.
Comment #121
SteffenRRerolled patch / fixed spacing issues in ajax.es6.js..
Comment #122
super_romeo CreditAttribution: super_romeo as a volunteer commentedThank you for reroll!
Comment #123
SteffenRI forgot to add the interdiff - let's do this with this comment.
Comment #124
sanket1007 CreditAttribution: sanket1007 commentedPlease ignore this patch. I confirm that #121 works for me.
Comment #125
sanket1007 CreditAttribution: sanket1007 commentedComment #127
mr.york CreditAttribution: mr.york at Agence Inovae commentedReroll #121 patch.
Comment #128
raman.b CreditAttribution: raman.b at OpenSense Labs commentedComment #129
olivier.br CreditAttribution: olivier.br commentedComment #130
nod_consolidating issues under the JavaScript tag
Comment #131
mr.york CreditAttribution: mr.york at Agence Inovae commentedComment #133
andypostComment #134
Lukas von BlarerI have no idea if this is intended or not: a view with an exposed filter makes the first AJAX via GET and all subsequent ones via POST with the patch in #128.
Comment #135
Lukas von BlarerComment #136
Lukas von BlarerFor other people having this issue with exposed filters: views_ajax_get solves this for me.
Comment #138
melvinlouwerse CreditAttribution: melvinlouwerse at Ibuildings for RTL Nieuws commentedreroll
Comment #139
rwaery.11 CreditAttribution: rwaery.11 commentedUpdated patch to correctly get dialogOptions from query parameter when set.
Comment #141
Supreetam09 CreditAttribution: Supreetam09 as a volunteer and at Acquia commentedAdding patch to fix custom commands failure.
Comment #142
catchNote that we might be able to reduce the size of
ajax_page_state
further after #3303067: Compress aggregate URL query strings.Re-rolling for 10.1.x - most of the request changes are no longer necessary due to request->all() for Symfony 5/6 compatiblity already being done.
Comment #143
catchThis starts adding #type support to #ajax - current patch only allows it for links.
Comment #144
catchLooking at views AJAX support, which is the main (only?) non-form AJAX use case in core.
This gets me a valid AJAX response from the views AJAX controller, but the page doesn't actually update so work in progress.
I'm just switching from POST to GET for now, not trying to support a configuration option.
Comment #145
catchOK this actually gets Views AJAX working with manual testing (hardcoded to GET instead of hardcoded to POST). Haven't run all the views tests yet, but you can manually test by switching the dblog view to use AJAX then using the pager (as long as there's enough log messages for a pager).
Comment #146
catchInterdiff is against #142 since that makes the most sense for reviewing changes since the re-roll.
Comment #147
catchUnused use statements.
Comment #149
catchOpened #3303646: Convert Views to use GET for AJAX for actually implementing this in Views.
Comment #150
b_sharpe CreditAttribution: b_sharpe at ImageX commentedTo actually get the full benefit here (i.e. CDN/Page cache), wouldn't we want GET request to return a cacheable response by default as well? Or would that be the controller's sole responsibility? #2701085: Cacheable Ajax response
Comment #151
catch#2701085: Cacheable Ajax response is in. Will try to revive the patch here soon. We probably need #3303067: Compress aggregate URL query strings which is RTBC.
The views tests failures we might just want to change the test assumptions there but needs a bit of a closer look. BigPipe is a bit more ominous though but see how things are going after a re-roll.
Comment #152
catchRe-roll + a couple of test changes.
Should be just down one Standard/BigPipe test failure now, haven't looked into that yet.
Comment #154
catchOK I think I found the big pipe test failure. Big pipe fakes an AJAX request, this means that it's literally the only current 'GET' AJAX request in core because it doesn't set the request type to POST, but it's setting ajax_page_state on the request as if it's a POST request. Because core's current AJAX system only ever looks in request->request this is fine in HEAD, but when we have ajax_page_state in either request->request or request->query and there's a mismatch it breaks.
I think there's probably a question of whether we'd want to implement a fallback bc layer for the behaviour change, where GET requests look in request->request if nothing is found in request->query for ajax_page_state, that should allow code doing the same as big_pipe to work without any changes (apart from deprecation errors). Somehow doubt any modules other than big_pipe are doing this except perhaps in the occasional test though.
However, trying to get to a green patch first.
Comment #155
catchIf we want to compress ajax_page_state using the new API from #3303067: Compress aggregate URL query strings (likely) it won't be possible to provide a bc layer, so I don't think we should here, will need a clear change record and maybe contrib issues opened for any modules doing something weird.
Comment #156
nod_Haven't found why yet butchanging the /admin/content view to use ajax we get a little styling problem with this patchLots of classes don't get added to the html sent back from the GET request
Comment #157
Wim LeersBigPipe changes: LGTM!
Comment #158
nod_Theme is claro in the GET request, In the response the settings are updated to have
ajaxPageState.theme = "umami"
Comment #159
catchThanks for the testing in #154 - this is because the patch also needs to update the AJAX theme request negotiator to handle GET requests too, in the same way as the attachments processor.
Also adding some boilerplate for a fallback in the attachments processor.
Comment #160
catchComment #162
catchAlright the bc layer was trying to be a bit too helpful - ajax_page_state isn't always set. This makes it only trigger the deprecation when it actually finds something in the wrong place.
Still some new test failures with this I think, but stopping for the day/week.
Comment #164
catchCouple more test fails down in ViewsAjaxControllerTest.
That leaves media library test - the bug is real, but there's a lot of custom AJAX stuff in there and I can't track down an issue now, looks like something has been made a no-op since there's no errors either in PHP or console.
And the exposed filter test which I might look at next.
Comment #166
catchShould be a green patch now hopefully.
The last remaining fail was tricky to track down, will try to summarise:
- HEAD Views always uses POST requests for AJAX, because that's all there is.
- patch converts Views to always use GET requests for AJAX.
- media library builds an ajax-enabled view in an AJAX form submission, which is a.... wait for it... POST request!!!
Because ajax_form=1 was in the URL, it was erroring out with a very odd BrokenPostRequestException thinking it had a form without a form_id - but that's because the logic was looking for something that we were mimicking by accident, the POST wasn't actually broken as such just expectations not matched.
So... even if we use GET in views by default, we still have to support POST, which just means reverting a couple of hunks, then it all works again with zero changes to media_library despite it doing some very, very complex AJAX logic with custom js and etc.
Working on that made me realise a lot of repeated logic can be simplified if we just use $request->get('foo') which is explictly there for controllers that are sometimes GET and sometimes POST so the patch gets a tiny bit smaller.
Comment #167
catchGiven Views + media library provides a lot of functional javascript test coverage, I think we should mark #3303646: Convert Views to use GET for AJAX as duplicate and handle views here - the views part of the patch is not massive.
We probably still need to integrate #3303067: Compress aggregate URL query strings into this issue once it's committed (and maybe postpone on that) to avoid monster query strings. It means compressing and uncompressing
ajax_page_state['libraries']
.Comment #168
szloredan CreditAttribution: szloredan at 1xINTERNET commentedReroll for 9.5x
Comment #169
catchThe patch applies to 10.1.x, and won't be applied to 9.5.x (or 10.0.x), so there wasn't a need to re-roll. It could use reviews and testing though.
Comment #170
catch#3303067: Compress aggregate URL query strings landed, which means we can look at compressing ajax_page_state here now. Leaving needs review for #166 since it still needs reviews.
Comment #171
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #172
catch#166 still applied, and I noticed that #168 was missing several hunks from the patch.
Re-uploading #166, re-rolled just in case there's any patch fuzz but not sure there was even that.
Comment #173
andypostI bet some fixes will have collision with related #2484991: Add the session to the request in KernelTestBase, BrowserTestBase, and drush
Comment #174
sker101 CreditAttribution: sker101 as a volunteer commentedthe patch no longer applys on latest of 10.0.x, rerolling.
Comment #176
Arlina CreditAttribution: Arlina at Chapter Three commentedHere's a back-port patch that applies to Drupal 9.5.2, if it's useful to anyone: 956186-175-for-D9.patch
Comment #177
lauriiiPatch no longer applies
Comment #178
smustgrave CreditAttribution: smustgrave at Mobomo commentedPer @catch in #167 should #3303646: Convert Views to use GET for AJAX be included here?
Comment #179
catch@smustgrave - that's actually done in the patch. I think it makes sense to do them together, since Views is more or less our only non-form use of AJAX in core, so going to go ahead and mark the other issue duplicate.
Comment #180
catchLooking through the related issues I rediscovered #2500313: Add views render caching on views ajax requests which adds a configuration option to views pagers whether they use GET or POST or not.
If we wanted to do that, we might want to remove the views changes from this patch after all, and then focus on that one (which requires this patch anyway). Probably need @lendude here.
Comment #181
LendudeAs @catch pointed out in #179:
much of the confidence we have that this change works is based on existing Views test coverage not breaking. So if we remove the Views changes, we also lose quite a bit of coverage for this change, right? I think that doing the changes to Views here is probably better.
Only without the option to switch back to POST this might break some custom code that depends on this being POST? Probably not likely to happen, but might we need another CR that outlines the Views changes? Or expand the existing one?
Comment #182
catchDiscussed this with @lendude a bit in slack. We both think the configuration option added in #2500313: Add views render caching on views ajax requests could end up being overkill. So, we could continue with just switching Views AJAX to GET in this issue.
That's most likely to be successful if we compress ajax_page_state, so I've opened an issue for that: #3348789: Compress ajax_page_state. Can think about precedence and whether to merge issues once there's a working patch there.
Comment #183
nod_Going to call it, I've clicked around after making core views use ajax and nothing appears broken: pagers in different themes, media library, bigpipe.
removing the needs test tag since views tests are implicitly testing this. added a line about views in the CR
Comment #184
anairamzapHello, we needed this patch on 9.5 so I took the patch in #177 and re-rolled it to apply to 9.5.x.
Attaching it here in case is helpful for someone else.
Thanks!
m
Comment #185
catchI've made a start on #3348789: Compress ajax_page_state.
Comment #186
anairamzapI'm correcting the patch provided in #184 since when I did the re-roll I forgot to:
*es6.js
filesyarn build:js
We need to do those steps since Drupal 9.5.x still uses the build commands :)
Checked the new patch changes running
bash core/scripts/dev/commit-code-check.sh
in local and all checks passed OK, so hopefully they also pass in drupalci.Comment #187
catchComment #188
lauriiiAnother reroll based on #177.
Comment #189
lauriiiI was reviewing the code and I was wondering if we should be using the
method
property fromjQuery.ajax()
instead becausetype
is an alias for it. It seems like it would also be clearer because it would separate this from#type
, and would also align closer to the terminology that is usually used in the context of HTTP requests. However, it looks like we are already using themethod
key in the ajax settings for configuring the method used for inserting new content in the targeted element. 😞I went through contributed modules to see if they would be impacted by this to address #181. I found that there are at least two modules that would be impacted by this; Smart Content module and a submodule of the Content-Security-Policy module. Maybe we could just file issues in their issue queue for this? I also updated the CR to explicitly mention this aspect of the change.
Comment #191
lauriiiCommitted 29763ea and pushed to 10.1.x. Thank you for everyone who worked on this over the past 13 years!
Comment #192
lauriiiFiled an issue in the Content-Security-Policy module queue: #3352875: Add support for GET Ajax requests.
Comment #193
catchDiscussed #189 briefly with @lauriii and we wondered about 'httpMethod' as a key name, so I opened #3352984: Use httpMethod instead of type for AJAX get/post request property.
Comment #195
claudiu.cristeaWhen opening a dialog from a link (the route/controller way) with GET HTTP request, the
data-dialog-options
is not respected. I've opened #3394220: Dialog options are not honoured when open a dialog using GET