Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#17 | interdiff.txt | 1.16 KB | David_Rothstein |
#17 | autosave-all-values-1908264-17.patch | 3 KB | David_Rothstein |
#15 | interdiff.txt | 1.43 KB | David_Rothstein |
#15 | autosave-all-values-1908264-15.patch | 2.37 KB | David_Rothstein |
#13 | autosave-all-values-1908264-13.patch | 1.77 KB | jrb |
Comments
Comment #1
ezheidtmann CreditAttribution: ezheidtmann commentedThis issue persists in 7.x-2.2.
Comment #2
DamienMcKennaThere are two parts to this bug:
Comment #3
fabsor CreditAttribution: fabsor commentedWe 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.
Comment #4
fabsor CreditAttribution: fabsor commentedWe 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.
Comment #5
fabsor CreditAttribution: fabsor commentedComment #6
gnaag CreditAttribution: gnaag commentedIt would be great. Now using field collections, the first item saved well, the others are lost.
Comment #7
jrbHere'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).
Comment #8
czigor CreditAttribution: czigor commentedThe 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?
Comment #9
matthensley CreditAttribution: matthensley commentedis 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?
Comment #10
czigor CreditAttribution: czigor commentedThe 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.)
Comment #11
matthensley CreditAttribution: matthensley commentedWhat about doing this:
Before doing drupal_build_form on the restore call:
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.
Comment #12
cpsarros CreditAttribution: cpsarros commentedAny progress on this? Can someone provide a patch for the current dev?
Comment #13
jrbHere's a re-roll of the patch in #4 and #7 to work with the latest dev version (2016-Jan-19).
Comment #14
alimbert CreditAttribution: alimbert commentedHas anyone tied #13 yet?
Comment #15
David_Rothstein CreditAttribution: David_Rothstein at Tag1 Consulting commentedThe 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.
Comment #16
czigor CreditAttribution: czigor at Liip for FREITAG lab. AG commented@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.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedWell 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:
DELETE FROM cache_form;
- or wait for 6 hours, if you really want to :)Comment #18
czigor CreditAttribution: czigor at Liip for FREITAG lab. AG commentedAh, 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.
Hmmm... I thought multistep/AJAX forms get the part of the form values from the cache. Is this not true?
Comment #19
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedOh, 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 :)
Comment #20
sumthief CreditAttribution: sumthief as a volunteer and at DrupalJedi commentedPatch from #17 works good form me. Need someone else to test it and then we can go RTBC this issue.
Comment #21
majorrobot CreditAttribution: majorrobot at Major Robot Interactive commentedI 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!
Comment #22
sumthief CreditAttribution: sumthief as a volunteer and at DrupalJedi commented