As initially reported by kbahey at http://drupal.org/node/138706#comment-244726 (comment #118 of the original FAPI3 issue):

Reading the documentation that is here http://drupal.org/node/144132 I find something that really needs to be fixed: the function prototypes have asymmetric argument ordering.

For example, for *_form_alter() we have $form_values, $form_id, $form_state, but for *_validate() we have $form_id, $form_values, $form_state. For *_submit() we have $form_id, $form_values, $form_state.

It would be best for everyone if we have a general consistent order for the arguments, for example, $form_id, $form_values followed by $form_state. This would make the learning curve less steep, as well as reduce confusion and errors.

Dries, myself, and many others agree we should do this. However, http://drupal.org/node/138706 has become impossible to follow, so I'm submitting this as a separate issue so we can start fresh.

CommentFileSizeAuthor
#27 comment.module_77.patch751 bytesax
#21 upload.module_44.patch812 bytesax
#15 fapi_install.patch1.21 KBeaton
#7 form_params.patch57.63 KBeaton
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kbahey’s picture

kbahey CreditAttribution: kbahey commented

Obviously, I give this a +1.

Think of all the newbies ...

eaton’s picture

eaton CreditAttribution: eaton commented

changing the hook_form_alter() parameters won't work; it relies on the first parameter being passed by reference and thus MUST be $form. That was part of the change to use drupal_alter() rather than custom iterators every time we needed to alter a FAPI-like structure. (links, menus, profiles, etc).

Alter functions and submit/validate functions really do accept a different set of params, so things aren't quite as clear cut. What we could do is standardize on this order:

$form, $form_state, [$form_id | $form_values]

That would ensure that $form and $form_state are always the first parameters, while the 'different' bits come at the end.

Any thoughts?

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

that sounds pretty reasonable to me. my only question is: "what FAPI-related functions don't take a form_id, and why not?" ;) seems like we *could* pass the form_id everywhere, even if it's not technically required. then, we could just have the order be something like:

form, form_id, form_state, [form_values]

chx’s picture

chx CreditAttribution: chx commented

eaton++ . form_id passing in to validate/submit--

eaton’s picture

eaton CreditAttribution: eaton commented

Right now, hook_form_alter() is actually the only place that needs $form_id -- we now use the id to retrieve the form, but no longer do switching logic based on form_id in the submit and validate handlers.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

great, that's fine with me. keeping everything 3 args deep is nice, for consistency, too. ;) so, i remove whatever percieved hesitation #3 might have emplied, and fully embrace/support #2.

eaton’s picture

eaton CreditAttribution: eaton commented
Status: Active » Needs review
FileSize
form_params.patch57.63 KB

here's a quick roll of the patch -- DEFINITELY needs testing.

David Strauss’s picture

David Strauss
he/him
CreditAttribution: David Strauss commented

Thanks for taking care of the dirty work necessary to make the interface consistent.

David Strauss’s picture

David Strauss
he/him
CreditAttribution: David Strauss commented
Title: unify FAPI3 callback signatures » Standardize Form API 3 hook parameter order

Hopefully this patch will get more attention and testing with this new title. I had to read the thread to realize what this issue was actually about.

Dries’s picture

Dries CreditAttribution: Dries commented

I'd like this to go in, so hopefully, we can get some testers here! :)

morphir’s picture

morphir CreditAttribution: morphir commented
Status: Needs review » Reviewed & tested by the community

+1

I have tested this one, and everything seems just fine.

morphir’s picture

morphir CreditAttribution: morphir commented

also,

make sure that handbook-pages on coding standard gets updated.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

morphir’s picture

morphir CreditAttribution: morphir commented
Status: Fixed » Needs work

there is a fugly bug in the install process.

eaton’s picture

eaton CreditAttribution: eaton commented
Status: Needs work » Needs review
FileSize
fapi_install.patch1.21 KB

Install.php was hard-calling the user module's internal FAPI functions, and that line had been missed. Fixed!

morphir’s picture

morphir CreditAttribution: morphir commented
Status: Needs review » Reviewed & tested by the community

works!

dmitrig01’s picture

dmitrig01 CreditAttribution: dmitrig01 commented

confirmed

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented

Confirmed. Thanks Eaton.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks.

ax’s picture

ax CreditAttribution: ax commented
Status: Fixed » Needs work

line 917

$form = form_builder('upload_js', $form, array());

breaks my site with a "PHP Fatal error: Only variables can be passed by reference". so either array() should be a variable or &$form_state in function form_builder($form_id, $form, &$form_state) {} should be changed to $form_state.

ax’s picture

ax CreditAttribution: ax commented
Status: Needs work » Needs review
FileSize
upload.module_44.patch812 bytes

array() should be $form_state. patch attached.

drewish’s picture

drewish CreditAttribution: drewish commented

it does seem like $form_state should be passed in but if its omission wasn't a bug, i'd be a fan of something like:

$form = form_builder('upload_js', $form, $foo = array());
ax’s picture

ax CreditAttribution: ax commented

i'm pretty sure the omission *was* a bug, as $form_state is defined just the line above. fixed with my patch at #21.

yched’s picture

yched CreditAttribution: yched commented

Follow up bugfix : args reordering for form submit handlers needs to be reflected in batch engine.
Patch submitted in http://drupal.org/node/147501

ax’s picture

ax CreditAttribution: ax commented
Status: Needs review » Reviewed & tested by the community

please commit the remaining typo fix, #21 at http://drupal.org/node/146470#comment-251011 . tnx!

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

ax’s picture

ax CreditAttribution: ax commented
Status: Fixed » Reviewed & tested by the community
FileSize
comment.module_77.patch751 bytes

another follow-up fix, this time in comment.module. hope thats the last one :S

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Reviewed & tested by the community » Needs review

that patch scares me. what's the difference between $form_state['values'] and $form_values? i fear the answer is: "none, they're identical". i remember eaton saying he wanted to get rid of $form_values entirely. if it's completely duplicate with $form_state['values'], it seems like we should just finish removing it completely before the API freeze, both to avoid developer confusion, and to reduce the memory footprint of passing around big, redundant arrays all over the place.

p.s. @ax: for future reference, it's considered bad form to post your own patch as RTBC. your own work is always "needs review" until independently verified by someone else. thanks. in this case, i'm setting it back to needs review since i'd like eaton and/or chx's input on this topic of standardizing the hook parameters by way of removing $form_values entirely.

eaton’s picture

eaton CreditAttribution: eaton commented

We can definitely kill $form_values. It's redundant and only serves as a convenience -- I'd be happy to see it removed. Given how close we are to the freeze, however, I'd want to roll that change in with http://drupal.org/node/146667 -- fixing problems with $form_state in parameterized builder functions. Given how late we are in the cycle I have no desire to start mucking around with numerous mutually exclusive patches.

and to reduce the memory footprint of passing around big, redundant arrays all over the place.

it's a reference, not a copy, so there's no memory penalty, but all other arguments for removing it stand.

ax’s picture

ax CreditAttribution: ax commented

@dww:

> p.s. @ax: for future reference, it's considered bad form to post your
> own patch as RTBC. your own work is always "needs review" until
> independently verified by someone else. thanks.

i know, and i usually do (see #21 above). point is, "needs review" often rots in the queue (see #25 above), and i don't have the time to follow up on these. in this case - where a obvisious typo completely breaks something, here: the comment submission - i just did the same as yched at #24 (and me at #25) above: post the patch and set it to RTBC so it gets attention. so in 2 out of 3 cases (and after eatons #29, i'd say in 3 out of 3 cases) i guess this was the right thing to do, because otherwise we would still have a broken HEAD.

of course it would be better to get attention for critical patches in "need review" state, but thats another issue.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Needs review » Fixed

let's call this fixed and move our attention to eaton's latest patch at http://drupal.org/node/146667

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)
dharamgollapudi’s picture

dharamgollapudi CreditAttribution: dharamgollapudi commented

Subscribing....