Spin-off from #597108: Consistent array key syntax in $form_state (underscores vs. spaces):
1) hook_forms() is "almost" a form registry, but only invoked in case a $form_id does not exist as function.
2) #571086: Allow to specify a 'wrapper callback' before executing a form builder function will allow to define a "wrapper callback" for a $form_id in hook_forms(). For that to work, the $form_id must not exist as callable function, so hook_forms() is invoked at all.
3) #93587: Using drupal_not_found() in "pulled" forms results in double 404 message identifies a common problem: Anyone and anything can invoke drupal_get_form($form_id) and there won't be any access checks. And there is no way a form builder could tell that access is denied (403) - or a form wasn't built at all (404).
4) #367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info'] introduced $form_state['include_file'], which is required to rebuild a form from cache on a different path. Currently, that value is auto-determined based on the menu router item of the current path - but that certainly is very limiting.
5) Forms should be treated as a representation of a certain object. node_view() displays a node, node_form() is the same node, but represented in an edit form.
I believe we could solve many problems by introducing and requiring hook_form_info() for all forms:
/**
* Implement hook_form_info().
*
* This is almost the same as hook_forms(), but of course, without $form_id.
*/
function hook_form_info($args) {
$forms['node_form'] = array(
'form callback' => 'node_form', // optional?
'form arguments' => array('static arg'), // Like current hook_forms(), prepended to passed $args.
'wrapper callback' => 'node_form_wrapper', // optional.
'access callback' => 'node_edit_access',
'access arguments' => array($args[0]),
'file' => 'node.pages.inc',
// ...more?
);
return $forms;
}
Comments
Comment #2
sunComment #4
sunSee also #597108: Consistent array key syntax in $form_state (underscores vs. spaces)
Comment #7
joachim CreditAttribution: joachim commented> 5) Forms should be treated as a representation of a certain object. node_view() displays a node, node_form() is the same node, but represented in an edit form.
That's an interesting idea. I'm not sure what benefits it gives us, but it sounds good :)
How would you represent that in hook_form_info / hook_entity_info?
Comment #9
aidanlis CreditAttribution: aidanlis commentedMoving toward standard CRUD helpers for objects makes sense, both at the database level and in the UI.
Comment #10
fagoSubscribing + agreed that we need something like that.
Some issues I ran over with the current API:
1. There is currently no way to know which include file to include to make use of a form as this info lives in hook_menu(). This makes cleanly re-using forms quite impossible without having extra-knowledge.
We should decouple the include-loading part from the menu system so forms can be rendered and processed on its own.
2. In case you want to re-use a form it's fun that you cannot go with the base-form id. You need to find out the specific form-id (i.e. incorparting the entity bundle) first, however the used pattern may differ so you cannot generically know it. Then, once you figured the specific-form id out the system has to figure out the base-form-id for you first, so it can find the implementation.
Thus, it's an unnecessary crux to have to use the specific form-id when calling drupal_get_form(). Instead we should go always with the base form id for identifying a form and let the form implement-er come up with the specific form id afterwards.
Comment #11
eaton CreditAttribution: eaton commentedSun, thanks for jumping onto this. It's something that's been debated in back-channelly discussions for a number of years now, ever since hook_forms() made its way in in D5 as a workaround. Short version? I'm very much in favor of it. I'm not familiar enough with some of the config management decisions that have been made to know whether info hooks are the right pattern for this going forward, but that approach (or the modern drupal equivalent) would be a big improvement.
The current magic-naming Form definition mechanism we use is a holdover from the "hooks are everything" days of Drupal 4.6, when FAPI was originally created. There was a great deal of concern that forcing people to explicitly define their forms would receive a ton of pushback. The result was a large number of magic function names. Over time we realized that we needed more explicit specifications, and those were grafted on top of the existing magic naming conventions -- submit and validate hooks, form building functions, etc. In my experience, it's a real point of confusion for people who are learning the API.
I'll reply quickly to a couple of the specific points:
Yep, this sucks. We should change hook_forms() to hook_form_info() or the D8-appropriate equivalent, and require that all forms be defined with it. The current ugly mechanism of 'dynamic' forms parsing unimplemented form_id's in hook_forms() could be replaced by a combination of hook_form() and a unique key specified when the form is built, or possibly the form build parameters.
I'll look closer at that issue in particular, but it seems relatively unobjectionable. This is the kind of stuff that should really be in the form's metadata rather than the definition structure for the form itself. IE, the 'wrapper callback' would make sense to move into the proposed info hook that would replace hook_forms().
The problem of forms access information being on the menu definition rather than the form itself has tripped up quite a few developers. Mirroring the access callback and param structure from hook_menu would probably make sense, though we'd have to think through the implications. Should menus inherit the callbacks of the forms they display? Would devs need to specify the access information in both hook_menu and hook_forms/hook_form_info/etc?
Another thumbs up. Explicit is better than implicit when it comes to this kind of stuff, and if we want sensible defaults, we should provide them rather than assuming them based on other data.
I'd say this should be optional rather than inherent, and it definitely shouldn't be automatic. There are lots of situations where forms are representative of a cluster of objects, or a part of one object, or no particular objects at all depending on what their intent is. I think it would be useful if each entity's info also specified the form ID to edit or create it, but I'm worried about what the ripple effects would be if it went any farther than that.
In the code below, I'd actually recommend that 'form callback' be required. Make it *EXPLICIT* rather than magical, or we'll be right back at square one. While we're at it, I wouldn't mind eliminating automagical submit and validate functions, either. ;-)
Comment #12
Crell CreditAttribution: Crell commentedIf we're going as far as turning forms into a defined thingie in an info hook that defines lots of callbacks explicitly...
In Drupal 8, that's a plugin. Looking at the code at the bottom of #11, my mind immediately says "Oh, ctools procedural plugins, which in core D8 should be OO plugins using the system that's been developed."
Since I want to make at least the form API engine itself OO so that it can be managed through the DIC, that fits very well. Form-as-object can exist even alongside an array-based definition if we want to keep that for now.
Comment #13
plachTangentially related: #1499596: Introduce a basic entity form controller.
Comment #14
geek-merlin@crell #12:
>Since I want to make at least the form API engine itself OO
is there an issue for that (could not find)
and
will we finally get render objects for what we know as render arrays?
TIA!
Comment #15
Crell CreditAttribution: Crell commentedI don't think there's an issue yet. There's some chatter across various issues in CMI. I doubt that we'd convert render arrays to render objects without a massive refactoring anyway.
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedThis might be needed (or at any rate, can help) both #774876: Form cache entries can expire before their host page's cache entry, resulting in all AJAX failing and #1761488: Process forms in a request listener per the explanation in #774876-11: Form cache entries can expire before their host page's cache entry, resulting in all AJAX failing.
Comment #25
apaderno