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.
bot, run!
Comment | File | Size | Author |
---|---|---|---|
#46 | form-state-persistence9.patch | 10.25 KB | fago |
#44 | form-state-persistence8.patch | 10.25 KB | fago |
#35 | form-state-persistence7.patch | 9.25 KB | fago |
#32 | form-state-persistence6.patch | 9.01 KB | fago |
#30 | form-state-persistence5.patch | 8.43 KB | fago |
Comments
Comment #1
fagoComment #2
fagoComment #3
fagoCurrently only build_info and storage stays alive if you have a form workflow going through multiple steps. Making the whole $form_state staying alive would help solving #622922: User input keeping during multistep forms and #634984: Registration form breaks with add-more buttons. Also imo the name already suggests that the $form_state persists during the whole form workflow.
The above patch already implements that, but if we really decide to go that way I think it would make sense to
* eliminate the storage key as it's not needed any more
* introduce $form_state['build'] or so, which keeps volatile data only available during one build. Then move form API internal build related stuff in there like 'validate_handlers', 'buttons', .. That way form elements also have a space where they can put volatile data into.
Apart that we would have to think which form_state values should persist and which not. The patch above imitates the current behaviour of not persisting them at all. However I wonder, what is if a form constructors sets 'no_redirect'? Once the form was retrieved from cache it's gone, so persisting that would make imo sense. Further I think even 'rebuild', 'redirect', 'no_redirect', 'cache', 'no_cache' would make sense to persist too. (OK we don't have to care about 'no_cache' anyway ;).
Comment #4
fagoAlso the $form_state is quite useless as of know, as its content may be lost any-time due to a possibly activated cache. So let's make it actually useful again.
Comment #6
sunI now identified the need for this in #644150: $form_state is stateless, even with enabled caching.
But we probably still want only a selected subset, or, white-list. Reversing it into a blacklist still sounds too much for D7 to me.
Comment #7
fagoTalked with chx about that. So we'll move the form API internal flags into $form_state['build'] - which we don't cache. Flags related to modules like 'rebuild' and 'redirect' stay outside to keep the API. That way we also have a good distinction between internal and non-internal stuff.
I'll tackle that as soon as #622922: User input keeping during multistep forms landed.
Comment #8
sunAs far as D7 is concerned, I don't think that a patch that requires developers to update their code from $form_state['rebuild'] to $form_state['build']['rebuild'] (or whatever) will be in the loop.
For D7, I guess that only the white-list approach, or, shifting/copying around $form_state properties before/after getting/setting the cache will be possible.
I.e.
Comment #9
fago>Flags related to modules like 'rebuild' and 'redirect' stay outside to keep the API.
So that's why we want to keep them as where there are except for internal stuff - that way we have internal stuff separated and the API stays.
@build_info:
I'd use 'build' not 'build_info' for the internal stuff related to *one* build, thus it doesn't need to be cached. In contrast to 'build_info' which is info required to rebuild the form, which needs to be cached.
Comment #10
sunPerhaps we're talking past each other. Can you clarify what exactly you want to do? Either an example snippet like I did in #8 or a conceptual patch that will perhaps fail would be great.
Comment #11
yched CreditAttribution: yched commentedThis *might* clash with #629794: Scaling issues with batch API - which I really wish got committed ;-).
From the OP over there:
From the patch over there:
Comment #12
fago@yched: I see. We'd just have to adapt it to follow the pattern what's kept if this would get it in earlier. Should be no problem.
@sun: I did a test-case which fails without this. It creates a simple form which uses the $form_state to pass data from the constructor to a handler. Once the form is altered() and gets cached because of new elements triggering caching, the handler break because $form_state isn't cached.
chx suggested to move the internal stuff into $form_state['internal'] which then is not cached. Well I think the name doesn't reflect that this is stuff related only to the current build, but I'm fine with it either.
Patch containing only the test attached.
Comment #14
fagoComment #15
fagoComment #17
fago* Updated original patch to move 'internal' build related properties into $form_state['internal']. That way the $form_state is cleaned up to separate 'internal' properties and the exclusion list is much shorter now. Where would be a good place to document $form_state['internal']?
* Also added the above test-case to the patch, which should now pass.
Comment #19
fagoExecuting button handlers were broken, fixed that and made sure to get all replacements. $form_state['clicked_button'] is sometimes used to read out values from the clicked button, perhaps it shouldn't go into 'internal'? Opinions?
Comment #21
fagoPatch should use -p0 ..
Comment #23
fagoComment #24
fagoDiscussed with chx again -> I changed the patch to be more cautious with what moves into 'internal'. Remaining stuff moved into it are "method, groups, buttons, complete form and process_input" => patch size halved! Still we have an internal place where processing elements can put any internal stuff that shouldn't be cached.
Comment #25
fagohttp://drupal.org/node/644222#comment-2326118 is a good real-life example of why we need this: Usually forms rely on $form_state not being lost.. ;)
Comment #26
sunThis is too much for D7.
We can fix this internally in form_set_cache() and form_get_cache().
This should be added to the FormsRebuildTestCase() instead, if required at all.
Do we really need a new test form for this?
#ajax is #process, not _alter.
I'm on crack. Are you, too?
Comment #27
fagoWhat's wrong with the extra form? It's an easy to grasp form that should demonstrate the problem. Also the form doesn't use rebuilding, so it doesn't fit into the rebuild test case.
#ajax gets processed yes, but it might be added to a form via _alter.
Comment #28
fagoOk, I updated the patch don't move around any internal value at all. I added $form_state['build'] though, which may be used by form elements when they want to temporary store data somewhere. Also I fixed the naming of the testcase.
Comment #29
sunI'm not sure why this bug still exists, it should have been part of #622922: User input keeping during multistep forms.
We need to move 'args' into 'build_info'. Otherwise, drupal_build_form() will throw notices in certain situations.
This info belongs into Doxygen of drupal_build_form(), where also 'storage' needs to be updated.
Instead of 'build', we should use a more precise term here. 'temp' or 'temporary' would sound more appropriate to me.
Can we group these after "public" and internal properties?
public: cache, must_validate, no_redirect, etc. - basically all that are mentioned in PHPDoc of drupal_build_form().
internal: all the others set by form building and input handling.
That would make more obvious that we missed no_cache here, for example. Perhaps more.
On that note though, we also need to discuss whether 'cache' shouldn't be cached, too. I think I already added a @todo to form_test.module that complains about the property not persisting.
I'm on crack. Are you, too?
Comment #30
fagoI addressed those issues. I left out 'no_cache' as when it's there we won't cache anyway ;) However I added it now, as maybe this might make sense to not cache it other situations as when batch API caches form state (see yched's comment above) for which we then can reuse the defined keys.
I left out 'programmed' and 'wrapper_callback' - as those can't change during the lifecycle anyway.
@caching 'cache': Let's deal with that in #648170: Form constructors cannot enable form caching or form rebuilding. Once this is in it should be easy.
Comment #31
sunI think we need to move this information above the documented special properties now, so developers know that $form_state is/can be cached and what exactly is cached (and not).
Forgot to rename?
Thanks! Can we add an inline comment above 'cache' and 'buttons' to denote the two property groups?
Good catch! That makes totally sense. :)
Please add this to class FormsFormStorageTestCase instead. (we probably also want to rename that test case, but we don't necessarily need to do that in this patch)
This title could use some more love.
We actually should stop defining types. Makes it easier to unhide the test module and manually access the test pages in Navigation menu.
This review is powered by Dreditor.
Comment #32
fagoThanks for the thorough reviews!
>We actually should stop defining types. Makes it easier to unhide the test module and manually access the test pages in Navigation menu.
Agreed, however as of now all forms use CALLBACKs - thus I didn't change that for consistency. Rolling a small separate patch that changes all of them makes sense though.
I addressed everything else and re-rolled. Also I moved 'clicked_button' up to the publicly used properties.
Comment #33
Dries CreditAttribution: Dries commentedCan we use 'temporary' instead of 'temp'? From the documentation it is not clear how temporary temporary data is? Does it persist across page requests? From the documentation, it is also not clear when you're supposed to use the temporary data. In other words, the documentation is a bit light.
Comment #34
sunI agree with Dries that we want to connect this info to form caching (persisting across requests), but also state that $form_state stays intact during the same request in case a form is rebuilt.
I think we should call them
"Public properties defined by form constructors and form handlers."
and
"Internal properties defined by form processing."
'clicked_button' definitely belongs to the internal properties. I think the public list should really equal the Doxygen of drupal_build_form().
I'm on crack. Are you, too?
Comment #35
fagoWell I think we can discuss a lot what's internal and what not, because this is rather unclear. I like your suggested comments, they make a better distinction.
@Dries:
Ok, I changed it to use 'temporary' and tried to improve the docs. I used "current page request" and "during whole form workflow" to describe how long the data is cached. I hope that's clear now?
Comment #36
sunuhm, across page requests? That is only true when form caching is enabled and when form rebuilding is disabled.
This review is powered by Dreditor.
Comment #37
fagohm. When a form rebuilds, it will do caching and so the state will persist. But true the only case when it won't persist is when form caching isn't activated initially, then it won't be persisted from the first load of the form to the next page request. But usually that's fine as then the constructor runs again on the next page request and so the data is populated again, thus the data will be always there!
To be exact we could write something like:
"The data put into $form_state is persisting across page requests once the form rebuilds or caching is activated, thus it can be accessed and used during the whole form workflow."
However that's more complicated and boils down to:
"put the data in $form_state and it will be there later on"
Comment #38
sunWell, this:
"put the data in $form_state and it will be there later on"
means to me: The data I put in there, which I may have altered in a #process or #validate will be contained as is.
But that's not necessarily the case, because form caching needs to be enabled. Form rebuilding will contain the same data during the rebuild, but not across page requests. That is, because we don't re-use the $form_build_id when invoking drupal_rebuild_form(). And that said, that's one of the most awkward WTFs in Form API, IMHO.
Comment #39
fago? It doesn't matter that the build id changes as the new build id will then be used to retrieve the cache. As chx has pointed out it's important that it changes, so the cache doesn't get confused if a user presses back in the browser... But the $form_state or without this patch the form state storage stays alive particular during rebuilds, test it if you don't believe me ;)
>>"put the data in $form_state and it will be there later on"
>means to me: The data I put in there, which I may have altered in a #process or #validate will be contained as is.
It will be. The only thing is that when you don't have form caching enabled you setup $form_state twice. Once during the first page load, then when you submit the initial form the first time you setup the data a second time - but as you just have set it up again it's there! If caching is enabled, it will be cached immediately.
After that it's going to be cached if the form workflow proceeds, so the data will be there.
Comment #40
sunI don't believe you, since I had enormous WTF moments with form caching/rebuilding recently. Ideally, this should be covered tests. But that doesn't belong into this issue.
So let's just clarify that comment and make it more precise regarding cross-request persistence, based on the assumptions we currently have, and be done with this issue. ;)
Comment #41
fago>"The data put into $form_state is persisting across page requests once the form rebuilds or caching is activated, thus it can be accessed and used during the whole form workflow."
Well I don't think we need to be that exact, I think the current text of the patch in #35 is currently better:
Explaining the differences between cached and non cached form workflows goes imho to far at this place. Documentation like this probably better goes into a handbook page with graphics showing the different workflows across multiple page requests.
Comment #42
fagoSo I just documented the form workflow across page loads, hopefully this helps more people to understand it: See http://drupal.org/node/650016
Comment #43
sunWe don't need to be lengthy, but the information is absolutely required for the PHPDoc of $form_state, because this is essential API documentation. Anyone who wants to change the API needs to be able to understand how it is intended to work.
So I suggest to change this to:
Furthermore, it may be used to store information related to the processed data in the form, which will persist across page requests when the 'cache' or 'rebuild' flag is set.
(also note trailing white-space here)
Flipping these two would make sense.
Trailing white-space here.
Missing trailing comma.
Wrong indentation and missing PHPDoc.
s/Implement/Implements/
I'd like to see the 'cache' and 'rebuild' flags tested separately, i.e. conditionally.
Just setting 'cache' => TRUE in this alter hook should make the $form_state persist. You may also need to trigger a validation error to properly test this.
Just setting 'rebuild' => TRUE in the submit handler should make the $form_state persist as well.
This review is powered by Dreditor.
Comment #44
fagook, I addressed all your comments and improved the test to test
- simple single-submit usage
- validation failure + submit again
- rebuilding (submit 2 times)
both with and without caching enabled.
As I said it's working now ;)
>I don't believe you, since I had enormous WTF moments with form caching/rebuilding recently. Ideally, this should be covered tests. But that doesn't belong into this issue.
Then just create a simple test-case for this WTFs - then we can see what's wrong and squash those bugs. The only WTF-issue I can see left once this is in is #648170: Form constructors cannot enable form caching or form rebuilding.
Comment #45
sunIt is unfortunate and stupid and I'm totally sorry to have to request another re-roll, but as explained in http://drupal.org/node/1354, newlines are NOT supported within @param by the API parser.
This review is powered by Dreditor.
Comment #46
fagoNo big deal, I'm already used to it... ;)
Comment #47
sunheh, thanks! ;)
Comment #48
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #51
scottstone72 CreditAttribution: scottstone72 commented46: form-state-persistence9.patch queued for re-testing.
Comment #53
mr.baileysSeems that this issue was re-opened by mistake. Closing again.