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

#2500313: Add views render caching on views ajax requests

CommentFileSizeAuthor
#188 956186-188-10.1.x.patch16.45 KBlauriii
#186 interdiff_184-185.txt2.6 KBanairamzap
#186 956186-174-reroll-95-185.patch18.06 KBanairamzap
#184 956186-174-reroll-95-184.patch16.33 KBanairamzap
#177 956186-174-reroll.patch16.36 KBlauriii
#176 956186-175-for-D9.patch14.22 KBArlina
#174 956186-174.patch16.37 KBsker101
#172 956186-172.patch16.37 KBcatch
#171 956186-nr-bot.txt144 bytesneeds-review-queue-bot
#168 956186-168.patch4.22 KBszloredan
#166 956186-166.patch16.37 KBcatch
#166 956186-166-interdiff.txt5.81 KBcatch
#164 956186-164.patch17.39 KBcatch
#164 956186-interdiff-164.txt1.32 KBcatch
#162 956186-162.patch17.22 KBcatch
#160 956186-160.patch14.66 KBcatch
#159 956186-159.patch14.46 KBcatch
#159 956186-interdiff.txt2.62 KBcatch
#156 Capture d’écran du 2022-11-25 10-52-22.png103.25 KBnod_
#154 956186-154.patch11.73 KBcatch
#152 956186.patch10.65 KBcatch
#147 956186-147.patch8.3 KBcatch
#147 956186-interdiff-146-147.txt675 bytescatch
#146 956186-145.patch7.92 KBcatch
#146 956186-142-145-interdiff.txt6.27 KBcatch
#144 956186-144.patch4.36 KBcatch
#143 956186-143.patch2.5 KBcatch
#143 956186-interdiff.txt868 bytescatch
#142 956186-142.patch1.66 KBcatch
#141 interdiff_139-141.txt345 bytesSupreetam09
#141 956186-141.patch5.43 KBSupreetam09
#139 interdiff_138-139.txt1.31 KBrwaery.11
#139 956186-139.patch5.75 KBrwaery.11
#138 956186-138.patch4.17 KBmelvinlouwerse
#128 interdiff_127-128.txt595 bytesraman.b
#128 956186-128.patch3.87 KBraman.b
#127 drupal-956186-127-ajax-get-reroll.patch4.19 KBmr.york
#124 drupal-956186-124-ajax-get.patch414 bytessanket1007
#123 interdiff_115_121.txt1.36 KBSteffenR
#121 drupal-956186-121-ajax-get.patch4.37 KBSteffenR
#120 drupal-956186-120-ajax-get.patch3.53 KBSteffenR
#115 drupal-956186-115-ajax-get.patch3.87 KBSteffenR
#111 drupal-956186-111-ajax-get.patch3.27 KBgeek-merlin
#105 ajax-956186_105.patch3.76 KBkaythay
#103 ajax-956186_103.patch3.96 KBkaythay
#85 ajax-956186-65_2x_1.patch3.66 KBdpolant
#79 ajax-956186-65_1x.patch3.32 KBdpolant
#79 ajax-956186-65_2x.patch3.12 KBdpolant
#75 ajax-956186-65_2.patch3.11 KBdpolant
#75 ajax-956186-65_1_8-1-8.patch3.31 KBdpolant
#73 ajax-956186-65_1.patch3 KBcatch
#69 ajax-956186-65.patch3.01 KBcatch
#68 ajax-956186-65.patch2.94 KBcatch
#65 ajax-956186-65.patch1.04 KBcatch
#49 956186.1.against_D7.16.b.patch21.19 KBsokrplare
#48 D7 using GET for Views.jpg103.68 KBsokrplare
#35 core-remove-files-from-ajax-requests-956186-35.patch3.78 KBnod_
#21 ajax-html-id-956186-21.patch3.02 KBeffulgentsia
#6 956186.6.patch23.7 KByhahn
#1 956186.1.patch22.39 KByhahn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yhahn’s picture

Status: Active » Needs review
FileSize
22.39 KB
effulgentsia’s picture

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

Status: Needs review » Needs work

The last submitted patch, 956186.1.patch, failed testing.

alex_b’s picture

Status: Needs work » Needs review

Sub - without HTTP GET AJAX no reverse proxy caching for ajax pagers.

rfay’s picture

Status: Needs review » Needs work

Thanks for the good work on this. Subscribing.

yhahn’s picture

Status: Needs work » Needs review
FileSize
23.7 KB

Updated patch fixes HTML ID test.

effulgentsia’s picture

Title: Reduce HTTP request size for requests made by AJAX framework » Ensure it is possible for a contrib module to alter AJAXified links to use GET requests
Category: bug » task
Priority: Normal » Critical

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

mikeytown2’s picture

Some 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

JeremyFrench’s picture

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

if Drupal doesn't provide a way for high performance AJAX, then cutting edge developers will look to other frameworks.

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.

effulgentsia’s picture

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

I think you're right. I'm just not 100% positive.

To re-iterate from #7:

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.

...

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.

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.

yhahn’s picture

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

sun’s picture

Issue tags: +API change

Switch drupal_html_id() from using integer increments to a randomized short hash. Makes collision between requests extremely unlikely and drops the need for ajax_html_ids in the request.
...
Generate a page state css hash and js hash for any given array of css/js files. Store it in the cache table. Transfer this in ajax_page_state instead of the full list of CSS/JS files [...] Upon receiving an AJAX request, use cached list of css/js files looked up by page state hashes. Eliminates the need for client to send the list of css/js files back to Drupal.

I 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:

  1. Only the client knows about the actual HTML IDs on the page.

    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.

  2. Relying on an additional page state cache means that we'd need another, non-volatile cache like {cache_form}.

    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.

chx’s picture

Is this going to break any existing Drupal 7 theme that happens to theme by an ID? If so, then how is this D7 material?

rfay’s picture

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

chx’s picture

If 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?

rfay’s picture

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

dmitrig01’s picture

Well, then what's the point of using IDs, or having the in markup at all?

sun’s picture

Title: Ensure it is possible for a contrib module to alter AJAXified links to use GET requests » Ensure it is possible to use AJAX with GET requests

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

Paul Natsuo Kishimoto’s picture

This 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() generates example--1, example--2, etc. for $id = 'example'. If the client sent $salt = 'qZ6', then the request could return example--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.

yhahn’s picture

Some quick notes on Sun's comments:

Only the client knows about the actual HTML IDs on the page. 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.

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

Relying on an additional page state cache means that we'd need another, non-volatile cache like {cache_form}.

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 : )

effulgentsia’s picture

Priority: Critical » Major
Issue tags: -API change
FileSize
3.02 KB

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

That's a good move for sure. *everything* needs a hook_alter :D

webchick’s picture

I'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.

effulgentsia’s picture

@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:

<div id="node-<?php print $node->nid; ?>">
...
</div>

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?

sun’s picture

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

webchick’s picture

I 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?

effulgentsia’s picture

Issue tags: +API addition
no such code exercising these alters exists. And if it did, we'd be considering it for core inclusion. Or am I mistaken?

yhahn's #6 patch includes this hunk:

+++ includes/common.inc	29 Oct 2010 14:44:57 -0000
@@ -3667,17 +3665,17 @@ function drupal_html_id($id) {
-  // Ensure IDs are unique by appending a counter after the first occurrence.
-  // The counter needs to be appended with a delimiter that does not exist in
-  // the base ID. Requiring a unique delimiter helps ensure that we really do
-  // return unique IDs and also helps us re-create the $seen_ids array during
-  // AJAX requests.
-  if (isset($seen_ids[$id])) {
-    $id = $id . '--' . ++$seen_ids[$id];
-  }
-  else {
-    $seen_ids[$id] = 1;
+  // Ensure IDs are unique by appending a random short hash. This provides a
+  // reliable method of ensuring uniqueness for AJAX requests where there is not
+  // necessarily a way to compare id's from the original page delivery with the
+  // AJAX response.
+  if (isset($seen_ids[$id]) || isset($_REQUEST['ajax_page_state'])) {
+    while (!isset($hash) || isset($seen_ids[$id . '-'. $hash])) {
+      $hash = substr(md5(uniqid(mt_rand())), 0, 8);
+    }
+    $id = $id .'-'. $hash;
   }
+  $seen_ids[$id] = 1;

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():

    // Ideally, Drupal would provide an API to persist state information about
    // prior page requests in the database, and we'd be able to add this
    // function's $seen_ids static variable to that state information in order
    // to have it properly initialized for this page request. However, no such
    // page state API exists, so instead, ajax.js adds all of the in-use HTML
    // ids to the POST data of AJAX submissions.

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:

  1. A server-side page state implementation that would require no altering of drupal_html_id(), but that exists in theory only, and might not be workable in practice for busy sites.
  2. A client-provided salt approach that would require altering drupal_html_id().
  3. A randomization approach that would require altering drupal_html_id(), but in a different way than the client-provided salt approach.

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.

rfay’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs backport to D7

Has to go into 8.x first.

sun’s picture

Status: Needs review » Needs work
Issue tags: +Performance, +Ajax, +API addition, +Needs backport to D7

The last submitted patch, ajax-html-id-956186-21.patch, failed testing.

Steven Jones’s picture

I'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.

nod_’s picture

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.

sun’s picture

Issue tags: -Needs backport to D7

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

nod_’s picture

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

nod_’s picture

Status: Needs work » Needs review
FileSize
3.78 KB

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.

Status: Needs review » Needs work

The last submitted patch, core-remove-files-from-ajax-requests-956186-35.patch, failed testing.

Steven Jones’s picture

@nod_ https://drupal.org/node/561858#comment-3404404 was the comment that nicely summarises why the current approach is in core.

nod_’s picture

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?

Steven Jones’s picture

@nod_ I don't see whyt we need to run away to another issue, let's just stay here!

Patches to follow...

davidseth’s picture

Hello @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!

Steven Jones’s picture

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

bibo’s picture

This 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?

andypost’s picture

Also 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

andypost’s picture

Filed 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

scottatdrake’s picture

This 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?

I'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.

scottatdrake’s picture

EDIT (thought comments were threaded)

sun’s picture

Title: Ensure it is possible to use AJAX with GET requests » [meta] Allow to use AJAX with GET requests
Category: task » feature
Status: Needs work » Active

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

sokrplare’s picture

Working 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:

  • Drupal 7.x - 687ade1998e7b6ab6d15d670eb0a52b98b9a1ae5
  • Views 7.x-3.x - 0e5b4f822e754fc004f2d2ad6fc37302733a8257
  • Ctools 7.x-1.x - 1851741bbc16e3978b064992225f1026a43c2b54

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.

  • token - unique per user and request
  • css - unique per user, different on the first request, but then the same on every following request
  • js - unique per user, same across all requests

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!

sokrplare’s picture

Update: 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:

  1. It looks like Varnish can be configured to exclude some URL parameters, so the changing token, css, and js values I mentioned in my last comment can likely be excluded on the Varnish side
  2. If not (we've only read it can be done, not done it yet), you can comment out those three at the end of 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.

j0rd’s picture

I'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:

;suhosin.get.max_array_depth = 50
;suhosin.get.max_array_index_length = 64
;suhosin.get.max_name_length = 64
;suhosin.get.max_totalname_length = 256
;; max length for get param value
;suhosin.get.max_value_length = 512
;; maximum amount of get vars that can be passed to /system/ajax
;suhosin.get.max_vars = 100
;suhosin.get.disallow_nul = on

Post defaults on debian lenny:

;suhosin.post.max_array_depth = 100
;suhosin.post.max_array_index_length = 64
;suhosin.post.max_name_length = 64
;suhosin.post.max_totalname_length = 256
;suhosin.post.max_value_length = 1000000
;suhosin.post.max_vars = 1000
;suhosin.post.disallow_nul = on

PHP after 5.3.9 also has this variable defaulted to 1000.

max_input_vars = 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+).

jamix’s picture

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

jamix’s picture

As 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 with ajax_html_ids at all.

Hopefully that code can contribute to the discussion.

Steven Jones’s picture

Issue summary: View changes

Add #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.

Steven Jones’s picture

catch’s picture

Category: Feature request » Task
Leon Kessler’s picture

I'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.

nod_’s picture

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.

jsst queued 6: 956186.6.patch for re-testing.

The last submitted patch, 6: 956186.6.patch, failed testing.

effulgentsia’s picture

Wim Leers’s picture

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

Wim Leers’s picture

geerlingguy’s picture

Can 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+.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Title: [meta] Allow to use AJAX with GET requests » Allow to use AJAX with GET requests
Status: Active » Needs review
FileSize
1.04 KB

Here'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.

dawehner’s picture

I was wondering whether we could gzip the page state. Maybe this would help us for big ajax page states.

Status: Needs review » Needs work

The last submitted patch, 65: ajax-956186-65.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
2.94 KB

Slightly more patch.

gzipping the page state is interesting.

catch’s picture

That's going to crash and burn.

The last submitted patch, 68: ajax-956186-65.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 69: ajax-956186-65.patch, failed testing.

dawehner’s picture

That's going to crash and burn.

Seriously catch, 14 failures. You disappoint these days!

Don't we need some code in ajax.js to also use query parameters?

catch’s picture

Status: Needs work » Needs review
FileSize
3 KB
catch’s picture

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

dpolant’s picture

I'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.

Status: Needs review » Needs work

The last submitted patch, 75: ajax-956186-65_2.patch, failed testing.

The last submitted patch, 75: ajax-956186-65_1_8-1-8.patch, failed testing.

The last submitted patch, 75: ajax-956186-65_2.patch, failed testing.

dpolant’s picture

Those 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).

catch’s picture

Issue summary: View changes
dawehner’s picture

Issue tags: +Needs tests

It 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?

dpolant’s picture

If you apply the patch from https://www.drupal.org/node/2500313 you can switch views to use GET.

catch’s picture

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dpolant’s picture

Here's another version of the 2.x patch with another $ajax_page_state call changed.

Status: Needs review » Needs work

The last submitted patch, 85: ajax-956186-65_2x_1.patch, failed testing.

slasher13’s picture

Status: Needs work » Needs review

unrelated fail -> retested

The last submitted patch, 49: 956186.1.against_D7.16.b.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pjcdawkins’s picture

Title: Allow to use AJAX with GET requests » Allow AJAX to use GET requests

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

Wim Leers’s picture

@pjcdawkins That's a better name indeed, thanks!

braindrift’s picture

Status: Needs review » Needs work

Hi @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".

Pere Orga’s picture

Hi @braindrift

If a library is added during an ajax request, the ajaxPageState.libraries contains duplicate values.

I understand that this is an existing bug, not specific to adding support for GET in AJAX. Please can you create a separate issue?

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

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)

braindrift’s picture

Status: Needs work » Needs review

Created a separate issue.

dpolant’s picture

dpolant’s picture

Confirmed 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?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Denis Danielyan’s picture

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

dawehner’s picture

I really think this needs tests, which would have caught that problem @Denis Danielyan just described.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

anavarre’s picture

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kaythay’s picture

A reroll. Still needs tests.

Status: Needs review » Needs work

The last submitted patch, 103: ajax-956186_103.patch, failed testing. View results

kaythay’s picture

FileSize
3.76 KB

Sorry, had been working against 8.6.x instead of 8.7.x

kaythay’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
@@ -128,7 +128,8 @@ public function processAttachments(AttachmentsInterface $response) {
-    $ajax_page_state = $request->request->get('ajax_page_state');
+    $ajax_page_state = $request->query->get('ajax_page_state') ?: $request->request->get('ajax_page_state');

This (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 default

cchoe1’s picture

Am 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

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

geek-merlin’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record
--- a/core/misc/ajax.js
+++ b/core/misc/ajax.js
@@ -241,7 +241,8 @@ function _toConsumableArray(arr) { if (Array.isArray(arr)) { for (var i = 0, arr
       },
 
       dataType: 'json',
-      type: 'POST'
+      type: typeof ajax.type !== 'undefined' ? ajax.type : 'POST'
+
     };
 

#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!).

geek-merlin’s picture

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

super_romeo’s picture

+ 1 for Needs review :)

SteffenR’s picture

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

agiraud’s picture

Is there any way to make the ajax requests cacheable when using .use-ajax ?
Why is it not possible to add type for this case ?

owenpm3’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Needs work

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

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

SteffenR’s picture

Attached an updated patch against latest 8.9.x.

super_romeo’s picture

Thank you for reroll!

SteffenR’s picture

FileSize
1.36 KB

I forgot to add the interdiff - let's do this with this comment.

sanket1007’s picture

Please ignore this patch. I confirm that #121 works for me.

sanket1007’s picture

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

mr.york’s picture

Status: Needs work » Needs review
FileSize
4.19 KB

Reroll #121 patch.

raman.b’s picture

olivier.br’s picture

Issue summary: View changes
nod_’s picture

Issue tags: +JavaScript

consolidating issues under the JavaScript tag

mr.york’s picture

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Lukas von Blarer’s picture

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

Lukas von Blarer’s picture

Status: Needs review » Needs work
Lukas von Blarer’s picture

For other people having this issue with exposed filters: views_ajax_get solves this for me.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

melvinlouwerse’s picture

rwaery.11’s picture

Updated patch to correctly get dialogOptions from query parameter when set.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Supreetam09’s picture

Status: Needs work » Needs review
FileSize
5.43 KB
345 bytes

Adding patch to fix custom commands failure.

catch’s picture

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

catch’s picture

This starts adding #type support to #ajax - current patch only allows it for links.

catch’s picture

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

catch’s picture

Issue summary: View changes

OK 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).

catch’s picture

Version: 9.5.x-dev » 10.1.x-dev
FileSize
6.27 KB
7.92 KB

Interdiff is against #142 since that makes the most sense for reviewing changes since the re-roll.

catch’s picture

Unused use statements.

Status: Needs review » Needs work

The last submitted patch, 147: 956186-147.patch, failed testing. View results

catch’s picture

Opened #3303646: Convert Views to use GET for AJAX for actually implementing this in Views.

b_sharpe’s picture

To 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

catch’s picture

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

catch’s picture

Status: Needs work » Needs review
FileSize
10.65 KB

Re-roll + a couple of test changes.

Should be just down one Standard/BigPipe test failure now, haven't looked into that yet.

Status: Needs review » Needs work

The last submitted patch, 152: 956186.patch, failed testing. View results

catch’s picture

Status: Needs work » Needs review
FileSize
11.73 KB

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

catch’s picture

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

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

nod_’s picture

Haven't found why yet but changing the /admin/content view to use ajax we get a little styling problem with this patch

Lots of classes don't get added to the html sent back from the GET request

Wim Leers’s picture

+++ b/core/modules/big_pipe/src/Render/BigPipe.php
@@ -465,7 +465,7 @@ protected function sendNoJsPlaceholders($html, $no_js_placeholders, AttachedAsse
-      $fake_request->request->set('ajax_page_state', ['libraries' => implode(',', $cumulative_assets->getAlreadyLoadedLibraries())]);
+      $fake_request->query->set('ajax_page_state', ['libraries' => implode(',', $cumulative_assets->getAlreadyLoadedLibraries())]);

@@ -575,7 +575,7 @@ protected function sendPlaceholders(array $placeholders, array $placeholder_orde
-      $fake_request->request->set('ajax_page_state', ['libraries' => implode(',', $cumulative_assets->getAlreadyLoadedLibraries())] + $cumulative_assets->getSettings()['ajaxPageState']);
+      $fake_request->query->set('ajax_page_state', ['libraries' => implode(',', $cumulative_assets->getAlreadyLoadedLibraries())] + $cumulative_assets->getSettings()['ajaxPageState']);

BigPipe changes: LGTM!

nod_’s picture

Theme is claro in the GET request, In the response the settings are updated to have ajaxPageState.theme = "umami"

catch’s picture

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

catch’s picture

Status: Needs review » Needs work

The last submitted patch, 160: 956186-160.patch, failed testing. View results

catch’s picture

Status: Needs work » Needs review
FileSize
17.22 KB

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

Status: Needs review » Needs work

The last submitted patch, 162: 956186-162.patch, failed testing. View results

catch’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
17.39 KB

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

Status: Needs review » Needs work

The last submitted patch, 164: 956186-164.patch, failed testing. View results

catch’s picture

Status: Needs work » Needs review
FileSize
5.81 KB
16.37 KB

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

catch’s picture

Given 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'].

szloredan’s picture

catch’s picture

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

catch’s picture

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

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
144 bytes

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

catch’s picture

Status: Needs work » Needs review
FileSize
16.37 KB

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

andypost’s picture

sker101’s picture

the patch no longer applys on latest of 10.0.x, rerolling.

Status: Needs review » Needs work

The last submitted patch, 174: 956186-174.patch, failed testing. View results

Arlina’s picture

FileSize
14.22 KB

Here's a back-port patch that applies to Drupal 9.5.2, if it's useful to anyone: 956186-175-for-D9.patch

lauriii’s picture

Status: Needs work » Needs review
Issue tags: -JavaScript +JavaScript
FileSize
16.36 KB

Patch no longer applies

smustgrave’s picture

Per @catch in #167 should #3303646: Convert Views to use GET for AJAX be included here?

catch’s picture

Issue summary: View changes

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

catch’s picture

Issue summary: View changes

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

Lendude’s picture

As @catch pointed out in #179:

since Views is more or less our only non-form use of AJAX in core

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?

catch’s picture

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

nod_’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

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

anairamzap’s picture

Hello, 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

catch’s picture

anairamzap’s picture

I'm correcting the patch provided in #184 since when I did the re-roll I forgot to:

  • Add the javascript changes to the *es6.js files
  • Build js with yarn build:js
  • Commit the changes after the build

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.

catch’s picture

lauriii’s picture

Another reroll based on #177.

lauriii’s picture

I was reviewing the code and I was wondering if we should be using the method property from jQuery.ajax() instead because type 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 the method 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.

  • lauriii committed 29763eaa on 10.1.x
    Issue #956186 by catch, dpolant, SteffenR, yhahn, lauriii, nod_,...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed 29763ea and pushed to 10.1.x. Thank you for everyone who worked on this over the past 13 years!

lauriii’s picture

Filed an issue in the Content-Security-Policy module queue: #3352875: Add support for GET Ajax requests.

catch’s picture

Discussed #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.

Status: Fixed » Closed (fixed)

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

claudiu.cristea’s picture

When 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