Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ezheidtmann’s picture

This issue persists in 7.x-2.2.

DamienMcKenna’s picture

There are two parts to this bug:

  • Ensuring the correct fields are available on the form; this is quite complicated as it'll require submitting the "add another" button to make FormAPI properly have the correct number of fields.
  • Ensuring filling in all fields works correctly.
fabsor’s picture

Issue summary: View changes

We will have to rework the module significantly if we want to fix this I'm afraid. The best way would be to save more information, not just the values but save the actual state of the form. when autosaving.

fabsor’s picture

We could actually get away easier than I thought if we just use the ajax helper function to get the form and save the whole form state. A big bonus for this approach is that we could remove a lot of custom hacks.

fabsor’s picture

Status: Active » Needs review
gnaag’s picture

It would be great. Now using field collections, the first item saved well, the others are lost.

jrb’s picture

Here's a re-roll of the patch in #4 to work with the latest dev version (2014-Jul-10).

This is working for me, BTW (original patch in #4 and this re-roll).

czigor’s picture

Status: Needs review » Needs work

The idea is brilliant, however ajax_get_form() gets $form and $form_state using form_get_cache() and the form cache expires after 6 hours. I am not sure about imposing such a limitation on autosave.

This could be worked around by creating a cache identical to the form cache except that it does not expire.

What are your thoughts on this?

matthensley’s picture

is the implication here that the saved data would no longer be valid after 6 hours, or just that should the form remain open for over 6 hours autosave would no longer be able to pull the necessary data?

czigor’s picture

The implication is that 6 hours after caching the form it would be impossible to restore the autosaved form.

(Independent of autosave: Drupal cannot submit a form that is open longer than 6 hours because the form cache is needed for submission.)

matthensley’s picture

What about doing this:

Before doing drupal_build_form on the restore call:

<?php 
$form_state['input']['form_token'] = drupal_get_token($form_id);
?>

Essentially setting a new fresh token on the state you're about to restore so that it gels with the current session?

I may be way off base, as I'm still pretty new to this stuff, but I was just able to save a restore that is days old after implementing. If this doesn't sound totally insecure / broken, I can figure out how to make a patch, but I thought I'd bounce the idea here first to see if it makes sense logically.

cpsarros’s picture

Issue summary: View changes

Any progress on this? Can someone provide a patch for the current dev?

jrb’s picture

Here's a re-roll of the patch in #4 and #7 to work with the latest dev version (2016-Jan-19).

alimbert’s picture

Has anyone tied #13 yet?

David_Rothstein’s picture

Title: Autosave adds only the first value in a multiple value field » Autosave adds only the first value in a multiple value field and doesn't work with many forms that rely on Ajax or $form_state
Version: 7.x-2.1 » 7.x-2.x-dev
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
2.37 KB
1.43 KB

The patches in this issue fix much more than what the title said :)

However they rely on ajax_get_form() returning something from the cache, which won't always happen since not all forms are cached in the first place. For example, the patch in #13 breaks autosave on the Basic Page content type on a standard install of Drupal 7 core.

Probably the best way to fix that is to force all autosaved forms to be cached (just like Drupal core forces all Ajax-enabled forms to be cached).

The attached version does that, and also removes one more bit of code that doesn't appear to be necessary now that the module is storing all of $form_state.

Seems like the overall approach here is good, although if you deploy this to an existing site then it will presumably break any autosaved data that's already in the database.

Regarding #8-#10, it may be true that some Drupal forms (certainly not all forms) won't submit correctly if they have disappeared from the form cache, and therefore autosave won't work for those forms if more than 6 hours have passed. But isn't that a preexisting problem? I don't see how it's related to this issue.

czigor’s picture

@David_Rothstein It's related because that would apply the 6 hours limitation even to uncached forms. This would kind of beat the purpose of the module in many cases.

David_Rothstein’s picture

Well if a form is uncached in the first place then it's not going to be relying on anything in the form cache in order to submit correctly... So the 6 hour limitation shouldn't apply.

(And as far as I know most cached forms don't rely on the form cache in order to submit correctly either. Why do you think that they do?)

But actually testing that out I noticed a problem; to support the above scenario, something similar to the 'include_file' code I removed in #15 is needed after all. (Because normally form_get_cache() takes care of that, but it won't if there's nothing in the cache.)

So with this new patch you can try it out like this:

  1. Turn on autosave for basic pages and go to node/add/page on a standard Drupal 7 install.
  2. Make some changes to the form but don't save them; wait until autosave kicks in.
  3. Navigate somewhere else.
  4. In the database, DELETE FROM cache_form; - or wait for 6 hours, if you really want to :)
  5. Go back to node/add/page, restore the autosave data, and submit. Everything should work fine.
czigor’s picture

Ah, ok, now I get it. ajax_get_form() is called only during autoform save, not autoform restore. Then this should be ok. Vow.

The only issue we might have is with BC. But this might be worth a new major release.

(And as far as I know most cached forms don't rely on the form cache in order to submit correctly either. Why do you think that they do?)

Hmmm... I thought multistep/AJAX forms get the part of the form values from the cache. Is this not true?

David_Rothstein’s picture

Hmmm... I thought multistep/AJAX forms get the part of the form values from the cache. Is this not true?

Oh, they do. I thought you were talking about the final form submission at the end. I am not sure of the details to be honest, but my guess is that a typical multistep form will fail badly if the form cache entry disappears, but a typical Ajax form will do better (can probably still submit the form itself at that point, but maybe can't do additional Ajax updates).

And also that with autosave, this patch won't make any of those scenarios worse, and in some cases might even make it better (since we effectively are passing in a copy of $form_state that is the same as what would have been in the form cache but went missing from there). But this could probably use some testing :)

sumthief’s picture

Patch from #17 works good form me. Need someone else to test it and then we can go RTBC this issue.

majorrobot’s picture

I can also confirm #17 works. Also tested the interaction with the form cache, as per instructions in #17, and everything works as it should.

This would be a great feature to have in a new release!

sumthief’s picture

Status: Needs review » Reviewed & tested by the community