API page: http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...
Describe the problem you have found:
The docs say
Note that this hook fires after hook_form_FORM_ID_alter() and before hook_form_alter().
I believe this is the wrong way round (and certainly goes against the docs for hook_form_alter()).
Cheers!
Comment | File | Size | Author |
---|---|---|---|
#26 | hook_form_alter_docs-1229896-26.patch | 8.14 KB | nicl |
#23 | hook_form_alter_docs-1229896-23.patch | 8.12 KB | nicl |
#21 | hook_form_alter_docs-1229896-21.patch | 7.6 KB | nicl |
#19 | hook_form_alter_docs-1229896-19.patch | 6.33 KB | nicl |
#18 | hook_form_alter_docs-1229896-17.patch | 6.32 KB | nicl |
Comments
Comment #1
jhodgdonActually, hook_form_alter() doesn't mention hook_form_BASE_FORM_ID_alter() at all, and it should -- it only refers to hook_form_FORM_ID_alter(). But it's true, the order mentioned in hook_form_alter() is opposite the one mentioned in hook_form_BASE_FORM_ID_alter(). And hook_form_FORM_ID_alter() doc doesn't say anything about the order.
So... let's see. Looking at the bottom of http://api.drupal.org/api/drupal/includes--form.inc/function/drupal_prep... I see:
So the order is hook_form_alter(), hook_form_BASE_FORM_ID_alter(), hook_form_FORM_ID_alter() [general to specific].
What needs to be done here:
a) hook_form_alter(), hook_form_BASE_FORM_ID_alter(), and hook_form_FORM_ID_alter() all need to link to each other with @see.
b) hook_form_alter() should mention hook_form_BASE_FORM_ID_alter() in its "... you can also use..." section that currently only talks about hook_form_FORM_ID_alter().
c) All three functions should mention the correct order of the hook firings (general to specific).
This is probably a good project for a relatively novice doc contributor...
Comment #2
nicl CreditAttribution: nicl commentedHey, step up novice contributor. Someone needs to check I have understood the call order properly (I have basically used the help from above along with the pre-existing comments.
Open to any suggestions for improvements obviously!
nicl
Comment #4
Bès CreditAttribution: Bès commentedi can't get the patch (maybe because of the #) to test it.
Please can you re-upload it ?
Comment #5
nicl CreditAttribution: nicl commentedOk, patch name is updated...hopefully it works this time!
Comment #6
nicl CreditAttribution: nicl commentedComment #7
DjebbZ CreditAttribution: DjebbZ commentedI'm all for it. I just rerolled the patch and removed trailing whitespaces that git didn't like.
Comment #8
jhodgdonThanks for your contribution!
I would like to see a few things changed in this patch:
a)
I think, that, maybe, this, could use, a few less commas? :)
b)
After reading this paragraph, I was confused about whether all hook_form_alter() calls were done, then all hook_form_BASE_... etc. or whether it would do all of the hooks for one module, then move on to the next. The documentation should ideally make that clear.
That's it -- the rest looks good, thanks!
Comment #9
jhodgdonActually, maybe this should mention how a BASE_FORM would be defined?
And by the way, the answer to the question in (b) is that drupal_alter() will invoke all existing functions for module A, then all for module B, etc., followed by all for the base theme, and finally for the theme.
Comment #10
DjebbZ CreditAttribution: DjebbZ commentedAgree. Explaining how BASE_FORM is formed would be good.
Comment #11
nicl CreditAttribution: nicl commentedOk, I made some changes which (hopefully) make it clearer - though to be honest I struggled to describe the order in a simple and clear manner - I paraphrased jhodgdon in #9 (thanks).
Re: info on how a BASE_FORM would be defined. Does this mean how to make one, or how the ID is defined? I (now - didn't before) know that base forms are defined using hook_forms() and that the base form ID is taken from the relevant callback value defined in this function. But it is useful to add this or not ?
I added some minor info about this in the docs for the base form alter (e.g. @see hook_forms() ) but perhaps more is needed?
ps. sorry for the delay, have been pretty busy recently !
Comment #12
nicl CreditAttribution: nicl commentedComment #13
jhodgdonThanks!
Minor typographical fixes needed:
- Line wrapping. All lines should wrap as close to 80 characters as possible, without going over.
- I think we need a comma after "module" in this line:
Within each module form alter hooks...
As far as the content... I think this is pretty good! There was only one thing that I think needs fixing. I don't see anything in the hook_forms() documentation that explains anything about (or even mentions) base forms:
http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...
So the line that says "See hook_forms() for more information on how to implement base forms in Drupal." is not very helpful. I think the way to make it helpful would be to document how to make base forms in hook_forms(). Could we add that to the patch?
Hmmm... node_forms() and comment_forms() do not (as far as I can tell) define any base forms. So now I am confused, since this is the example given for the BASE form alter hook... Maybe I just don't know how to define them?
Comment #14
nicl CreditAttribution: nicl commentedHi,
as I understand it base forms are defined like any other (although you might have some more conditionals in the form than normal - to account for the multiple uses) and individual form calls are effectively routed towards the single base form using hook_forms().
hook_forms() - not to be confused with the similarly named hook_form() is the routing mechanism.
I have never had to make a base form myself so maybe someone with more knowledge can step in. But I think this is how the process works...
Comment #15
jhodgdonI don't know the answer off-hand. The way to figure it out would probably be to look at a module that uses base forms and see how they did it.
Comment #16
nicl CreditAttribution: nicl commentedOk, finally I have done another patch for this.
I have added the changes you suggested, including some slight improvements (I hope) to the hook_forms() documentation to clarify a bit what it does. Before it didn't even use the term 'base forms' at all!
I looked at various implementations of hook_forms (in both core and contrib - such as the webform module and views) and am pretty confident my understanding is correct but I'll try and find someone on IRC in drupal-contribute who knows base forms well to review too.
Anyway, let me know what you think and whether and how it can be improved - I definitely agree that the docs on base forms are not great, and could almost certainly be further improved.
Thanks,
nicl
Comment #17
xjmOne minor correction:
Should begin with "Provides".
Other than that point, the changes appear clear, correct, and to standard. I would understand the order of form processing much better thanks to these changes.
Comment #18
nicl CreditAttribution: nicl commentedOk thanks xjm, I've made the changes...
nicl
Comment #19
nicl CreditAttribution: nicl commentedOk, bit embarrassed, but here is another patch which gets the 'Provides' right this time!
Apologies for the slew of patches :(
Comment #20
jhodgdonI still don't think this is telling me how to define a base form that I could use with hook_form_BASE_FORM_ID_alter(). How do I tell Drupal what the base form ID is when defining in hook_forms(), and how do I know if I want to implement hook_BASE_FORM_ID_alter()?
Comment #21
nicl CreditAttribution: nicl commentedOk, I've updated the patch to account for the new directory changes, and also to hopefully make things a bit clearer.
Specifically, I added information about:
Comment #22
jhodgdonThis documentation looks really good! I did find a couple of small things that need fixing, and then we can get this committed:
a)
This is hook doc... should stay Provide not Provides. See
http://drupal.org/node/1354#hooks
b) Same for the BASE hook doc in the next section of the patch (Provide/Provides).
c)
I think this should end in . instead of :
d)
This information should also be added to the section of the documentation of drupal_build_form() where $form_state array is documented.
http://api.drupal.org/api/drupal/core--includes--form.inc/function/drupa...
e) In order to gauge the correctness of this wonderfully clear new documentation, I took a look at drupal_retrieve_form(). It looks like if a function exists with the same name as the form ID, hook_forms() information is never used... we should probably mention that in hook_forms(), because the way it's worded now, it says you can override this behavior using hook_forms(), but really you can only override it if the form ID function does not exist.
f) Our standard in Drupal docs is to use American spellings -- behavior not behavior.
Comment #23
nicl CreditAttribution: nicl commentedHey, thanks for the feedback. I've tried to make the changes requested! Let me know thoughts...
Comment #24
DjebbZ CreditAttribution: DjebbZ commentedPatch looks good to me. I learned a lot just by reading the comments, which is a good sign. Now I understand when hook_forms() is called : only when no function with the same name as the parameter of drupal_get_form() exisits. I think you made it clear how the whole stuff works. nicl++ :)
I'm not changing the status to RTBC, I leave this jhodgdon.
Comment #25
jhodgdonLooks pretty good! One more thing to fix:
In the second line ids -> IDs.
In the third line, needs a space between 'base' and form.
Also, maybe the third line here should say "... much easier for other modules to apply..."?
Comment #26
nicl CreditAttribution: nicl commentedThanks for the reviews DjebbZ and jhodgson !
Comment #27
jhodgdonLooks good -- thanks for all of the iterations!
Comment #28
Dries CreditAttribution: Dries commentedCommitted to 7.x and 8.x.
So glad to see the documentation keeps getting better and better.
Comment #29
DjebbZ CreditAttribution: DjebbZ commented/me happy to contribute to the docs.