Do not make entries in "cache_form" when viewing forms that use #ajax['callback'] (Drupal 7 port)

Nice! Would it be possible to post interdiffs, too?

Thank you very much for the detailed explanation of the changes.

That is very helpful. I hope to be able to provide a review within the next few days, but I you don't get an answer by Monday next week, please ping this issue again.

Also even though I added the 'Needs framework manager review' tag, the issue could get independently to RTBC, too. No need to wait for me - if there are others that can review this issue.

--

In general:

The new approach saves one DB write (yeah!), however will make ajax requests quite a bit slower usually - depending on how much needs to be built to reach the form.

It is really nice that you can easily get the old behavior back by using a path of system/ajax however, so you can opt-in again to the old behavior.

I am not yet sure if we can make the behavior the new default (opt-out) or instead allow a mechanism to opt-in to the new one, that is something we will discuss in our core committers group.

It mostly depends if we can construct a case, where the new behavior would fail, while the old would work.

--

We will definitely need some legacy tests - in any case.

--

Last but not least:

Thank you so much for working on this!

This fixes probably the biggest D7 problem when it comes to ecommerce sites, we've had clients with 100G cache_form tables cause of product nodes that use #ajax.

I'm eager to see if this fixes the long lasting issue. So far I've been using a module made by bmunslow in this ticket https://www.drupal.org/node/1694574 as a temporary solution. It works pretty well but still a bit hackish.

#12 / #13: It would certainly help for this issue to get in earlier, if community members would heavily test this on (their) real sites and report back any breakage and successes.

That said we could also consider to make this opt-in via variable for 7.5x and the default in 7.60 (as it fixes a big bug).

@bojanz:

This fixes probably the biggest D7 problem when it comes to ecommerce sites, we've had clients with 100G cache_form tables cause of product nodes that use #ajax.

That's actually what led me to find this issue. Having trouble right now with caching and ajax requests (adding to cart and switching products via attribute select lists). Our cache_form is huge. Is it safe to truncate? Could that be the cause of our issues? We're getting loads of "Invalid form POST data".

A little off topic, sorry for that..

#15: Yes, it is safe to truncate, however active users will loose their form data, so only do that during a deployment period at a low-traffic time.

And yes, #15 this patch should resolve those issues, so if you would be able to test it (even if just on staging for now), that would be great!

#16: Sounds good! Ok, so my problem then is that It's hard for me to manually reproduce the problem. Do you have a fool proof way of achieving that?

BTW, truncating the table seems to have fixed the issue - but I'm not 100% sure due to lack of ability to reproduce the issue easily. I thought that Entity Cache was behind my problem (see #2820670: Problem with AJAX requests (Commerce)), because I recall having this exact problem the last time I tried out that module for increased performance, and now it happened again. We've been running this e-commerce site since 2011 and haven't had this problem before other than that time. The only thing we changed this week was installing Entity Cache (along with the Commerce Entity Cache module) and moving from 5 min to 3 hrs of Minimum cache lifetime. One of those things must've been the cause, or at least in part, to this happening.

UPDATE: The problems I've had are most likely related to #2475503: Ajax add to cart works only for initial user, so my comments here may not be applicable.

#17: This patch could resolve this problem as well as normally no form cache is created when viewing.

#18 (btw, what's the syntax for comment references?): I see, that does sound nice. It would probably reduce the load of the system cron, which currently takes a loooong time to perform. What are the possible side effects?

#19: The possible side effects of this patch are:

- Performance can be slightly reduced, because system/ajax is pretty fast. However it will never be slower than performance for a user that has JS disabled or a form that has JS disabled.

- In theory it could be possible that a form that only works with AJAX (and never with a normal POST), could be unreachable on the page. Though in practice that should not happen.

Patch in general is pretty safe.

Applied the patch now, will report back with my findings

Ok, one problem found (testing patch from #8):

Some views with exposed filters (those that rely on cookies I think..) are empty. I can get the results by simply filtering the view (even with the same as the default values). I'm also still getting a few "Invalid form POST data." log entries from time to time in production.

UPDATE: The list of all views (admin/structure/views) as well as Contexts (admin/structure/context) are empty after applying the patch.

#22 Interesting, could you elaborate a little more about your setup and the setup of those views?

Tried #25 and it fixed those issues for me

Update: Plupload module, whether relevant or not, is not working properly after applying the patch. Can't upload or remove files (images in this case) via ajax on Plupload-enabled filefields.

Update 2: Another thing that is a bit troublesome is that it appears as if Ajax errors of this kind are no longer logged, so I'm not actually sure that the problem has been solved in my case.

Overall patch looks good to me.

I feel the process_input + form_count is short-cutting the form system and hence leads to compatibility problems.

My understanding of the wanted flow is:

- form_build()
- form_execute => intercept if ajax and return and exit

e.g. only if a form would be executed we want to go into the ajax mode. Then form builder should automatically find the right form on the page, etc.

So TL;DR: Lets ensure to keep changes to a minimum.

It might also be likely good to add a quick dedicated test case to test a simple ajax upload with the same form twice on the page (with different arguments), so that we have a test case to which we can test simplified patches against.

Great work overall!

Here is the detailed review:

1. +++ b/includes/ajax.inc
   @@ -336,17 +336,6 @@ function ajax_get_form() {
   -  // When a page level cache is enabled, the form-build id might have been
   -  // replaced from within form_get_cache. If this is the case, it is also
   -  // necessary to update it in the browser by issuing an appropriate Ajax
   -  // command.
   -  $commands = array();
   -  if (isset($form['#build_id_old']) && $form['#build_id_old'] != $form['#build_id']) {
   -    // If the form build ID has changed, issue an Ajax command to update it.
   -    $commands[] = ajax_command_update_build_id($form);
   -    $form_build_id = $form['#build_id'];
   -  }
   -
   
   @@ -361,7 +350,7 @@ function ajax_get_form() {
   -  return array($form, $form_state, $form_id, $form_build_id, $commands);
   +  return array($form, $form_state, $form_id, $form_build_id);
   
   @@ -382,9 +371,18 @@ function ajax_get_form() {
   -  list($form, $form_state, $form_id, $form_build_id, $commands) = ajax_get_form();
   +  list($form, $form_state, $form_id, $form_build_id) = ajax_get_form();
   

   Do we need to remove this?

   Other code could already be calling ajax_get_form() for their custom ajax path / callback.

2. +++ b/includes/ajax.inc
   @@ -382,9 +371,18 @@ function ajax_get_form() {
   +  ajax_form_return_commands($form, $form_state);
   ...
   +function ajax_form_return_commands($form, $form_state) {
   

   Could we pass initial commands to this to keep BC?

3. +++ b/includes/ajax.inc
   @@ -397,15 +395,23 @@ function ajax_form_callback() {
   -    if (!(is_array($result) && isset($result['#type']) && $result['#type'] == 'ajax')) {
   -      // Turn the response into a #type=ajax array if it isn't one already.
   -      $result = array(
   -        '#type' => 'ajax',
   -        '#commands' => ajax_prepare_response($result),
   -      );
   +    // Pull out the commands if provided directly.
   +    if (is_array($result) && isset($result['#type']) && $result['#type'] === 'ajax') {
   +      $commands = $result['#commands'];
   +    }
   +    // Turn the response into a #type=ajax array if it isn't one already.
   +    else {
   +      $commands = ajax_prepare_response($result);
   +    }
   

   Is there a logical change in here?

   Possibly we could leave that code alone if we pass in $commands?

4. +++ b/includes/ajax.inc
   @@ -397,15 +395,23 @@ function ajax_form_callback() {
   +    if (isset($form['#build_id_old']) && $form['#build_id_old'] !== $form['#build_id']) {
   +      $commands[] = ajax_command_update_build_id($form);
        }
   

   I would prefer a helper function ajax_get_form_full_page or something like that, which passes the $commands in and does the build_id_old check.

5. +++ b/includes/ajax.inc
   @@ -642,7 +648,10 @@ function ajax_footer() {
   +  // Custom callback paths set in $element['#ajax']['path'] are assumed to
   +  // require form caching. Using $element['#ajax']['callback'] on the other hand
   +  // does not require the form to be cached, and is the preferred method.
   +  if (!empty($element['#ajax_processed']) && !isset($element['#ajax']['callback'])) {
        $form_state['cache'] = TRUE;
      }
   

   What happens in 'callback' and 'path' are both set?

   Should the check not be on 'path'?

6. +++ b/includes/ajax.inc
   @@ -740,7 +749,7 @@ function ajax_pre_render_element($element) {
   -      'path' => 'system/ajax',
   +      'path' => $_GET['q'],
   

   This is likely where we would need the variable_get().

7. +++ b/includes/form.inc
   @@ -121,14 +121,37 @@
   +  $form_counts = &drupal_static(__FUNCTION__, array());
   +  $form_counts[$form_id] = isset($form_counts[$form_id]) ? (string) (intval($form_counts[$form_id]) + 1) : '1';
   

   Caching will make problems with that, e.g. consider a form that is cached in a block or entity.

8. +++ b/includes/form.inc
   @@ -121,14 +121,37 @@
   +  $form_state['build_info']['build_count'] = $form_counts[$form_id];
   

   Changing the build_info here feels wrong to me.

   Let me try to understand:

   The problem is that we need to identify the right form if the same form_id exists several times on the page?

   Why is the form_build_id not sufficient for that?

   How does that work in core without ajax at all?

   It should not be necessary at all.

   If that is a bug in Core, we will need to fix that generically first.

9. +++ b/includes/form.inc
   @@ -121,14 +121,37 @@
   +  $ajax_post = isset($_POST['ajax_page_state']) && $_POST['form_id'] === $form_id;
   

   Is that how we detect ajax requests elsewhere in the code base?

   I had something else in my mind for that, but could be wrong.

10. +++ b/includes/form.inc
    @@ -121,14 +121,37 @@
    +  if ($ajax_post && $form_state['process_input']) {
    +    $commands = ajax_form_return_commands($form, $form_state);
    

    If we call the helper here, which calls this function, then I think we can avoid a lot of the changes way above.

11. +++ b/includes/form.inc
    @@ -317,6 +340,11 @@ function drupal_build_form($form_id, &$form_state) {
    +    // If the form is built multiple times on the same page, only apply the
    +    // input to the matching build count.
    +    if (isset($form_state['input']['form_build_count']) && $form_state['input']['form_build_count'] != $form_state['build_info']['build_count']) {
    +      $form_state['input'] = array();
    +    }
    

    Should the form_build_id not be unique?

    Maybe we can do a different check (e.g. what form_execute() checks to detect if we should execute this form.)

12. +++ b/includes/form.inc
    @@ -409,10 +437,13 @@ function form_state_defaults() {
    +      'from_cache' => FALSE,
    +      'build_count' => 1,
    ...
    +    'process_input' => FALSE,
    

    Should not be necessary.

13. +++ b/includes/form.inc
    @@ -464,6 +495,15 @@ function form_state_defaults() {
    +  // If this form was not built from cache but needs to maintain the same build
    +  // ID, copy the value from input onto the old form.
    +  if (empty($form_state['build_info']['from_cache']) && isset($form_state['input']['form_build_id'])) {
    +    $old_form['#build_id'] = $form_state['input']['form_build_id'];
    +    $copy_build_id = FALSE;
    +  }
    
    @@ -471,7 +511,7 @@ function drupal_rebuild_form($form_id, &$form_state, $old_form = NULL) {
    -  $enforce_old_build_id = isset($old_form['#build_id']) && !empty($form_state['rebuild_info']['copy']['#build_id']);
    +  $enforce_old_build_id = isset($old_form['#build_id']) && $copy_build_id;
    

    Could you elaborate on those changes?

14. +++ b/includes/form.inc
    @@ -966,7 +1010,7 @@ function drupal_process_form($form_id, &$form, &$form_state) {
    -    if (($form_state['rebuild'] || !$form_state['executed']) && !form_get_errors()) {
    +    if ($form_state['process_input'] && ($form_state['rebuild'] || !$form_state['executed']) && !form_get_errors()) {
    

    I think we should integrate with the forms system more.

15. +++ b/includes/form.inc
    @@ -1033,6 +1077,16 @@ function drupal_prepare_form($form_id, &$form, &$form_state) {
    +  $form['form_build_count'] = array(
    +    '#type' => 'hidden',
    +    '#value' => $form_state['build_info']['build_count'],
    +    '#name' => 'form_build_count',
    +    '#parents' => array('form_build_count'),
    +  );
    

    As written before:

    Form Build ID should be sufficient for that.

#30: I am 100% +1 to the plan proposed and it is perfectly fine to tackle this in stages. As we get nearer to something commitable, we will decide if we want to make this the new default or opt-in until at least the 2-forms issue is fixed.

Tried the patch from #37 but uploads via Plupload are still causing problems, got an ajax error when uploading to a filefield:

TYPE: page not found
LOCATION: http://example.dev/file/ajax/field_product_image/und/form-2I92pFjLj6yZIH7Pd5EKn6avRORcc37pcnMJoOQd3FA 
REFERRER: http://example.dev/node/34033/edit?destination=admin/content
MESSAGE: file/ajax/field_product_image/und/form-2I92pFjLj6yZIH7Pd5EKn6avRORcc37pcnMJoOQd3FA

#37:

Why do we need the copy build ID for ajax, but not for the normal form submission?

And why do we need to distinguish between cached forms and fresh ones?

I have applied the #42 patch and have been testing it on a development server replicating my live environment using a reverse proxy.

Everything has worked as I desired in eliminating the annoying 'Invalid POST data' error in watchdog and ajax alert response after the typical time frame (6 hours) that a form expires from cache.

The only issue I suffer (so far) after a week of testing is an issue with the file module (file.module). The patch adds a function 'file_managed_file_ajax()'. When this callback is fired, for example, when the 'Remove' button is pressed in an attached file field, the wrapper ($button['#ajax']['wrapper']) is referencing an incremented css selector ID that doesn't yet exist.

1) Initial page load '

One more critical problem after applying the patch: forms do not work on 403 pages any more. Imagine an ajaxified login form on all pages. If you try to use it from a page you are not allowed to visit, AJAX operation will fail with 403 also.

Upgrading priority as this would keep people's form caches from blowing up

FWIW, i use a variation of this patch now in production for a client.

I will update this issue with my changes soon:

Basically the BC layer fails right now, but overall it works great.

My changes are basically (I lost the interdiff):

@@ -642,7 +651,10 @@ function ajax_footer() {
  */
 function ajax_process_form($element, &$form_state) {
   $element = ajax_pre_render_element($element);
-  if (!empty($element['#ajax_processed'])) {
+  // Custom callback paths set in $element['#ajax']['path'] are assumed to
+  // require form caching. Using $element['#ajax']['callback'] on the other hand
+  // does not require the form to be cached, and is the preferred method.
+  if (!empty($element['#ajax_processed']) && !isset($element['#ajax_no_form_cache'])) {
     $form_state['cache'] = TRUE;
   }
   return $element;

I used a new form key here to control that.

I might think of putting that in $element['#ajax']['no_form_cache'] instead, so the caller can easier put that as an option, too.

@@ -738,10 +750,24 @@ function ajax_pre_render_element($element) {
 
     $settings = $element['#ajax'];
 
+    $path = 'system/ajax';
+    $options = array();
+
+    if (isset($element['#ajax']['callback']) && !isset($element['#ajax']['path'])) {
+      $query_parameters = drupal_get_query_parameters($_GET, array('q'));
+      $query_parameters['_drupal_ajax'] = 1;
+
+      $path = $_GET['q'];
+      $options['query'] = $query_parameters;
+
+      // This does not need to be cached in the form cache.
+      $element['#ajax_no_form_cache'] = TRUE;
+    }
+
     // Assign default settings.
     $settings += array(
-      'path' => 'system/ajax',
-      'options' => array(),
+      'path' => $path,
+      'options' => $options,
     );
 
     // @todo Legacy support. Remove in Drupal 8.

I changed the BC layer to change the path by both checking a callback is available and no path being set already, then set the new property, which felt safer.

I also used _drupal_ajax=1 as in Drupal 8 to distinguish legacy ajax requests from new ones.

I think for 403 pages, we need to ensure that system/ajax is used, but we should know the page callback result already at the stage of the form building.

diff --git a/includes/form.inc b/includes/form.inc
index 3e727f9..e0decdd 100644
--- a/includes/form.inc
+++ b/includes/form.inc
@@ -127,7 +127,30 @@ function drupal_get_form($form_id) {
   array_shift($args);
   $form_state['build_info']['args'] = $args;
 
-  return drupal_build_form($form_id, $form_state);
+  // Disable redirection if the request was made via AJAX.
+  $ajax_post = isset($_GET['_drupal_ajax']) && !empty($_POST['ajax_page_state']) && $_POST['form_id'] === $form_id;
+  if ($ajax_post) {
+    $form_state['no_redirect'] = TRUE;
+    $form_state['rebuild_info']['copy']['#build_id'] = TRUE;
+    $form_state['rebuild_info']['copy']['#action'] = TRUE;
+  }
+
+  $form = drupal_build_form($form_id, $form_state);
+

I changed the code to check both _drupal_ajax and ajax_page_state as _drupal_ajax did leak into e.g. POST urls and made things problematic.

I think _drupal_ajax should be removed from _GET as early as possible (drupal_initialize_settings or such) and be moved in either a global variable or static state holding function instead, so that it will never leak out, where it does not belong.

I removed the file parts from this for now as for the client it was not needed and instead would put that to a follow-up as there is quite some risk involved as files are crazy complicated.

In my case it was a commerce site, where product color / variation switching was affected as cache_form eventually timed out while Varnish was still happily serving the old site due to 304 not modified mechanisms.

--

Another thing is that if this is called from the theming layer (e.g. drupal_render_page() already parts are output, which are then pre-pended to the ajax response. We need to somehow clear the output buffer before doing so and this also needs a test. D8 is not affected as they use the Response() class for encapsulating that.

--

I also had an idea how to ensure in custom code that those requests are as fast as possible:

Just build the relevant forms as early as possible, e.g. in hook_init() in the page request, then re-use the statically cached form building result.

That should work well and give similar performance to system/ajax.

@Fabianx: We just switched to PHP7 and as a result can't use Memcache yet (Acquia is waiting for it to stabilize). As a result, our {cache_form} table has grown exponentially and has threatened to bring the site to a halt.

Would you be able to post a re-roll of the patch with your changes? (Even splitting up the file handling stuff would be fine.) I would be happy to test it on our fairly high-traffic site.

Thanks!

Hello, sorry to barge in such high profile drupalers conversation.
I managed to apply the patch #42, hoping to show Ubercart price update when selecting different attributes from cached page.
<select id="edit-attributes-27" class="form-select ajax-processed" name="attributes[27]"> becomes
<select id="edit-attributes-27" class="form-select ajax-processed progress-disabled" name="attributes[27]" disabled=""> when click option.

After #42 i also changed code as#48. same behavior.

I can still add to cart but default options are chosen in cart page.

Subscribing.
Applied patch #42 because we have a registration form that has email validation via AJAX + a file upload field that was erroring.
With the patch in #42, my mediabrowser fails.

I so much want/need this patch.

I will try to follow it up properly.

I quickly tried it and I had issues on a site that's also Ubercart as #50 and an ajaxified cart with https://www.drupal.org/project/uc_ajax_cart_alt and I spot immediate issues.

Should the patch as it is affect contrib modules?

It affected mine but I am doing some odd hacks tricking the FAPI so that might be it as well.

Hello, i have no idea what you guys are talking about, But i can do the test...

With fresh Drupal core with Ubercart (+ Dependencies) only, i applied the patch #42. When changing the attribute,'Please wait' shows (form1). Then i click 'Add to cart' button, screen briefly shows my selections (form2). In cart page, default options are chosen (form3).

If it helps, I tried reproducing this as well, at least the initial bits as I have a good amount of ubercart experience.

The settings that ajaxify the attributes is in /admin/store/settings/products.

See screenshots and dump.

I, however, see a different error about maximum nested limit reached. I do have xdebug.max_nesting_level set, but I tried 256,500 and 1000 and all hit the limit.

Let me know if something else could be helpful.

Thanks!

We implemented patch #42 to address some odd behavior with custom, AJAX-based functionality. The root cause of the misbehavior was the form/form state being cached during the first of two AJAX requests. While the patch resolved the original issue, it also broke a number of other, more standard AJAX calls, mostly having to do with uploading files or partial form state.

After some correspondence with quicksketch, he pointed me at the check for AJAX data points in drupal_get_form(). My traces showed drupal_get_form() was never executed... ctools routed the broken AJAX call through page_manager_* functionality, eventually ending up at drupal_build_form(). Since *get_form is simply a wrapper around "make a new, blank form state and call drupal_build_form()", I tried moving the check for AJAX state into that function instead. That appears to have resolved the issues we were encountering.

I've attached an inter-diff between my final version and patch #42. These changes should help with some breakages caused by ctools, panels, and other contrib modules which manipulate the routing themselves. It simply moves tests for AJAX into drupal_build_form().

Is it possible to hook into the ajax form mechanism to disable its cache without applying the patch?

@quicksketch

once you have a cms content with body of "PHP code" format lets say for example:

Test output before form rendering here:

<?php
function demo_demo_form($form, &$form_state) {
  return array(
    'email' => array(
      '#type' => 'textfield',
      '#title' => 'demo'
    ),
    'submit' => array(
      '#type' => 'submit',
      '#value' => 'subscribe',
      '#ajax' => array('callback' => 'demo_form_ajax_submit'),
    ),
  );
}
function demo_form_ajax_submit($form, $form_state) {
  return 'Subscribed !';
}
echo drupal_render(drupal_get_form('demo_demo_form'));

if you will submit it and it's ajax submit (POST to self), in ajax response instead of valid json you will get "Test output before form rendering here:" prepended in http response. error will be displayed

to fix this, we can do this:

diff --git a/includes/form.inc b/includes/form.inc
index 32990c5..c184d5e 100644
--- a/includes/form.inc
+++ b/includes/form.inc
@@ -147,6 +147,12 @@ function drupal_get_form($form_id) {
       $commands[] = ajax_command_update_build_id($form);
     }
     $commands = ajax_form_return_commands($form, $form_state, $commands);
+
+    $status = ob_get_status();
+    if (!empty($status)) {
+      ob_clean();
+    }
+
     ajax_deliver($commands);
     exit();
   }

just before ajax deliver response, cleanup output buffer if there is one

we currently testing #42 patch with diff above in our env

Re-rolled the combined patch of #42 and #58, wasn't clear if #61 was needed or not so I didn't include it, also untested.

Uhm,

+ ajax_form_return_commands($form, $form_state, $commands);

should be:

+ return ajax_form_return_commands($form, $form_state, $commands);

Then this does not break system/ajax paths anymore ... lol

I've written some more code based on this patch and here are my core committer thoughts on the state of it:

- 1. system/ajax must not break (see last comment), but easy to fix. But this needs to have test coverage.

- 2. I like the changes to move this to form_builder instead of drupal_get_form(). That makes a lot of sense.

- 3. I think a first iteration should make this opt-in instead of opt-out. The ability to opt-in a form to not use system/ajax is pretty huge in itself already.

If we wanted to make it easier for contrib / custom to change the opt-in property we could add an drupal_alter() call before calling / processing ajax_pre_render_element().

- 4. I don't think the _drupal_ajax property is necessary in D7 (unlike in Drupal 8).

- 5. I think we should check $_POST['ajax_page_state'] instead of $form_state['input']['ajax_page_state'] as nothing else in core does so.

- 6. I still think the new property to inform ajax_process_form() is necessary. This is also useful for a contrib module to know if this is traditional system/path ajax or the new way. A better name is needed though. #ajax_use_callback, #ajax_via_normal_path or such. I don't like that we say that as soon as callback is set, we MUST use the new way and not cache the form. Similar to the opt-in concerns.

To explain that case a little more:

- Module sets both callback and ajax path to 'system/ajax' (opt-out of the patch). In the current patch, the form_state['cache'] is set to FALSE, so that won't work.

Unrelated, but I learned and it is helpful to document that somewhere:

- The reason the form-old-build-id dance is needed is because there can be stateful ajax callbacks (e.g. examples/ajax_example/dynamic_sections) and the form state is cached based on the new form ID.

- The reason state-changing ajax callbacks need the form cache at all is that most rely on the validated values, but if you rely on validated values without a form cache, then you don't have any values in the first run through the form.

And if you e.g. show a form element based on the value of another one (in PHP), then validation will fail this element if its not shown. There is several ways around that, but examples module examples fail that test if you kill the form state cache so I assume it will be common for custom / contrib to do the same.

Thanks,

Fabian

Edit:

We also need this chunk as else parameters won't be passed correctly:

+    $path = 'system/ajax';
+    $options = array();
+
+    if (isset($element['#ajax']['callback']) && !isset($element['#ajax']['path'])) {
+      $query_parameters = drupal_get_query_parameters($_GET, array('q'));
+
+      $path = $_GET['q'];
+      $options['query'] = $query_parameters;
+
+      // This does not need to be cached in the form cache.
+      $element['#ajax_via_normal_path'] = TRUE;
+    }

Edit 2:

And I absolutely agree that we should do the ob_flush() to avoid putting content out - if we are called from theme layer or such.

Thank you for all of the hard work in this issue. The patches do what this issue advertises very well, but there are side effects (for example, I couldn't save Views UI modal forms).

I came up with a workaround that anyone can use in a custom module to simply rebuild the form if no cached form can be found. I'll paste it here for anyone who needs a (custom) solution that works right now.

Note, this solution cannot be used in a generic way because rebuilding the form requires knowledge of the original form arguments. In my use case, I was adding some ajax stuff to a Webform.

Step 1: Define the 'path' to use for your element's '#ajax' property so that we skip 'system/ajax' entirely.

'#ajax' => array(
  'path' => "my-custom-module-ajax/$node_id/$submission_id",
  'callback' => 'my_custom_module_ajax_element_callback',
),

Step 2: Define the menu callback.

/**
 * Implements hook_menu().
 */
function my_custom_module_menu() {
  $items['my-custom-module-ajax/%webform_menu/%webform_menu_submission'] = array(
    'type' => MENU_CALLBACK,
    'load arguments' => array(1, 2),
    'access callback' => TRUE,
    'page callback' => 'my_custom_module_ajax_form_callback',
    'page arguments' => array(1, 2),
    'delivery callback' => 'ajax_deliver',
    'theme callback' => 'ajax_base_page_theme',
  );
  return $items;
}

Step 3: Create the custom ajax callback which is an alternative to ajax_form_callback(). The essential part here is that if the form cannot be retrieved from the form cache, we simply rebuild it and then continue on with the code from ajax_get_form() and ajax_form_callback().

/**
 * Ajax callback that rebuilds expired cache_form entries if necessary.
 */
function my_custom_module_ajax_form_callback($node, $submission) {
  $commands = array();

  // Get the form from the cache.
  // See ajax_get_form().
  $form_state = form_state_defaults();
  $form_build_id = $_POST['form_build_id'];
  $form = form_get_cache($form_build_id, $form_state);

  // Form wasn't found in cache, so let's rebuild it.
  // NOTE: Set your custom form arguments here...
  if (!$form) {
    $form_state['build_info']['args'] = array($node, $submission, TRUE);
    $form = drupal_retrieve_form($_POST['form_id'], $form_state);
    drupal_prepare_form($_POST['form_id'], $form, $form_state);
    $form['#build_id_old'] = $form_build_id;
  }

  // Other stuff from ajax_get_form().
  if (isset($form['#build_id_old']) && $form['#build_id_old'] != $form['#build_id']) {
    $commands[] = ajax_command_update_build_id($form);
  }
  $form_state['no_redirect'] = TRUE;
  $form_state['rebuild_info']['copy']['#build_id'] = TRUE;
  $form_state['rebuild_info']['copy']['#action'] = TRUE;
  $form_state['input'] = $_POST;

  // Finally, remaining stuff from ajax_form_callback().
  drupal_process_form($form['#form_id'], $form, $form_state);
  if (!empty($form_state['triggering_element'])) {
    $callback = $form_state['triggering_element']['#ajax']['callback'];
  }
  if (!empty($callback) && is_callable($callback)) {
    $result = $callback($form, $form_state);
    if (!(is_array($result) && isset($result['#type']) && $result['#type'] == 'ajax')) {
      $result = array(
        '#type' => 'ajax',
        '#commands' => ajax_prepare_response($result),
      );
    }
    $result['#commands'] = array_merge($commands, $result['#commands']);
    return $result;
  }
}

Enjoy!

I'm currently working on a re-roll with tweaks of this.

Hello all.

I found myself here looking for a solution to an AJAX from not updating after I updated a rather complex site from PHP 5.3 to PHP 7.

The problem is that when an AJAX callback was executed, the form fragment it needed was no longer in $form.

My problem seems unrelated to this issue, and certainly is based on the title, but I applied the patch in #63 and added some of the mentioned items in #66 (more on that below), and the form now works as it did before. Thanks for that! It was driving me crazy trying to figure it out.

However, in further testing, I found that the changes in #58 broke CTools modal dialogs. When I reverted to using #63 with the changes listed in #66 but without the modifications in #58, the modal forms started working again, and the original issue was still fixed.

I've attached two updated patches:

1. A re-roll of #63 with some of #66 added and
2. A re-roll of #63 with some of #66 added, but without the changes in #58.

As to the items I added from #66...

I included these items in the patches:

• 1.1. system/ajax must not break (see last comment), but easy to fix.
• 4. I don't think the _drupal_ajax property is necessary in D7 (unlike in Drupal 8).
• 5. I think we should check $_POST['ajax_page_state'] instead of $form_state['input']['ajax_page_state'] as nothing else in core does so.
• 6. I still think the new property to inform ajax_process_form() is necessary.
• Edit 1: We also need this chunk as else parameters won't be passed correctly:
• Edit 2: And I absolutely agree that we should do the ob_flush() to avoid putting content out - if we are called from theme layer or such.

(For that last one, I used ob_clean(), which is what was listed in #61. If that really should be ob_flush(), let me know.)

I did not include this item:

• 1.2. But this needs to have test coverage.

I did not know what needed to be done with this item, even after reading through the entire thread:

• 3. I think a first iteration should make this opt-in instead of opt-out.

We applied my second patch from #69 (drupal-no_cache_form-63-with-66-without-58-2819375-69.patch) to our production site and found a form where it did not work correctly. It seems that where there are multiple AJAX triggered fields, only the first one worked. For all the rest of them, processing stopped early after the first field without doing anything with the triggering field. After some testing and troubleshooting, I found that commenting out $element['#ajax_no_form_cache'] = TRUE; resolved the problem. After running through our tests, I have not yet seen that commenting out that line caused anything else to break.

@joelstein, I'm curious to know if you did the same thing I did, would that fix your problem without jumping through all the hoops you did?

I'm submitting updated patches without the ajax_no_form_cache flag.

I also added more PHP tests to see if this passes on other PHP versions.

I'm back with another modification to the patch.
This one removes the file handling part, due to a problem we were having with the Media Browser, as was mentioned in #44 but which I missed.
Again, I have two patches: with and without the changes in #58.
The interdiffs are between these patches and the ones I submitted in #72.

Bumping to 7.61. This didn't make it into 7.60.

+1

1. +++ b/includes/ajax.inc
   @@ -738,10 +747,21 @@ function ajax_pre_render_element($element) {
   +    if (isset($element['#ajax']['callback']) && !isset($element['#ajax']['path'])) {
   

   Let's add a variable_get() for now that you can do:

   $enabled_callbacks = variable_get('drupal_ajax_no_form_cache_enabled_callbacks', []);
   if (isset($element['#ajax']['callback']) 
    && isset($enabled_callbacks[$element['#ajax']['callback']]) && !isset($element['#ajax']['path'])) {
   }
   

2. +++ b/includes/ajax.inc
   @@ -738,10 +747,21 @@ function ajax_pre_render_element($element) {
   +      $query_parameters['_drupal_ajax'] = 1;
   

   This is no longer needed.

Here's a patch that does #82 and extends from drupal-no_cache_form-63-with-66-without-58-2819375-76.patch.

Actually, let's go back to #69 and add the variable_get opt-in to it. It will also leave out the file changes.

Here we add some test coverage that specifically tests the new opt-in approach.

testbot says 1 fail here:
https://dispatcher.drupalci.org/job/drupal_d7/106733/testReport/AJAX/AJA...

Line 646 of modules/simpletest/tests/ajax.test:

I could be missing something, but AFAICS this:

    $cids = array();                                                               
    $results = cache_get_multiple($cids, 'cache_form');                            
    $this->assert(empty($results)); 

...will always get an empty array for $results.

See, for example, the db cache backend's implementation of cache_get_multiple. Plus also this implementation of a getMultiple method which returns straight away if $cids is empty (not sure if it should actually be returning an empty array, but you get the point...)

It looks like perhaps the intention is a wildcard cache_get (or something like SELECT COUNT(*) FROM cache_form) but I don't think that's what this code does... which would be one reason why the assertion that expects to get results back from the cache is failing:

    $cids = array();                                                               
    $results = cache_get_multiple($cids, 'cache_form');                            
    $this->assert(!empty($results)); 

However, I had a quick try at putting something like a count query in to check whether there were actually entries in the cache_form table, and I didn't find any.

One reason could be that this test has the page cache switched on, so doing a second GET for the form may just retrieve the cached HTML without actually exercising the Form API code again with the drupal_ajax_no_form_cache_enabled_callbacks variable set to an empty array.

I ran out of time messing about with this and hadn't managed to get the test to work properly, but hopefully these observations are of some use.

New patch based on #87.

Made a few changes to the new tests; I confirmed that we needed to change the way we were checking for entries in cache_form; I've added a very rudimentary way of looking up the specific form:

   /**
+   * Check for a given form build id in cache_form.
+   */
+  protected function getFormFromCache($form_build_id) {
+    return (bool) cache_get('form_' . $form_build_id, 'cache_form');
+  }

Removed this from the test form:

 function ajax_forms_test_no_form_cache($form, &$form_state) {
-  $form_state['no_cache'] = TRUE;

...as with that there the form was never going to be cached.

I also removed page caching from the test, as I confirmed this would prevent the form being re-processed with the different settings unless the page cache is cleared (or a cache-buster query string used, for example). It seems to me we're not testing the page cache here, so we're better just removing that.

Restored Fabianx's flag to disable form caching (per #48):

 function ajax_process_form($element, &$form_state) {
   $element = ajax_pre_render_element($element);
-  if (!empty($element['#ajax_processed'])) {
+  if (!empty($element['#ajax_processed']) && !isset($element['#ajax']['no_form_cache'])) {
     $form_state['cache'] = TRUE;
   }
   return $element;
@@ -750,12 +750,14 @@
     $path = 'system/ajax';
     $options = array();
 
-    $enabled_callbacks = variable_get('drupal_ajax_no_form_cache_enabled_callbacks', []);
+    $enabled_callbacks = variable_get('drupal_ajax_no_form_cache_enabled_callbacks', array());
     if (isset($element['#ajax']['callback']) && isset($enabled_callbacks[$element['#ajax']['callback']]) && !isset($element['#ajax']['path'])) {
       $query_parameters = drupal_get_query_parameters($_GET, array('q'));
 
       $path = $_GET['q'];
       $options['query'] = $query_parameters;
+
+      $element['#ajax']['no_form_cache'] = TRUE;
     }

...as without it, entries were always written to cache_form. We're not actually adding a new FAPI flag to $form_state (which quicksketch was concerned about in #53), but it seems like it is necessary to add a property to the element for this to work as opt-in (per #66 point 6). I am then a bit confused about #69 and #72 though.

The new tests pass locally; let's see what the test bot thinks.

Wondering if there's anything to stop this going to RTBC as tests are now passing?

Hi,

I'm having difficulties with the cache_form on ajax driven forms. However my ajax code also involves submits, frequent submits of the same form.
If I set in the settings.php the $conf['form_cache_expiration'] = a value more than one day, then the cache_form usually grows too big, and exceeds the 500MB total database space, this includes the cron frequent deletion of the cache_form expired forms.

But if I set the $conf['form_cache_expiration'] = to lets say 8 hours, then the users that will start some exercises will come back 10 hours afterwards and then they get an "Invalid post" ajax error that I can see in the error log.

Will this patch fix my issue? I guess it will improve it, however the description of this issue refers more to "viewing" ajax driven pages, this is why I'm asking whether this patch will fix my issue with submitting ajax forms.

I'm less familiar with drupal as you are, but in my simplistic view, what I need, it's just the last version of an ajax form, so that the users can continue afterwards, so the cache in the cache_form can be overwritten with the last values of that form, for the same user(within the same session for the same page/form). I do not need the cached values of that form for the previous invocations of Sorry if the complexity is higher in reality, maybe I'm oversimplifying.

For reasons of completion, I have disabled the anonymous cache, have the minimum cache lifetime set to none, and captcha installed on all pages (so that users can post comments).

Thank you

looks good to me, originally authored by one of the best "quicksketch"

The only thing that the core maintainers might complain about is maybe the 80 column limit, but not sure if that really matters anymore and if insist, we could reroll a new patch with the 80 character limit imposed. I honestly though find it a bit difficult to limit things down that low however I can see the argument for it.

RTBC

A solution based on patch #94 may be unstable for Commerce Add to Cart forms on the catalog page (at least for me). Also note that this solution does not work for random list of products. At the same time, a solution based on #67 works great.

I have an issue with the patch #94.
It is working as expected on my ajaxified login form.

I also have an ajaxified "feedback" new node form - which is working well when creating entries in cache_form. However, as it is available on every single page, I try to not include entries in cache_form. As soon as I do so, using

  // don't cache the form
   $enabled_callbacks = variable_get('drupal_ajax_no_form_cache_enabled_callbacks', array());
   $enabled_callbacks['feedback_popup_helper_ajax_submit'] = TRUE;
   variable_set('drupal_ajax_no_form_cache_enabled_callbacks', $enabled_callbacks);

My callback function isn't called anymore for some reason - and I can't understand why. Instead I get a popup error mesage with HTTP Result Code: 200, StatusText: OK and ResponseText = content of the page I am currently on, which is not what is expected. The new node is still properly created though.

I think the node save is somehow overriding my custom handler and refreshes the page. Does anyone know why?

EDIT: I made a bit of progress and realized that it's because I am calling drupal_build_form to generate my form and not drupal_get_form. With drupal_get_form, I have my desired behavior, except that the form doesn't empty after a successful submission. I also realized that on the ajax post page, drupal_get_form is actually called several times, which is not ideal from the performance perspective. How comes?

***EDIT***Client is running php 7.3.x , I have a VBO form with ajax in a couple different forms and , patch 42 helped in one case, but caused a regression in another case. I will follow up with more details as I approach a solutionand views_php , I needed this patch when running php 7.3.x

Thanks for the patch!
Another satisfied client!

possibly related issue #2274543: [7.x-1.x] Remove usage of deprecated create_function()

spent a lot of work on making views_php work with the closure library, this core patch was the missing piece of the puzzle for me.

update: my form ids are randomly generated by VBO with ajax forms (views_php also might be a factor as well not sure), so configuring them to be excluded from caching is maybe not practical (looking into this).

I'll look into a better solution for my use case, will follow up if I can nail this.

for more info, see my comment here:

#2274543-103: [7.x-1.x] Remove usage of deprecated create_function()

if I downgrade to php 5.6.x then clear caches, my random id ajax forms work, but as soon as I switch back to php 7.3.x , I get an error Unsupported operand types in form_get_cache() (line 525 of web/includes/form.inc).

my patches for views_php work fine in my particular ajax vbo form views_php use case, however as soon as I switch back to php 7.3.x , core is unable to deserialize the cached form.

***EDIT***ok, I did some more testing, patch 42 by @quicksketch works for my use case where the form ids are randomly created by VBO and I have no way to know what they will be, still rolls cleanly.***EDIT***

so, patch 42 with php 7.3.x and views_php using the closure library using VBO and ajax forms and associated patch to replace create_function is good for us! Thanks @quicksketch!

the latest patch with the 'opt-in' doesn't work for us because we cannot know what form id to opt in on form cache excluding as it is a form id generated by the VBO module. ***EDIT*** re-evaluating this ***EDIT***

***EDIT***After re-reading this issue, the safest working solution for my clients is patch 42 and apply the interdiff from comment #58 on top of patch 42. ***EDIT***

This is what I will go with for my clients at this time.

@joseph.olstad Following up on comment #106 the patch in #63 attempted to do the same but failed testing, could a new patch be forthcoming?

hi @izmeez ,
I am reviewing this now, with php 7.3.x I found a regression with patch 42 with or without interdiff 58.
If you enable threaded commenting on a content type and enable the contrib module ajax_comments and the comment_goodness module , you can create a comment but replies to that comment thread will not generate a comment.

I found a symptom, a notice from includes/form.inc Undefined index: form_id in drupal_get_form() (line 132 of web/includes/form.inc). where line 132 was either the patched line from patch 42 and possibly 58, I was testing with and without interdiff 58 so I am not sure if that had an affect on the line number.

with either of the patches 42 with or without interdiff 58

and patch 94

the ajax_comments module stops working for comments with threaded responses (unable to reply) under php 7.3.x when these patches are made.

To reproduce this, enable comments for a content type, enable comment_goodness , enable threaded comments and the ajax_comments module

configure Drupal to run under php 7.3.x add a comment to the node (it works), then reply to that comment, the reply will not save.

HOWEVER, this patch (42 with or without interdiff 58) DOES help another use case of mine, however I'm caught between fixing one problem and breaking something else.

I haven't looked closely enough at the patch or the issue to pinpoint a fix (yet). With that said, I reviewed patch 42, with or without 58 interdiff and patch 94, and in my tests $_POST['form_id'] in includes/form.inc was undefined, I saw a warning about that a few times during tests, in either of the patches, I tried a few things quickly but no dice yet.

patch 94 didn't work either way, patch 42 helped with my vbo ajax form, but breaks ajax_comments

Private message me if you want access to the test environments that I have for the two use cases. I can easily switch between php 7.3.x , 5.6, 7.0, 7.1, 7.2

I can reproduce exactly where patch 42 succeeds and where it fails

My idea for tomorrow is to compare includes/form.inc and includes/ajax.inc between Backdrop cms and patch 42

I am hoping that Backdrop cms has fixed this and maybe can backport the related backdrop cms changes to D7, worth a try, a diff might provide some hints.

I'm not sure why this issue is child of #3012308: D7: Fully support PHP 7.3 ?

To me this change to the Form API does not seem like something we need to fix in order to be compatible with PHP 7.3, and that's what I'd expect to be the case for children of that meta issue. As such, the association is not helpful when trying to decide what work remains to be done for PHP 7.3 support.

That's why I'm removing that relationship between the issues. If I'm missing something here and the link should be restored, please let me know.

Mcdruid, this is a php 7 issue. Should still be tagged as such. When I revert to php 5.6 I no longer get operand type error when using vbo with ajax form.

Also when I patch with patch 42, it allows me to bump up to php 7.3 without the operand type error in form.inc

However when patched ajax_comments module enabled, replies to threads do not work.

I am going to spend a bit more time on this today. This is a blocker for us I might have to advise my client to roll back to php 5.6 if I do not find a solution soon.

@mcdruid

Because #2656548: Fully support PHP 7.0 in Drupal 7 was repeatedly closed, where else should it live?
I'm happy to re-open #2656548: Fully support PHP 7.0 in Drupal 7 and re-parent it to that, let us know.

Thanks for the feedback @joseph.olstad and @MustangGB

However, my understanding of this issue is that it's about changing the Form API such that ajax forms work in a fundamentally different way (i.e. without using cache_form).

There are many good reasons we might want to make that change.

If the patches here avoid some PHP 7 pitfalls though, I think we'd need one or more followups to fix those PHP 7 issues independently of any changes to the Form API.

In #104 @joseph.olstad mentioned:

.. as soon as I switch back to php 7.3.x , I get an error Unsupported operand types in form_get_cache() (line 525 of web/includes/form.inc).

...but I think there are other slightly different errors mentioned elsewhere.

Do we already have a separate issue for any of this?

I'd argue any PHP 7 (in)compatibility problems should be handled separately from the functionality change being proposed in this issue.

I've backported 'Backdrop' form.inc and ajax.inc , it's "almost" working, I'm getting one error, "Invalid form POST data." but the ajax call is actually working correctly so I'm not sure why I get this.

I've managed to fix the ajax_comments issues, now it's the VBO ajax form, it's working, but getting a warning about post data.

So anyhow, leaning heavily on work done by Quicksketch here, I took his patch 42, and then massaged/backported some of his work to Backdrop form.inc and ajax.inc , this is very helpful so far.

However one last bit to figure out.

Whether or not this passes tests, it does resolve my use cases , ajax_comments replies on node comments when ajax_comments is enabled using php 7.3 works fine.

Also, VBO ajax forms with or without views_php using the closure library, works fine as well no issues. Tested, I will retest this tomorrow just to make sure but pretty confident this is resolving all my use cases with php 7.3 and ajax forms.

Ok, Thanks to everyone here for the tests, thanks to Quicksketch for patch 42 and for Backdrop, I just backported 'SOME' pieces of form.inc and ajax.inc from Backdrop latest head back to patch 42 , as well, I added the simpletests from patch 94.

Please see the interdiff showing the changes from patch 42 to patch 117

Queueing up tests.

wow that's a lot of errors.

Ok I rebased from patch 94, and with these changes
Ok, I've tested this patch inside out on php 5.6 , php 7.0 , php 7.1 , php 7.2 and php 7.3

I'm having a problem with my use case using php 7.3 , getting unexpected operand types, if I change the related line of code it will finish the ajax call but the callback gives a warning message about expecting four parameters in views_form (views.module), received only two parameters. But all other versions of php I have tested work fine with this patch and do not get this error.

I will open a new issue for the followup php 7.3 issue , appears possibly a problem in cache_get , or cache_set, or form_cache_get I'm not sure yet.

I've tidied up the comments in this patch, borrowing some comments improvements from Backdrop. See the interdiff.

not included with the patch, is a line I have to add for php 7.3 , as a workaround, it still throws an error, but after completing the ajax call that actually saves my data correctly.

here's the line of code:

/includes/form.inc

         // Re-populate $form_state for subsequent rebuilds.
-        $form_state = $cached->data + $form_state;
+        $form_state = (empty($cached->data)) ? $form_state : $cached->data + $form_state;

         // Indicate this build was loaded from the cache, not a fresh build.
         $form_state['build_info']['from_cache'] = TRUE;

If I do not change this line as illustrated, I get an unsupported operands error in php 7.3.x, however no such problem in includes/form.inc exactly on the + operation between $cached->data + $form_state, however php 7.2.x and other versions of php I tried 7.1, 7.0 do NOT have this problem, they work just fine. It is only php 7.3 that has this issue.

In php 7.3 with this line of code added to the patch 120, my vbo views ajax form saves the data, but throws an error in views.module about expecting 4 parameters and getting only 2 , I believe this is due to a problem with cache_get working with the closure library I'm using with views_php.

Not entirely sure, however, it's working albeit with an annoying warning message popup confirmation about expecting 4 parameters and getting only 2. While this warning comes from views.module view_form , I suspect the real issue is somewhere up in includes/form.inc in form_cache_get or cache_get it'self, not sure yet.

ironically the patch 117 with the 90 simpletest errors actually worked in my testing environment without the annoying popup warning message but with that many simpltest errors I decided against going in that direction.

16 fails in php 7.3, might be related to the issue I am observing with unexpected operand type but not necessarily the same issue.

I'll open a new issue for the php 7.3 support.

Ok, sorry for all the noise here, I've tested in my environments php 5.6, 7.0, 7.1 , 7.2 and 7.3 with this patch.
There's no issue with it, it works.

The php 7.3 issue I noticed is unrelated, I've done significant diagnosis and it appears to be a bug with php 7.3.8 that is probably fixed in 7.3.9

so there's no action item there.

I've attached a cleaned up patch with fixes for really bad comment style in includes/file.inc
also some backdrop backport for a form validation error class. Should probably remove it but I thought it was interesting.

Patch 94 is good as is, but check out my patch 122, it's got a lot of fixes for comments that are busted, see the interdiff.

feel free pick what you like out of it, discard the rest.

back to RTBC, I want to move this issue forward

take patch 94 as-is if you like or take a look at my interdiff 94 to 122 and see what you want from it.

I'll let the core maintainers decide what they want to do.

patch 122 is closer aligned to backdrop cms , in terms of comments and use of call_func()

both patches work, they both do the job.

Comment changes are fine, but unrelated non-needed code changes are not.

Over to mcdruid for a re-review and then back to me for final review.

ok Fabianx, as requested, here's the comment fixes isolated, apply this interdiff to patch 94 and you'll get the comments fixes.

or, just take this patch.

Ok, to follow up with the noise I mentioned above, extensive testing and somewhat convincing results.

the out of scope php 7.3.x issue I am mentionning probably hasn't been seen in D8 yet because the popular editablefields module has NOT been ported to D8 yet.

php 7.3.x behaves slightly differently than php 7.2.x when it comes to empty values being operanded and I managed to stumble on a contrib module called editablefields that exposes this. editablefields provides an ajax form in a view and in that view will be an entity , editablefields attempted to rebuild a form from cache and pass it to views, in php 7.3.x if the cache->data is empty it cannot be fused with the + operator to the $form_state without throwing an unsupported operands in form_get_cache in includes/form.inc

All of this stems to some edge case where serialize() mis-serializes something.

I do notice an unserialize error in includes/cache.inc that basically was ignored by versions of php less than 7.3. Only php 7.3.x makes noise about this.

So, to maintain php 7.3.x compatibility with the editablefields module I had to first patch the editablefields module (see mention below) and also apply this core patch:
https://www.drupal.org/files/issues/2019-11-04/php7.3_edge_case_closure_...

the patch to the editablefields module basically replaces ALL instances of
$form_state['rebuild'] = TRUE;
with:
$form_state['rebuild'] = FALSE;

with these two patches my clients site is functioning as it did with php 7.2.x

Maintaining RTBC on patch 124!

The 124 patch is good, does not cause regressions, performs its job as advertised. The noise that I mention is a complicated recipe of editablefields and views_php with opis/closure and is totally out of scope for this issue. Just wanted to provide followup here.

Ok, still maintaining and holding the RTBC on patch 124 (or 94, but 124, see the 94_to_124 interdiff I provided with the patch)

as for the php 7.3.x specific issue that is exposed by editablefields ajax forms
see:
#3093423: php 7.3.x compatibility, unexpected operands in form_get_cache and too few arguments

Ran into an issue getting 500 errors from ajax using Ubercart attributes. :-/ Cache clearing didn't help.

Running Drupal 7.69 on PHP 7.3

I am getting notices with patch 124 applied:

Notice: Undefined index: form_id v drupal_get_form() (line 132 in /includes/form.inc).

So I added an isset call:

 $ajax_post = isset($_POST['ajax_page_state']) && isset($_POST['form_id']) && $_POST['form_id'] === $form_id;

@philsward , have you tried patch 124?

I found the same issue as @Neo13 in comment #131. Throws an undefined index error.

I'm not sure performing an isset fixes the problem though. It is an Ajax request, just that the $_POST data is not set.

An example would be when the browser session has expired, but the user still has a form loaded and attempts to submit.

Yes, it doesn't fix the underlying issue. Just the notice.

It would be best to refresh the form / page in this case to allow for proper form submission?

in my case my client was using the views_php module, I ported over the views_php code/logic somewhere else, disabled the views_php code
so basically I moved funky views_php logic that was at the root cause of my issues and used the hook system instead to achieve the same thing.

I cannot recall whether or not I ended up keeping the patch installed.

@Neo13, yes something to that effect would probably be best. I have decided not to use this patch and instead have implemented a solution involving the use of ajax commands.

I found this to be a helpful post: https://drupal.stackexchange.com/questions/109094/how-to-reload-page-aft...

@joseph.olstad, I understand, my final solution uses the hook system too. I won't be using this patch, but if others find it useful the "Undefined index" issue will need to be fixed.

@ron_s Please share your code for other people :)

@Neo13 our code is very customized for our solution and would not make any sense. Using the link I provided gives everything you'll need to create code that meets your needs.

Sorry that this dropped of the radar, but it sound like Needs Work is appropriate based on recent comments.

It's also not clear (to me) which patch we'd be reviewing now; if there's more than one candidate for review, can we please have a summary of the differences?

Reroll #124 for 7.98