[meta issue] Major Form API/Field API problems

Short reminder for people new to those issues: the issue with Fields in forms is that any form that wants to include fields needs to be multistep, because of the 'add more' button.

Am I right in stating that the solution for #367006: [meta] Field attach API integration for entity forms is ungrokable will rely on standardizing the use of $form_state for 'entity' forms - or more generally for forms that want to include fields, but that in order to move forward we need to clarify how $form_state and its subparts behave regarding form workflow and cache / rebuild behavior ? Or is this more intricated that this ?

Awesome. Here's my recommendation for how to proceed: first, let's remove all the magic triggered by $form_state['storage'] and just have it be what it says: a place to store information, either within or across page requests of a form workflow. Does #634440: Remove auto-rebuilding magic for $form_state['storage'] accomplish all of that, or is there weird caching magic that also needs solving in #583730: How many ways are there to cache a form? before $form_state['storage'] can be used without side effects?

Then, let's discuss fago's idea of using $form_state['storage'] as THE place from which form constructor functions populate default values. If that flies, it will solve #622922: User input keeping during multistep forms. If it doesn't fly, we'll need to continue on that issue until we have a workable solution.

After that, I think we'll have a framework in place for tackling the remaining issues.

Cross-posted with yched, but yes, I think the problem being tackled in #622922: User input keeping during multistep forms is more generic than just FieldAPI. It should be possible to add "add more" buttons that work with and without js to ANY form. And I think the solution will require either something along the lines of what's being proposed in that issue or fago's recommendation to build all forms using $form_state['storage'].

Once that's in place, I think #367006: [meta] Field attach API integration for entity forms is ungrokable can be focused on FieldAPI specific details regarding entities.

I certainly agree with effulgentsia that magic behavior based on $form_state['storage'] should be eliminated. This has been a long-term hazard.

Thanks sun for providing this overview! Indeed a lot of issues..

Well as already mentioned I think we should start using $form_state['storage'] as the place for holding data regardless it's a multistep form or not. Then it can become one anytime - this should fix mostly all of this issues. See http://drupal.org/node/622922#comment-2276812 for a more detailed post why I think this is the right solution.

>Does #634440: Remove auto-rebuilding magic for $form_state['storage'] accomplish all of that, or is there weird caching magic that also needs solving before $form_state['storage'] can be used without side effects?

Simplifying the caching logic is a separate thing, however then another possible problem is left. The form constructor is called differently when the form is set to be cached - which is done automatically once there is #ajax stuff in it or once the form is rebuilt (then caching is activated one step later).
So this might cause problems when the original form relies on the form constructor to run to put stuff in the $form_state for it's handlers (without storage) or includes files. Ideally the form would always behave the same way here too.
To achieve that we could activate $form_state['cache'] by default. However then we would have to ensure that the cache isn't overwhelmed with useless stuff from forms appearing on nearly any page like the user login block or search form. Setting 'no_cache' for those forms would be a possible way to go. Or we just stay with this difference and try to educate developers to write their forms right using form storage to pass around things and/or develop with $form_state['cache'] enabled.
So keeping values when rebuilding would solve the problem for the form values, but still the form behaves different thus forms relying on the form constructor to include or put stuff into formstate without storage would break once 'cache' is activated.

It seems to be clear that we need to remove the magic from 'storage' to properly proceed with other patches.

form_state['storage'] triggers caching because you only want to use form_state['storage'] to persist state from one step to another. So if you have storage then you surely wanted to rebuild. I am not exactly sure whether this is the magic you want to remove and if yes then what we would achieve.

I want to mention #386678: Using/setting form_state['storage'] in one form on a page prevents any other form on page from posting ($_POST is destroyed) here. This is a D6 issue, and I have tested but not studied the issue on D7 and it seems not to manifest itself in the same way in D7 (or to have been resolved completely). However, it's an example of the kind of magic that bites you when we're done. I'll plan to write a test for this case.

Let's make a commitment that each of these edge cases/problem issues gets good tests before being committed.

>The form constructor is called differently when the form is set to be cached
When caching is activated the form isn't constructed again when the user submits its data. Thus when the form isn't cached it's working to put data in $form_state without storage so that validate/submit handler can access. Once the form is cached, the constructor isn't run before and $form_state misses the values the handlers rely on. They same problem may occur when the constructor is including files needed later on...

@Whether it is a good idea to also remove the caching magic when 'storage' is populated
When there is no rebuild, nothing will be cached. The rebuild is really triggering the caching (in regard to storage), so the patch already removes this magic by removing the rebuild. The "magic" left is that a rebuild enables caching. But that's necessary so that the storage is available during multiple steps/rebuilds.

subscribe

@rfay: Re #10: That particular problem has been fixed for D7 by using $form_state['input'] instead of $_POST, thereby preventing a rebuild of one form from having an affect on another form on the same page. However, the rest of what's been said in relation to #634440: Remove auto-rebuilding magic for $form_state['storage'] is true, and I'm glad it landed.

new idea:
* http://drupal.org/node/622922#comment-2300826
* -> #641356: Cache more of $form_state when form caching is enabled

comments wanted! :)

>I'm not sure what we gain from the patch in #641356: $form_state should stay alive during the whole form workflow, because it is only changing how the values in $form_state are cached, and, I'm rather opposed to do such a change at this stage of the release cycle.

"better backward compatibility" - we don't have to move everything into storage but we can stay using variables already in $form_state, e.g. $form_state['node']

>I'm rather opposed to do such a change at this stage of the release cycle.

I agree it's late for those changes, but I think that's a problem for each solution fixing the problem of #634984: Registration form breaks with add-more buttons in general - thus making forms not break when altering it to be multistep.

Also this fix works fine with every single form in core - that's because it makes $form_state working as it's expected by devs anyway. You put data in there and expect it to be still there later on.

Subscribing. I'm trying to develop an entity module right now and running head first into FAPI WTF land. There's stuff going on here that I can't even begin to comprehend, mostly the workflow of the $form['#builder_function'] stuff.