See #671184-74: AJAX form can submit inappropriately to system/ajax after failed validation. This patch needs some tests added, but wanting a bot-check on just this much.

CommentFileSizeAuthor
#124 drupal.form-ajax-rebuild.124.patch13.17 KBsun
#122 drupal.form-ajax-rebuild.121.patch13.05 KBsun
#119 drupal.form-ajax-rebuild.119.patch13.36 KBsun
#115 756762-fapi-rebuild-115.patch12.57 KBeffulgentsia
#113 756762-fapi-rebuild-113.patch9.88 KBeffulgentsia
#111 756762-fapi-rebuild-111.patch9.84 KBeffulgentsia
#105 756762-ajax-nonbutton-rebuild-105.patch2.44 KBeffulgentsia
#103 ajax-fix.patch3.81 KBfago
#99 drupal.ajax-rebuild.99.patch12.06 KBsun
#93 756762_followup.patch445 bytesIsland Usurper
#88 drupal.form-ajax-rebuild.88.patch28.53 KBsun
#85 drupal.form-ajax-rebuild.85.patch27.86 KBsun
#78 drupal.form-ajax-rebuild.78.patch25.9 KBsun
#74 ajax.patch25.18 KBfago
#68 ajax.patch25.16 KBfago
#65 ajax.patch25.16 KBfago
#59 drupal.ajax_rules_756762_59.patch21.07 KBpwolanin
#49 drupal.ajax_rules_756762_49.patch21.33 KBrfay
#49 drupal.ajax_rules_756762_47_to_49_diff.txt946 bytesrfay
#47 fapi_ajax_reroll.patch21.32 KBfago
#33 form_ajax_fix.patch21.26 KBfago
#29 drupal.ajax_fapi_rebuild-29.patch21.97 KBeffulgentsia
#28 drupal.ajax_fapi_rebuild-28.patch20.45 KBeffulgentsia
#27 drupal.ajax_fapi_rebuild-27.patch18.58 KBeffulgentsia
#25 form_api_cleanup3.patch13.72 KBfago
#21 form_api_cleanup2.patch13.21 KBfago
#19 drupal.ajax_fapi_rebuild-19.patch5.12 KBeffulgentsia
#17 drupal.ajax_fapi_rebuild-17.patch5.11 KBeffulgentsia
#14 drupal.ajax_fapi_rebuild-14.patch18.8 KBeffulgentsia
#12 drupal.ajax_fapi_rebuild-12.patch17.94 KBeffulgentsia
#11 drupal.ajax_fapi_rebuild-11.patch17.95 KBeffulgentsia
#10 drupal.ajax_fapi_rebuild-10.patch17.93 KBeffulgentsia
#7 drupal.ajax_fapi_rebuild-7.patch16.51 KBeffulgentsia
#6 drupal.ajax_fapi_rebuild-6.patch16.53 KBeffulgentsia
#4 drupal.ajax_fapi_rebuild-4.patch12.19 KBeffulgentsia
#2 drupal.ajax_fapi_rebuild-2.patch12.11 KBeffulgentsia
drupal.ajax_fapi_rebuild.patch1.63 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rfay’s picture

subscribing

effulgentsia’s picture

More ambitious cleanup. No need to duplicate if ($form_state['rebuild'] && $form_state['process_input'] && !form_get_errors()) in every #ajax['path'] callback. Instead, let's ensure consistency by re-using drupal_build_form().

Status: Needs review » Needs work

The last submitted patch, drupal.ajax_fapi_rebuild-2.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
12.19 KB

.

sun’s picture

+++ modules/file/file.module	31 Mar 2010 23:46:15 -0000
@@ -217,8 +217,13 @@ function file_ajax_upload() {
+  // @todo ajax_get_form() issues a drupal_exit() when the form can't be
+  //   retrieved, preventing this code from running.
   if (!$form) {
     // Invalid form_build_id.
     drupal_set_message(t('An unrecoverable error occurred. Use of this form has expired. Try reloading the page and submitting again.'), 'error');

Can we issue this message in general instead of doing nothing?

131 critical left. Go review some!

effulgentsia’s picture

Meh. Trying to minimize AJAX refactoring as Drupal 7 needs to be stabilizing, but things just keep coming up, like dealing with $header if adding error return to ajax_get_form(). As far as I can tell though, this doesn't break BC.

effulgentsia’s picture

Minor fix.

rfay’s picture

This is great stuff, IMO.

+++ includes/ajax.inc	1 Apr 2010 01:32:24 -0000
@@ -292,10 +303,13 @@ function ajax_form_callback() {
-function ajax_deliver($page_callback_result) {
+function ajax_deliver($page_callback_result, $header = TRUE) {

This interface change sure looks like a winner. However, it would be far more flexible if you had the option of passing the content type, rather than a bool. There's an outstanding bug (that I can't find right now) that this would easily fix.

Edit: The issue I remembered was #459086: AJAX upload to JSON reply trouble, which I ended up marking fixed because I couldn't find a basis for it. However, had it been valid (or maybe it was) this interface change would be the first step to making it work right.

Powered by Dreditor.

cdale’s picture

+  // If $form cannot be loaded from the cache, the form_build_id in $_POST must
+  // be invalid, which can happen if:
+  // - Someone performed a POST request to system/ajax without first viewing the
+  //   concerned form in the browser.
+  // - More time has elapsed between the user viewing the initial form and
+  //   performing the AJAX submission than the lifetime of entries in the form
+  //   cache (6 hours, by default).
+  // - The form cache was fully flushed, removing recent entries as well as
+  //   expired ones (see http://drupal.org/node/512026).

FYI: This can also happen if a POST request exceeds post_max_size, as PHP drops all post data in this case for security reasons, therefore no build id can be found in $_POST. File uploads can quite often trigger this for lower values of post max size, or if someone tries to upload a file large than the configured post max size. (NOTE: This is different than upload_max_filesize, if upload_max_filesize is exceeded, but post_max_size is not, then there will still be a build id).

effulgentsia’s picture

Thanks! This improves file_ajax_upload() and incorporates #9. Re #8: if there's a use-case for $header supporting more, let's identify it. Currently, it's based on legacy AJAX code that is explained at the end of ajax_deliver(). To be honest, I'm not even all that sure how much of that is still true. There is no core code using 'multipart' and I'm not sure if the IFRAME issue still exists.

effulgentsia’s picture

+++ includes/ajax.inc	1 Apr 2010 02:47:20 -0000
@@ -219,32 +223,47 @@ function ajax_render($commands = array()
+    // Not drupal_exit() because drupal_deliver_page() performs necessary
+    // cleanup.

This patch makes this clearer, I hope.

effulgentsia’s picture

+++ includes/ajax.inc	1 Apr 2010 02:59:27 -0000
@@ -219,32 +223,47 @@ function ajax_render($commands = array()
+    // @todo Add exit() to drupal_deliver_page()?
+    exit();

No parentheses for exit

fago’s picture

>$form_properties_to_preserve_during_rebuild

puh, that parameter is really ugly ;) What about just adding a new flag $form_state['ajax'] and then let drupal_rebuild_form() care about keeping the right properties?

>+function ajax_get_form($process = FALSE) {

The term 'process' is already used in FAPI terminology (->drupal_process_form()), so I think it's better to not use it here. Why not just use $build - that would indicate that it calls drupal_build_form().

Another issue with terminiology we have is the mix of "form builder" vs "form constructor" - but actually form building is already overloaded (see form_builder(), drupal_build_form()), but that's another issue.

effulgentsia’s picture

puh, that parameter is really ugly ;) What about just adding a new flag $form_state['ajax'] and then let drupal_rebuild_form() care about keeping the right properties?

This makes it less ugly, but I oppose $form_state['ajax'] for reasons that I hope are clarified by the comments in this patch.

The term 'process' is already used in FAPI terminology (->drupal_process_form()), so I think it's better to not use it here. Why not just use $build - that would indicate that it calls drupal_build_form().

FAPI intermingles 'build' and 'process' all over the place, causing lots of confusion. drupal_process_form() calls form_builder() which calls #process functions. But I think $process is the correct name for the parameter, because the most important affect that it has is whether you get back a $form that has had drupal_process_form() called on it or not. I added comments to ajax_get_form(). Please see if you agree based on this reasoning.

sun’s picture

+++ includes/form.inc	2 Apr 2010 00:16:20 -0000
@@ -92,6 +92,9 @@ function drupal_get_form($form_id) {
+ *   NULL may be passed for this parameter, in which case, a form is processed
+ *   and returned only if it can be retrieved from cache, but if it can't be,
+ *   then no form constructor function is called.

I really don't like this multi-purposing of $form_id. Can we find alternative solutions?

+++ modules/file/file.module	2 Apr 2010 00:16:21 -0000
@@ -211,57 +211,41 @@ function file_ajax_upload() {
+  // Get the $form prior to processing and rebuilding, then process and rebuild
+  // and get the new $form. Use that to get the old and new $element being
+  // updated and the old and new file count.
+  list($old_form) = ajax_get_form(FALSE);
+  list($new_form) = ajax_get_form(TRUE);
+  $old_element = $old_form;
+  $new_element = $new_form;
...
+  $old_file_count = isset($old_element['#file_upload_delta']) ? $old_element['#file_upload_delta'] : 0;
+  $new_file_count = isset($new_element['#file_upload_delta']) ? $new_element['#file_upload_delta'] : 0;
...
   // Add the special AJAX class if a new file was added.
...
+  if ($new_file_count > $old_file_count) {
+    // @todo Why is $old_file_count used as the index here?
+    $new_element[$old_file_count]['#attributes']['class'][] = 'ajax-new-content';
   }

wow, this looks horrible :)

...and we only do this weird nightmare to get a stupid CSS class?? ;)

Please tell me I'm wrong.

131 critical left. Go review some!

fago’s picture

>For advanced use-cases, this parameter can be
>+ * an array of property names that are a subset of the ones that
>+ * drupal_rebuild_form() makes sure to preserve. For example, if this
>+ * parameter is array('#action'), then that property is preserved in the
>+ * rebuilt $form, but #build_id is not.

What are those advanced use-cases beside ajax and why can't they be handled in the form rebuild as usually? While $partial_rebuild sounds much better, it doesn't really fit either. Still the whole form is rebuilt, but we rebuild the same "build" of the form. Thus basically the flag says whether to build the same build(-id) again or a new one. So what about something like "$replace_build" - thus when TRUE the current build will be replaced on rebuild?

@$process:
ok, sounds sane. However this makes me think that 'rebuild' also belongs into drupal_process_form(). Isn't that really what we want? - just processing. But as it's baken half the way in drupal_build_form() we try to use that instead. So what about moving that rebuilding hunk into drupal_process_form() and making use of that instead?

The only possible problem I see here is that those 'batch_form_state' won't be rebuilt. I don't know how that form-batch stuff works and whether batch processing makes use of rebuilding, the comment says it's about displaying.

effulgentsia’s picture

OK, in response to the above (very appropriate) feedback, here's a patch that backs off and just tries to tackle this one issue.

For #5,#8,#9, let's do that in a separate issue.

For #15's comments about file.module: #736298: Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security.

Status: Needs review » Needs work

The last submitted patch, drupal.ajax_fapi_rebuild-17.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
5.12 KB

always happy to see bot successfully catching typos :)

effulgentsia’s picture

Category: bug » task
Issue tags: -Needs tests

Reclassifying this as a cleanup task (consistency++) rather than a bug-fix and removing the "Needs tests" tag. Here's why: on my local machine, I tried changing this code in drupal_build_form():

if ($form_state['rebuild'] && $form_state['process_input'] && !form_get_errors()) {
  $form = drupal_rebuild_form($form_id, $form_state);
}

to

if ($form_state['rebuild'] && $form_state['process_input']) {
  $form = drupal_rebuild_form($form_id, $form_state);
}

and ran all the Form API tests and they all still pass. In light of #622922: User input keeping during multistep forms, I'm not really sure how to create a non-artificial bug when a form is rebuilt even if there are validation errors.

But I still believe that it's more logical to not rebuild if there are validation errors, and to not rebuild if $form_state['rebuild'] isn't TRUE. And in any case, no one is proposing a change to that if statement within drupal_build_form(), and I continue to want to see AJAX form processing logic mirror non-AJAX form processing logic by default, and only be different when we can identify a need for it to be different.

fago’s picture

Category: task » bug
FileSize
13.21 KB

I think it's completely unlogical todo a rebuild if there is a validation error, as the user might not be able to correct it afterwards anymore ;) So that should be fine.

@new-approach:
I had a look whether we can improve drupal_process_form() to incorporate the rebuild hunk too - so we won't need to duplicate it. The problem with that is the batching stuff as mentioned above, so I took a look at that. What it does is basically that it interrupts the form API flow during submit to run the batch, so in the end in comes back. This is important when the form needs to be rebuilt. However previously, when it came back to the form from the batch and it didn't rebuild I think it misses $form - so that would go wrong.

Thus I tried to fix that so that the batching form state is only set when it needs to be rebuilt - which helps us to clean drupal_build_form() and drupal_process_form() up. Patch attached.

fago’s picture

Category: bug » task

? I didn't change that - back to task.

effulgentsia’s picture

I like this a lot in spirit, and I agree that it makes much more sense to think of rebuilding as the last part of the process step, but are we allowed to make such a change to drupal_process_form() at this stage of D7 development? Are we concerned about breaking BC for modules that call drupal_process_form() and expect it to not follow up with a rebuild? Do we need confirmation from sun? chx? Dries? before going further down the road of #21?

yched’s picture

I have to confess this area of FAPI is kind of above my head now. From a purely Batch API point of view, #21 sounds reasonable.

Not really related, but while looking it this area, I noticed #760738: $batch includes full form state in unneeded cases (shameless plug)

fago’s picture

FileSize
13.72 KB

Thanks yched, important to have green lights from the batch API side.

@#23: I don't think that drupal_process_form() is often used directly by modules beside of AJAX callbacks (I wasn't able to find one..). However AJAX callbacks needs to be fixed anyway so they follow the same form workflow as else - thus I think it should be ok to change that now.

However, drupal_form_submit() also makes use of drupal_process_form(), thus we need to check for programmatic form submissions. I added that, so they won't be rebuilt or cached as before. I also moved the rebuilt/cache hunk up into the already existing if($form_state['process_input']) clause, so we don't have to check that again.
Updated patch attached.

One think came to my mind though, should we introduce $form_state['no_rebuild'] analogously to no_cache and no_redirect?

yched’s picture

(side note, different can of worms : supporting batch API processes triggered in #ajax submits would be an interesting challenge... a jQUI progressbar ? Right now, and probably as far as D7 is concerned, badly breaks)

effulgentsia’s picture

Some small changes to #25, including a test strengthening because #25 should have failed, and that it didn't indicated a weak test.

effulgentsia’s picture

This one has a little more cleanup. I'm pretty happy with this. @fago: what do you think? @sun: your feedback would be great too.

I agree with fago (#25) that even though this changes what drupal_process_form() does, I don't think it will cause BC problems, so I don't see it as a problem for D7. Most places call drupal_(get|build)_form() or drupal_form_submit() and what those return is unchanged. The only contrib code that should be calling drupal_process_form() directly is #ajax['path'] callbacks, and from there, based on HEAD, the call to drupal_process_form() should be followed with a call to drupal_rebuild_form(). And that doesn't break with this patch. This patch makes it possible (and recommended) for those callbacks to remove the call to drupal_rebuild_form(), but for any that leave it in, other than some inefficiency from drupal_rebuild_form() being called twice, no functional bugs are introduced.

So all in all, I think this is reasonably safe, a really nice cleanup, helps AJAX modules do things right, and nudges FAPI a little closer to making sense.

effulgentsia’s picture

Updated docblock for drupal_rebuild_form().

effulgentsia’s picture

@fago: shameless plug, but could really use your masterful understanding of multistep forms on #736298: Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security. thanks!

fago’s picture

What was wrong with #25?

@$form_state['retain_during_rebuild'] - Do we really need to let
the user to specify that? Are there any use cases for that? If not, I'd prefer to keep things simple and don't have that option.

+ // Caching is normally done in drupal_process_form(), but what needs to be
+ // cached is the $form structure before it passes through form_builder(),
+ // so we need to do it here.
+ // @todo For Drupal 8, find a way to avoid this code duplication.
Hm, I don't think it's really duplication. It are two different cases and I think it's fine to handle them differently. Only when the form is rebuilt $form changes and thus needs to be cached again. If it's not rebuilt, it suffices to cache $form_state and keep the existing $form cache.

+ if ($form_state['rebuild'] && $form_state['process_input'] && !form_get_errors()) {

I removed the check from $form_state['process_input'] as it's already checked above, thus it's duplicated and can be removed.

@caching $unprocessed_form:
Well previously it did just update the cached form_state but not $form. What's the cause to change that? If form processing is at this stage the form has been processed, thus when caching is enabled $form is usually already cached - so the previous behaviour should be fine.

Where is the initial form_set_cache gone? With your patch the form is only cached when input is processed, however it needs to be cached also the first time the user just loads and views the form and AJAX process handler enable $form_state['cache']. Note the hunk is inside the if ($form_state['process_input']) check now.

But yes, moving that check into drupal_process_form() makes sense, I thought about that too. Also the variable name $unprocessed_form makes much sense.

@shameless plug: also see
#759344: support building forms defined in multiple includes
#759222: #tree does not default to TRUE, fails to meet developer expectations
;)

effulgentsia’s picture

What was wrong with #25?

It always called drupal_rebuild_form() without ever passing the $old_form parameter, even for AJAX. #29 strengthens the test that should have caught that. Other than that, since we moved so much from drupal_build_form() to drupal_process_form(), I wanted to move the form_set_cache() to there as well.

@$form_state['retain_during_rebuild'] - Do we really need to let
the user to specify that? Are there any use cases for that? If not, I'd prefer to keep things simple and don't have that option.

Well, someone needs to specify it, since with the patch (either #25 or #29), drupal_rebuild_form() is only called from a single place (drupal_process_form()), so somehow, $form_state needs to contain information that distinguishes AJAX from non-AJAX. You had earlier suggested creating a boolean $form_state['ajax'] variable, but suppose I want to build a betterajax.module. And in this betterajax implementation, I want to make it possible to use the browser's Back button to return to an earlier step. So in this case, I would not want #build_id being re-used. But I would still need #action to not get clobbered with the generic AJAX url. Basically, I think that the need to retain #build_id and #action are two separate needs that should not be coupled. It just so happens that Drupal's core AJAX implementation wants both retained. What's wrong with ajax_get_form() specifying that? That way, they stay uncoupled, only that one function sets the $form_state['retain_during_rebuild'] variable, and people writing #ajax['path'] callbacks just call ajax_get_form() as they already are and don't need to worry about it.

Hm, I don't think it's really duplication. It are two different cases and I think it's fine to handle them differently.

I'm okay with removing the comment, but I think it is duplication and a WTF that's worth noting. With your brilliant idea to move more into drupal_process_form(), I think it should be the only place that deals with form_set_cache(), since the purpose of caching the form is tied up with being able to process it when it gets submitted in the next page request. If we weren't in API feeze already, I would suggest that drupal_rebuild_form() should return the form prior to form_builder() being called on it, and to let drupal_process_form() re-invoke form_builder() (since despite the build/process terminology problems that we still haven't fully cleaned up, form_builder() is tied in with processing more so than with building). If this were done, then it would be easy for drupal_process_form() to handle calling form_set_cache() for rebuilt forms as well, because it's the exact same concept: cache $form from before the form_builder() step and the up-to-date $form_state.

I removed the check from $form_state['process_input'] as it's already checked above, thus it's duplicated and can be removed.

I moved the block outside of that if statement to make the "elseif" handle the case when input isn't being processed, thereby, allowing the removal of form_set_cache() from drupal_build_form().

Where is the initial form_set_cache gone? With your patch the form is only cached when input is processed, however it needs to be cached also the first time the user just loads and views the form and AJAX process handler enable $form_state['cache']. Note the hunk is inside the if ($form_state['process_input']) check now.

Nope. Look again. The one "elseif" at the end of drupal_process_form() handles the initial GET display as well as the POST submission.

fago’s picture

FileSize
21.26 KB

@#25: Indeed, thanks!

@$form_state['retain_during_rebuild']:
I see, with the use case of betterajax.module that makes perfectly sense. Let's do it that way.

@process_input: doh, indeed. I shouldn't review such complex patches without applying it :) However still this changes the caching logic a bit as now if the form is processes input but is not rebuilt, e.g. in case of validation errors the form cache gets overwritten although it *should* be already there. Anyway, it's not 100% sure it's already there and as $form_state['cache'] is enabled, we should make sure there is one now. Also updating the existing cache in some cases won't harm anyway, so I think this is fine.

Thus all together - #29 is great. I re-rolled it so it applies to HEAD again. (#760738: $batch includes full form state in unneeded cases got committed.)

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

However still this changes the caching logic a bit...Anyway, it's not 100% sure it's already there

I don't think it's changed at all from HEAD. In HEAD, whether $original_form is added to the cache or not depends on if that variable exists; NULL is passed instead when it doesn't exist, but that only happens if form_get_cache() returned a $form. With the patch, we always cache $unprocessed_form which is either the same as $original_form in HEAD or is what form_get_cache() just returned. So the only difference is when updating the cache after a validation error of a form that already existed in the cache, the patch re-caches the same $form, whereas HEAD doesn't: no functional difference AFAICT.

yched confirmed that this is good from a batch api perspective. I can confirm this is good from an AJAX perspective. And fago and I both like the form.inc changes, which while changing the functionality of drupal_process_form() with respect to it now calling drupal_rebuild_form(), is not really an API change as per #25/#28. So, I feel good calling this RTBC.

sun’s picture

um. This needs some more careful reviews.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

ok.

effulgentsia’s picture

Category: task » bug
Priority: Normal » Critical
Issue tags: +D7 Form API challenge

Reclassifying as a bug and bumping to critical, because some solution to this issue is a must for D7. At least as of now, form rebuilds are not always guaranteed to work correctly. Specifically, an entity editing form must have #builder_function called prior to it being rebuilt, or else the rebuild loses user input. This may get fixed by #735800: node form triggers form level submit functions on button level submits, without validation and related issues, but until FAPI can claim that rebuilds are always ok, even if not always necessary, it is a bug for ajax_form_callback() to rebuild the form without $form_state['rebuild'] and the other conditions being met.

If #33 is deemed too ambitious for D7, then this issue can be solved with either the initial issue patch or #19. While #33 is still on the table, tagging it accordingly.

fago’s picture

I do think #33 is the way to go. Form rebuilding has to behave the same way during ajax rebuilds as during regular rebuilds - the best way to achieve that is to *don't duplicate* the logic, but re-use the existing one. That is what the cleanup of #33 does.

fago’s picture

@processing & value assignment: I opened a separate issue for that: #768312: Cleanup form element processing and value assignment.

effulgentsia’s picture

More support for why it's bad that AJAX rebuilds forms without $form_state['rebuild'] being TRUE: #737686: Image field 'Remove' button broken: a bug that exists for the AJAX submission that does not exist for the non-AJAX submission of the same form and is due to an unrequested rebuild occurring during AJAX. #736298: Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security fixes it, but that the bug exists at all without having been caught by testbot, and that the test coverage of the non-AJAX submission doesn't give us confidence that the AJAX variant also works, is an example of why it's bad for there to be unnecessary deviation of FAPI processing for AJAX. So, yes, I agree with #38.

Jay.Chen’s picture

drupal.ajax_fapi_rebuild.patch queued for re-testing.

effulgentsia’s picture

Since this will take more thorough review time before landing, I posted the original patch to #737686: Image field 'Remove' button broken to fix that critical bug, so it doesn't need to wait until this issue's much nicer cleanup moves through the process.

Wim Leers’s picture

Subscribing.

fago’s picture

Issue tags: -D7 Form API challenge

#33: form_ajax_fix.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D7 Form API challenge

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

fago’s picture

Status: Needs work » Needs review
FileSize
21.32 KB

Status: Needs review » Needs work

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

rfay’s picture

Status: Needs work » Needs review
FileSize
946 bytes
21.33 KB

Another reroll - The drupal_html_id() commit broke the test. Attached is the patch and the diff from #47.

This has no change except in one line of the test.

YesCT’s picture

Issue tags: -D7 Form API challenge

#49: drupal.ajax_rules_756762_49.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal.ajax_rules_756762_49.patch, failed testing.

rfay’s picture

Status: Needs work » Needs review

#49: drupal.ajax_rules_756762_49.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D7 Form API challenge

The last submitted patch, drupal.ajax_rules_756762_49.patch, failed testing.

rfay’s picture

At least this time we got the correct message.

I took a shot at resolving the merge conflicts here, but there is a lot of water under the bridge and I think I'll defer to others who have their brains more properly in the FAPI and its recent changes.

effulgentsia’s picture

Yeah, this one could sure use a re-roll. I'll do it one of these days unless fago beats me to it.

Cross-linking #749106: Ajax form won't be cached after it's been submitted. I'm not sure without further investigation, but it may be the case that that issue points to yet another symptom of the fact that AJAX form handling entirely bypasses drupal_build_form() and all that it does after calling drupal_process_form(). #49 solves that by moving things out of drupal_build_form() and into drupal_process_form(), ensuring less divergence between non-AJAX and AJAX code flows (same essential point as #38, but not just with respect to rebuilding).

pwolanin’s picture

subscribe - I hope this fixes some of the absurd and non-intuitve flow e.g. for filefield in Drupal 6.

pwolanin’s picture

trying to re-roll, seems the problems are:

Hunk #1 FAILED at 168.
1 out of 3 hunks FAILED -- saving rejects to file includes/ajax.inc.rej

Hunk #1 FAILED at 168.
Hunk #2 succeeded at 354 (offset 103 lines).
Hunk #3 succeeded at 384 (offset 103 lines).
Hunk #4 FAILED at 404.
Hunk #5 succeeded at 427 (offset 103 lines).
Hunk #6 succeeded at 488 (offset 103 lines).
Hunk #7 succeeded at 666 (offset 107 lines).
Hunk #8 succeeded at 695 (offset 107 lines).
Hunk #9 succeeded at 759 (offset 107 lines).
Hunk #10 FAILED at 866.
3 out of 10 hunks FAILED -- saving rejects to file includes/form.inc.rej

pwolanin’s picture

looks like the ajax.inc conflict is just whitespace.

Two conflicts in form.inc looks to be just the change from md5() to drupal_hash_base64(), while the other a reformatted comment and a change to the new drupal_alter format..

pwolanin’s picture

Status: Needs work » Needs review
FileSize
21.07 KB

with conflicts resolved.

effulgentsia’s picture

Thanks, pwolanin! So just a reminder/summary of what this issue and the #59 patch is about: currently in HEAD, drupal_build_form() does a bunch of stuff after calling drupal_process_form(): stuff having to do with updating the form cache, rebuilding, and setting #theme. An AJAX workflow (see ajax_form_callback() for the canonical use-case), however, does not call drupal_build_form() (either directly or via drupal_get_form()), because the first part of drupal_build_form() (the part where it either retrieves the form from cache or calls the builder function appropriate for $form_id) doesn't work, because in AJAX context, we don't have a trustworthy $form_id. But even though AJAX requires its own special variant for the part of drupal_build_form() that runs before drupal_process_form() is called, everything FAPI-related that happens once drupal_process_form() is called should be the same for AJAX as it is for non-AJAX. So, this patch moves everything that drupal_build_form() does after calling drupal_process_form() into somewhere more appropriate: the rebuilding and cache updating logic into drupal_process_form(), and the #theme default into drupal_prepare_form().

fago and I have approved this already, but in #35, sun says it needs more careful review. So, let's get those careful reviews happening! Thanks!

effulgentsia’s picture

Issue tags: +API change

I realized this needs the "API Change" tag, because contrib modules implementing #ajax['path'] currently call drupal_rebuild_form() after calling drupal_process_form(), and after this patch, they'll need to only call drupal_process_form() and not call drupal_rebuild_form(). I think this is an acceptable BC break, because:

1) Only a few contrib modules should be using #ajax['path']: most contrib modules should be using #ajax['callback'] which will not require changes.

2) It's worth it, because currently, those modules are likely calling dupal_rebuild_form() incorrectly anyway (i.e., without checking for form_get_errors() or $form_state['rebuild']), since the two examples in core are incorrect. That's why this issue exists and is critical.

fago’s picture

I just ran into this one again :/ Totally weird that enabling #ajax changes behavior.

fago’s picture

Status: Needs review » Needs work
Issue tags: +API change, +D7 Form API challenge

The last submitted patch, drupal.ajax_rules_756762_59.patch, failed testing.

fago’s picture

FileSize
25.16 KB

ok, re-rolled the patch.

In the meantime drupal_build_form() changed a bit, so the patch broke. I fixed the patch to incorporate the changes in drupal_build_form() + I tried to further clean up drupal_build_form(). I'd really like to see drupal_build_form() dead simple so that everybody can easily follow the form workflow: get the form from cache / retrieve it + process it + return it for rendering. It's not more complicated than that, but we need to manage to get the additional logic out of drupal_build_form(). Most of this logic fits to a helper nevertheless, what would mean the code belongs there else someone that re-uses the helper would miss it.

What now is left in drupal_build_form() but I'd like to see moved to drupal_retrieve_form() is the form state cleaning as introduced by #800460: WTF coupling between $form_state['cache'] and form processing. The problem is that involves hook_form_alter(), what is invoked in drupal_prepare_form(). I do think this drupal_alter() is more about retrieving anyway, so it should move in to drupal_retrieve_form() too. But this would slightly change hook_form_alter() implementations as the passed form wouldn't be prepared any more, what looks fine to me, but is an API change. Thus the patch does not touch this. :/

Moving the logic out of drupal_build_form() in the end helps to avoid problems like this - as anyone re-using the helpers gets the full logic, not only parts of it.

fago’s picture

Status: Needs work » Needs review
fago’s picture

I just gave #65 a test with my ajax-enabled form that had problems when validation errors occur. With #65 the form is fine :)

fago’s picture

FileSize
25.16 KB

Re-rolled patch to keep it up2date.

Any reviews? :)

cwgordon7’s picture

Tested, everything worked - tested a few different AJAX behaviors. Code looks good to me, but I'd like someone else to take a look at this, too, before marking rtbc.

sun’s picture

Status: Needs review » Needs work
+++ includes/ajax.inc
@@ -239,6 +239,11 @@ function ajax_get_form() {
+  // When a form is rebuilt after AJAX processing, its #build_id and #action
+  // should not change.
+  $form_state['retain_during_rebuild']['#build_id'] = TRUE;
+  $form_state['retain_during_rebuild']['#action'] = TRUE;

How about putting these into Form API's semi-private 'build_info' space?

+++ includes/batch.inc
@@ -495,9 +495,11 @@ function _batch_finished() {
+    // If no redirection happened, redirect to the originating page. In case we
+    // need to rebuild, save the final $form_state for drupal_build_form().
+    if (!empty($_batch['form_state']['rebuild'])) {
+      $_SESSION['batch_form_state'] = $_batch['form_state'];
+    }

Is the batch or the form rebuilt?

s/we need to rebuild/the form needs to be rebuilt/

+++ includes/form.inc
@@ -275,127 +275,77 @@ function drupal_build_form($form_id, &$form_state) {
-  // After processing the form, the form builder or a #process callback may
-  // have set $form_state['cache'] to indicate that the original form and the
-  // $form_state shall be cached. But the form may only be cached if the
-  // special 'no_cache' property is not set to TRUE and we are not rebuilding.
-  elseif (isset($form_build_id) && $form_state['cache'] && empty($form_state['no_cache'])) {
-    // Cache the original, unprocessed form upon initial build of the form.
-    if (isset($original_form)) {
-      form_set_cache($form_build_id, $original_form, $form_state);
-    }
-    // After processing a cached form, only update the cached form state.
-    else {
-      form_set_cache($form_build_id, NULL, $form_state);

@@ -820,6 +788,29 @@ function drupal_process_form($form_id, &$form, &$form_state) {
+  // After processing the form, the form builder or a #process callback may
+  // have set $form_state['cache'] to indicate that the form and form state
+  // shall be cached. But the form may only be cached if the 'no_cache' property
+  // is not set to TRUE. Only cache $form as it was prior to form_builder(),
+  // because form_builder() must run for each request to accomodate new user
+  // input. We do not cache here for forms that have been rebuilt, because
+  // drupal_rebuild_form() takes care of that.
+  elseif ($form_state['cache'] && empty($form_state['no_cache'])) {
+    form_set_cache($form['#build_id'], $unprocessed_form, $form_state);
   }

I may be mistaken, but reviewing the patch only (without applying it), it seems like only the first condition is taken over; i.e., it only caches a new, original form, but never updates an already cached $form_state. That, however, is one of the essential form caching improvements we added to D7, so we absolutely need to retain that.

+++ includes/form.inc
@@ -904,6 +901,15 @@ function drupal_prepare_form($form_id, &$form, &$form_state) {
+  // Populate #theme if it was not set.
+  if (!isset($form['#theme'])) {
+    drupal_theme_initialize();
+    $registry = theme_get_registry();
+    if (isset($registry[$form_id])) {
+      $form['#theme'] = $form_id;

While touching this - shouldn't this pass FALSE, so we only load, but leave the theme decision to rendering?

I'm going to run Mollom module tests with this patch (which contains the most advanced Form API tests to my knowledge), but I'm pretty confident already that they won't pass due to the $form_state cache update issue mentioned above. Thus, marking needs work. (Interestingly, this also means that this expected functionality isn't covered in core's Form API tests yet)

Powered by Dreditor.

effulgentsia’s picture

Thanks for the reviews, cwgordon7 and sun! As a reminder for anyone joining this issue recently, I would have RTBC'd this issue many iterations ago (and did in #34), but in #35, sun indicated that this needed more reviewer attention. So I'm looking for a green light from sun before RTBC'ing again.

How about putting these into Form API's semi-private 'build_info' space?

+1

Is the batch or the form rebuilt?

To be honest, I don't really understand the batch system or how this patch affects it. yched said it looked good in #24, but I have no comment on that part of this patch.

I may be mistaken, but reviewing the patch only (without applying it), it seems like only the first condition is taken over; i.e., it only caches a new, original form, but never updates an already cached $form_state.

I think you're mistaken, and that the patch just moves the second condition to within drupal_process_form(). As per #60, the point of this issue and patch is to enable AJAX submissions to process forms without calling drupal_build_form(), but consistently with non-AJAX submissions. Looking forward to finding out whether Mollom tests confirm this or discover something interesting.

While touching this - shouldn't this pass FALSE, so we only load, but leave the theme decision to rendering?

That hunk is just moving identical code from drupal_build_form() into drupal_prepare_form(). Pass FALSE to what? Are you thinking of changes in #812016: Themes cannot always participate in drupal_alter() that aren't in HEAD yet?

fago’s picture

Assigned: Unassigned » fago
Status: Needs work » Needs review

Going to take care of that today.

fago’s picture

Status: Needs review » Needs work
fago’s picture

Status: Needs work » Needs review
FileSize
25.18 KB

How about putting these into Form API's semi-private 'build_info' space?

I'm unsure about that. It's supposed to be set by *the caller* before calling drupal_build_form() or drupal_rebuild_form() - as ajax_get_form() does for us. Thus it's not really FAPI private stuff, but some flags devs may set. Additionally 'build info' is cached, but we don't want this to be cached.

>s/we need to rebuild/the form needs to be rebuilt/

fixed.

I may be mistaken, but reviewing the patch only (without applying it), it seems like only the first condition is taken over; i.e., it only caches a new, original form, but never updates an already cached $form_state. That, however, is one of the essential form caching improvements we added to D7, so we absolutely need to retain that.

I also think that's fine.

> While touching this - shouldn't this pass FALSE, so we only load, but leave the theme decision to rendering?
!? Please clarify that.. ;)

Status: Needs review » Needs work

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

fago’s picture

This doesn't fail for me, nor failed it before? -> re-test.

fago’s picture

Status: Needs work » Needs review

#74: ajax.patch queued for re-testing.

sun’s picture

Alright, I was still unhappy with the namespace of those new $form_state properties. If we cannot use 'build_info', then let's go with 'rebuild_retain' ? In attached patch.

Aside from that, RTBC.

fago’s picture

Status: Needs review » Reviewed & tested by the community

'rebuild_retain' sounds good to me - thus setting it RTBC.

fago’s picture

#78: drupal.form-ajax-rebuild.78.patch queued for re-testing.

sun’s picture

This patch is much harder to re-roll than #757154: Base form_id via hook_forms() not taken into account for #validate, #submit, hook_form_FORMID_alter(), so let's get it in first, please.

sun’s picture

effulgentsia’s picture

#78: drupal.form-ajax-rebuild.78.patch queued for re-testing.

I asked for a retest, because #78 almost certainly needs a reroll due to #757154: Base form_id via hook_forms() not taken into account for #validate, #submit, hook_form_FORMID_alter().

Status: Reviewed & tested by the community » Needs work
Issue tags: +API change, +D7 Form API challenge

The last submitted patch, drupal.form-ajax-rebuild.78.patch, failed testing.

sun’s picture

Assigned: fago » sun
Status: Needs work » Needs review
FileSize
27.86 KB

Re-rolled against HEAD.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC. This is one of the last D7 Form API challenge patches! yay! :)

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Overall this looks good, and addresses some serious WTF stuff. I highly question the timing of this patch, but since Dries committed #757154: Base form_id via hook_forms() not taken into account for #validate, #submit, hook_form_FORMID_alter() I guess I'm to infer that this one is still on the table as well.

+++ includes/ajax.inc	10 Sep 2010 15:19:25 -0000
@@ -239,6 +239,11 @@ function ajax_get_form() {
+  // When a form is rebuilt after AJAX processing, its #build_id and #action
+  // should not change.
+  $form_state['rebuild_retain']['#build_id'] = TRUE;
+  $form_state['rebuild_retain']['#action'] = TRUE;

I don't follow the logic of this name, and would have no idea from reading dsm() output that "rebuild_retain" matched this explanation.

If this is basically the same thing as $form_state['build_info'] for AJAX, why don't we name it $form_state['ajax_build_info'] ? Hop on IRC if you can this weekend and let's bikeshed this in real time.

+++ includes/form.inc	10 Sep 2010 15:23:10 -0000
@@ -275,153 +275,77 @@ function drupal_build_form($form_id, &$f
+      $form_state = array_diff_key($form_state, array_flip(array_diff(form_state_keys_no_cache(), array('always_process', 'temporary')))) + $form_state_before_retrieval;

Wow, that is a hard line of code to read. As evidenced by the fact that it has a complete novella above it trying to explain what's happening here. Can we by chance split that up a bit so it's easier to grok?

+++ includes/form.inc	10 Sep 2010 15:23:10 -0000
@@ -275,153 +275,77 @@ function drupal_build_form($form_id, &$f
+  // Now that we have a constructed form, process it. This is where:

Congratulations. :) For the first time perhaps ever, I understand how this works. Great work on the docs here.

+++ includes/form.inc	10 Sep 2010 15:23:10 -0000
@@ -489,20 +414,22 @@ function drupal_rebuild_form($form_id, &
+  $form['#build_id'] = (isset($old_form['#build_id']) && !empty($form_state['rebuild_retain']['#build_id'])) ? $old_form['#build_id'] : 'form-' . drupal_hash_base64(uniqid(mt_rand(), TRUE) . mt_rand());

We do not pay a whitespace tax. Please split this up into multiple lines so that it's remotely grokkable.

+++ includes/form.inc	10 Sep 2010 15:23:10 -0000
@@ -870,15 +835,21 @@ function drupal_prepare_form($form_id, &
+  if (!isset($form['#build_id'])) {
+    $form['#build_id'] = 'form-' . drupal_hash_base64(uniqid(mt_rand(), TRUE) . mt_rand());
+  }
+  $form['form_build_id'] = array(
+    '#type' => 'hidden',
+    '#value' => $form['#build_id'],
+    '#id' => $form['#build_id'],
+    '#name' => 'form_build_id',
+  );

Everything else here is really well commented. Could we add a line or two here as ell to explain this?

+++ includes/form.inc	10 Sep 2010 15:23:10 -0000
@@ -942,6 +913,41 @@ function drupal_prepare_form($form_id, &
+  // If #theme has been set, check whether the theme function(s) exist, or
+  // remove the suggestion(s), so drupal_render() renders the children.

Hm. That's an awful lot of belly dancing. We probably should do some benchmarks to make sure we're not slowing things down horribly. Tagging.

Additionally, can we please get a summary of how the API changes in this patch affect module developers?

Powered by Dreditor.

sun’s picture

Status: Needs work » Needs review
FileSize
28.53 KB

Resolved all review issues. Went with $form_state['rebuild_info']['copy'], as that mentally maps to 'build_info', and also leaves room to add further form rebuilding properties at some point, if required. Additionally added a short explanation for as to why this is a separate property.

Also removing "Needs benchmarks" tag, since this patch does not have any performance impact. The belly-dancing for $form['#theme'] already exists in HEAD, and for most forms throughout Drupal core and contrib, #theme is not set, so we load the theme registry with and without this patch. Lastly, that code is only moved but not introduced in this patch, but the previously committed hook_form_BASE_FORMID_alter() patch instead, so if there is an issue with that code, we'd have to re-open that issue, but I'm 100% sure that there is no performance issue.

The actual API changes are quite small and will affect a minority of developers only, who implemented custom AJAX callbacks (such as File module's custom "file/ajax" AJAX upload path/handling):

1) ajax_get_form() returns $form and $form_state only:

-  list($form, $form_state, $form_id, $form_build_id) = ajax_get_form();
+  list($form, $form_state) = ajax_get_form();

2) To process an AJAX submission, you only need to call drupal_process_form() - it will automatically figure out on its own whether it has to call drupal_rebuild_form():

-  // Build, validate and if possible, submit the form.
-  drupal_process_form($form_id, $form, $form_state);
-
-  // This call recreates the form relying solely on the form_state that the
-  // drupal_process_form() set up.
-  $form = drupal_rebuild_form($form_id, $form_state, $form);
+  // Process user input. $form and $form_state are modified in the process.
+  drupal_process_form($form['#form_id'], $form, $form_state);

There are no other notable changes to regular form processing. Both changes are visible in the patch's changes to File module.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Re-reviewed the last patch. I agree with webchick that it is well-documented. Committed to CVS HEAD. Thanks all.

rfay’s picture

Congratulations! I'll announce this one, as there may be some custom callbacks out there.

rfay’s picture

Status: Fixed » Needs work

OK, this commit broke *every* example in AJAX example that returned a piece of a form from the callback.

Wasn't the API change "only if a custom callback was implemented"?

What's the scoop here? I think every AJAX implementation is broken.

Book module is completely broken as well.

Island Usurper’s picture

FileSize
445 bytes

2) To process an AJAX submission, you only need to call drupal_process_form() - it will automatically figure out on its own whether it has to call drupal_rebuild_form()

It looks like this only happens when $form_state['rebuild'] is TRUE, but I don't see any place where this would be set in the AJAX workflow. Did someone mean to add it in, but forgot?

Island Usurper’s picture

Status: Needs work » Needs review

Oh, let's test this.

rfay’s picture

#93 fixes everything I looked at that was broken (book module, AJAX examples)

rfay’s picture

Shameless plug: If we'd had #789186: Improve drupalPostAJAX() to be used more easily, leading to better AJAX test coverage in place already (which is RTBC and waiting) we would have had tests that would have prevented this regression.

Status: Needs review » Needs work

The last submitted patch, 756762_followup.patch, failed testing.

sun’s picture

I'm trying to figure out what went wrong here. #93 seems to be correct, but it breaks some other stuff. On it.

sun’s picture

Status: Needs work » Needs review
Issue tags: +beta blocker
FileSize
12.06 KB

Spent the last hours debugging this.

effulgentsia’s picture

Status: Needs review » Needs work
+++ includes/ajax.inc	17 Sep 2010 23:08:21 -0000
@@ -238,6 +238,7 @@ function ajax_get_form() {
+  $form_state['rebuild'] = TRUE;

No! This is exactly what this issue wants to stop. Whether a form needs to be rebuilt or not is for the form to decide. Forms should be reusable between non-AJAX context and AJAX context. If a form needs to be rebuilt when submitted in AJAX context, then it likely also needs to be rebuilt in non-AJAX context. If the Book or Examples modules have forms that need to be rebuilt, and those forms aren't setting 'rebuild' to TRUE in their submit handlers, now would be a great time to fix them.

Powered by Dreditor.

Dave Reid’s picture

Is this the reason changing field formatters on a content type's 'Manage Display' tab no longer works on HEAD or do I need to file a separate issue?

fago’s picture

No! This is exactly what this issue wants to stop. Whether a form needs to be rebuilt or not is for the form to decide. Forms should be reusable between non-AJAX context and AJAX context. If a form needs to be rebuilt when submitted in AJAX context, then it likely also needs to be rebuilt in non-AJAX context. If the Book or Examples modules have forms that need to be rebuilt, and those forms aren't setting 'rebuild' to TRUE in their submit handlers, now would be a great time to fix them.

Agreed, the workflow has to be the same. However previously the rebuild happened anyway, which was wrong, but now it's even disabled by default. This behaviour makes much sense to me with buttons, as you can enable #ajax and everything works as previously + for buttons we need to have the same default for #ajax as without so that they degrade properly when javascript is disabled. However for any non button triggering elements no submit handlers are executed now - so there is no way to set 'rebuild' there (except on validate which doesn't make much sense either).

Related, when doing ajax-rebuilds with submit buttons in Rules I often had the requirement to just enable rebuild in the submit handler. For that I implemented a simple generic submit handler that does just that, however we might want to ease this common scenario a bit. Unfortunately just enabling rebuild by default doesn't play for buttons as noted above.
On the other side, this is probably what developers want if ajax is enabled, as usually you want to rebuild then + we need it that way for non-submit button triggering elements anyway. So perhaps the way to go would be enabling rebuild by default if the triggering element has #ajax enabled - but important - regardless whether the form has been submitted with ajax or not. That way the button stays working the same way if submitted with or without ajax, but we need to properly communicate to developers that #ajax magically enables form rebuilding. I'm not much in favour of introducing magic here, but at least it is consistent of how non-button + button triggering elements work with #ajax + it is that what developers usually want.

fago’s picture

Status: Needs work » Needs review
FileSize
3.81 KB

ok, here is a patch implementing the approach suggested above. In order to make it possible to disable the automatic rebuilding, I introduced 'no_rebuild' which works analogously to no_redirect.

Apart from that, I found and removed the following wrong docs:

- * if a form validation handler has set 'rebuild' to TRUE and a validation
- * error occurred, then the form is rebuilt prior to being returned,
- * enabling form elements to be altered, as appropriate to the particular
- * validation error.

sun’s picture

+++ includes/form.inc
@@ -806,11 +810,19 @@ function drupal_process_form($form_id, &$form, &$form_state) {
+  if (!empty($form_state['triggering_element']['#ajax'])) {
+    $form_state['rebuild'] = TRUE;
+  }

Let's keep this but comment it out for now -- it looks like we'd re-add the magic we wanted to remove.

I'd like to see how one of the currently broken forms is non-magically fixed. Would we be using that same code in a regular form submit handler?

Powered by Dreditor.

effulgentsia’s picture

Well, we can make non-buttons run #submit handlers. All we need to do is set their #executes_submit_callback property to TRUE. For D8, I think we should consider dropping that property and let all elements be submittable, since with AJAX and non-HTML front-ends, there's no reason to keep treating buttons and non-buttons as so different.

Regardless of what else we choose to do, this patch is what I recommend doing for Book module.

But in addition to this patch, I also think fago is suggesting something worth considering, which is to revisit our assumption about what the default should be for $form_state['rebuild'] for elements that have a #ajax property. But if we do that, I don't think we should do it in form.inc, but rather, perhaps have ajax_process_form():

1) set #executes_submit_callback to TRUE
2) add a #submit of 'ajax_form_submit_rebuild' (or some other name), which can be a function that sets $form_state['rebuild'] to TRUE if it isn't already set to FALSE.

That way, we still keep to the rule that setting $form_state['rebuild'] to TRUE is something to be done in a form submit handler.

fago’s picture

ad #104:
Hm, as explained in #102 without that the only way for non submit-buttons to enable rebuilding would be doing it in a validation handler. But that doesn't make much sense to me and is rather bad DX. As said I'm not in favour of introducing magic either, but at least this is consistent with the previous behaviour of automatic rebuilds for ajax requests (what was magic too) + it is consistent in regard to non-JS and JS submits, thus forms with AJAX enabled buttons built that way degrade properly.

>Would we be using that same code in a regular form submit handler?
Implement a submit handler that sets rebuild to TRUE.

Of course I'd be happy if you can come up with a better solution introducing less magic :)

#ad 105:
@manually setting '#executes_submit_callback':
Hm, that's not very simple to use not? ;) I think ususally it would be the best to use the 'trigger as' functionality and trigger as a button + hide the button with JS. That way the form degrades too.

@ajax_process_form:
But with that form processing for AJAX and non AJAX submit would be different again?

effulgentsia’s picture

I think ususally it would be the best to use the 'trigger as' functionality and trigger as a button + hide the button with JS. That way the form degrades too.

I agree. In fact, I think this should be THE best practice for making forms that degrade to no-js correctly. Problem is Book module adds the drop-down and the button in two different places (and doesn't add the button from book_outline_form()), but perhaps with a little refactoring, this use-case can be converted to use 'trigger as' functionality.

@ajax_process_form:
But with that form processing for AJAX and non AJAX submit would be different again?

Nope. ajax_process_form() is a #process function that runs for any element type that supports a #ajax property, regardless of whether the submission happened via AJAX.

effulgentsia’s picture

Issue tags: +Needs tests

#789186: Improve drupalPostAJAX() to be used more easily, leading to better AJAX test coverage landed, so it should now be possible to get some tests into here for Book module and any other AJAX submitted form that is currently failing. Leaving issue status at "needs review" for now to encourage more review and discussion on approach.

effulgentsia’s picture

@Dave Reid: Re #101: I found one bug related to that that is not related to this issue. Here: #896162-11: Write tests for the Field UI formatter settings form. It's possible there's other bugs with that form, maybe some of which are related to this issue.

fago’s picture

Nope. ajax_process_form() is a #process function that runs for any element type that supports a #ajax property, regardless of whether the submission happened via AJAX.

Oh, sry. Now I got your suggestion - I confused #process with drupal_process_form()...

Doing it that way make sense to me, also making any #ajax enabled element to make use of #submit handlers is useful. However there are some issues:
* We cannot add a #submit property to #submit buttons as the main submit handlers of the form wouldn't be executed any more.
* It would do it for non-submit buttons too, so there is no difference to #submit buttons any more?

Maybe it would be useful to just have a #rebuild property that sets the default value dependent of the triggering element? That way the #ajax system could just set that and the property could be used without #ajax too. This would finally make non-submit buttons more useful again, as often one does not want to submit at all but needs submit handlers just for setting $form_state['rebuild'].

effulgentsia’s picture

So, I grepped core, and the Book module is the only example that is failing as a result of the change introduced in #88. I continue to think that the Book module use-case can be improved as per #105, but that can be done in a separate issue. In the meantime, its implementation as-is serves as a nice test case, so leaving it alone for now. As rfay mentions in #92, contrib use-cases like Examples module may be broken, so improving Book module alone is not enough anyway.

I'm not totally happy with the solution that's in this patch, but I'm happier with it than with anything else proposed so far. I put a todo for D8 to address what I'm not happy with, which is the divergence in how we handle submit buttons from everything else, and all the complexity that causes. But I don't think it's wise to change that in D7.

This also cleans up the docs error fago points out in #103, and some other stray code that has $form_state['rebuild'] = ... unnecessarily, confusingly, and in contradiction with the new docs.

I believe this fully fixes the regression introduced in #88 for non-buttons, but in a way that keeps form processing the same between a js and nojs submission, to keep facilitating error-free degradation when JS is disabled.

Assuming this patch meets with reviewer approval, a test should be added for the book module's outline form that fails in HEAD and passes with this patch. Anyone willing to do that?

Status: Needs review » Needs work

The last submitted patch, 756762-fapi-rebuild-111.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
9.88 KB

Status: Needs review » Needs work

The last submitted patch, 756762-fapi-rebuild-113.patch, failed testing.

effulgentsia’s picture

I fixed the failures by changing the tests. This is great! It means we had test coverage for an obscure feature, and can now consciously decide whether it's okay to drop that feature. The obscure feature dropped by this patch is the ability to submit a form with a 'button' (as opposed to a 'submit' or 'image_button'), and to have the behavior be that submit handlers don't run and the form is not rebuilt. This patch (along with #113) include the following comment:

+  // ... However, for forms that aren't fully executed
+  // (for example, submissions triggered by elements of the 'button' type rather
+  // than the 'submit' type, and for AJAX submissions triggered by non-buttons),
+  // there is no opportunity for a submit handler to set $form_state['rebuild'],
+  // but it makes no sense to redisplay the exact same form without an error for
+  // the user to correct, so we also rebuild error-free non-executed forms
+  // regardless of $form_state['rebuild'].

I continue to believe this, and therefore, believe that it's okay to drop that test. It's bad enough we have divergence between "submit and image buttons run submit handlers and don't rebuild by default" and "other elements don't run submit handlers but rebuild". Why have a 3rd code flow of "and buttons of type 'button' don't run submit handlers and don't rebuild, unless they also have a #ajax property"? That's the kind of stuff that keeps FAPI an impenetrable mess. I don't think that 3rd case is ever useful, so I'm okay dropping support for it. Note: there is no core code that uses #type => 'button', and I'm not aware of any use-case for it in contrib. Not to say there isn't some contrib module out there that uses a #type => 'button' and wants it to redisplay an error-free form without it being rebuilt, just that I can't think of why someone would want that.

effulgentsia’s picture

Status: Needs work » Needs review

.

sun’s picture

+++ includes/ajax.inc	23 Sep 2010 02:13:00 -0000
@@ -26,14 +26,33 @@
+ *     drupal_process_form() to processes the form submission and rebuild the

s/processes/process/

+++ includes/form.inc	23 Sep 2010 02:13:00 -0000
@@ -809,8 +821,21 @@ function drupal_process_form($form_id, &
+  if (($form_state['rebuild'] || empty($form_state['executed'])) && $form_state['process_input'] && !form_get_errors()) {
     $form = drupal_rebuild_form($form_id, $form_state, $form);
   }

It looks like this entire condition could be moved into the existing condition for 'process_input', right above it.

+++ modules/simpletest/tests/form.test	23 Sep 2010 02:13:01 -0000
@@ -727,24 +727,21 @@ class FormsFormStorageTestCase extends D
-    // Reload the form, but don't rebuild.
-    $this->drupalPost(NULL, $edit, 'Reload');
-    $this->assertText('Form constructions: 2');

I agree with you that buttons should not have the obscure behavior of not submitting the form. I always asked myself what that behavior is actually good for.

Powered by Dreditor.

fago’s picture

Generally +1 to that approach. Makes much sense to me.

I agree with you that buttons should not have the obscure behavior of not submitting the form. I always asked myself what that behavior is actually good for.

I'm glad to see I'm not the only one who missed the use case for that ;). +1 to clean that up and simplify things.

+ *   - After form processing is complete, ajax_form_callback() calls the
+ *     function named by #ajax['callback'], which returns the form element that
+ *     has been updated and needs to be returned to the browser.

It may also return some AJAX commands not?

+ *     done or requires another step. A validation handler may set
+ *     $form_state['rebuild'] to cause the form processing to bypass the submit
+ *     handlers and rebuild the form instead. However, there is no use-case for
+ *     this in Drupal core, because a form is only rebuilt if it is free of
+ *     validation errors, and it is rare for a validation handler to want to
+ *     prevent submit handler execution in the absence of a validation error.

Hm, so this tells me there is something we don't need or rarely need? Not very helpful. Instead it should tell me when I want to use it or for what it might be needed.

Thinking about that it could be used to implement custom form validation that shows a changed form when validation fails, e.g. I could think of mollom using it to conditionally show the captcha via a form rebuild.

ad #117; 2)
No, the elseif () has to run even if $form_state['process_input'] is FALSE.

sun’s picture

Incorporated all feedback. I don't think we need to write about use-cases. Only the technical details matter, i.e., what happens and what not, and possibly also why.

Status: Needs review » Needs work

The last submitted patch, drupal.form-ajax-rebuild.119.patch, failed testing.

rfay’s picture

sun’s picture

Status: Needs work » Needs review
FileSize
13.05 KB

ok, this effectively means that we need to improve/adjust the if/elseif conditions, as well as their documentation. It doesn't map.

Status: Needs review » Needs work

The last submitted patch, drupal.form-ajax-rebuild.121.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
13.17 KB

I think that attached patch gets us back more in line with the original clean-up intentions.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Alright, tests passed.

Since I merely fixed some comment wording and incorporated feedback into the patch, and since this patch currently prevents us from making progress in a couple of other critical issues, I'm going to mark this patch RTBC now. We can and should add advanced AJAX form rebuilding tests in a non-critical follow-up patch, but right now, it is important to get a beta out of the door.

sun’s picture

Without this patch, most AJAX functionality in HEAD is unusable. This patch also blocks testing of #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework

Dries’s picture

Status: Reviewed & tested by the community » Fixed

I stared at this patch for 45 minutes and it looks alright. Admittedly, I'm not sure I understand all the nuances but decided to move forward with this nonetheless. This issue had all the form API people involved, and it will hopefully allow us to make progress on some of the other problems/issues mentioned. Committed to CVS HEAD. Thanks all.

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests, -API change, -D7 Form API challenge, -beta blocker

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