hook_form_BASE_FORM_ID_alter(), hook_form_alter(), and hook_form_FORM_ID_alter() doc inconsistent.

Actually, 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:

 // Invoke hook_form_alter(), hook_form_BASE_FORM_ID_alter(), and
  // hook_form_FORM_ID_alter() implementations.
  $hooks = array('form');
  if (isset($form_state['build_info']['base_form_id'])) {
    $hooks[] = 'form_' . $form_state['build_info']['base_form_id'];
  }
  $hooks[] = 'form_' . $form_id;
  drupal_alter($hooks, $form, $form_state, $form_id);

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...

Hey, 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

i can't get the patch (maybe because of the #) to test it.
Please can you re-upload it ?

Ok, patch name is updated...hopefully it works this time!

I'm all for it. I just rerolled the patch and removed trailing whitespaces that git didn't like.

Thanks for your contribution!

I would like to see a few things changed in this patch:

a)

+ * Note that, in addition to hook_form_alter(), which is called for all forms,
+ * there are two, more specific, form hooks available. The first,

I think, that, maybe, this, could use, a few less commas? :)

b)

+ * For each module (in system weight order), form alter hooks are called in the
+ * following order: first, hook_form_alter(); second,
+ * hook_form_BASE_FORM_ID_alter(); third, hook_form_FORM_ID_alter(). So the more
+ * general hooks are called first followed by the more specific. After all
+ * module hook implementations are invoked, the hook_form_alter()
  * implementations from themes are invoked in the same manner.

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!

Actually, 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.

Agree. Explaining how BASE_FORM is formed would be good.

Ok, 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 !

Thanks!

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?

Hi,

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...

I 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.

Ok, 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

One minor correction:

+++ b/modules/system/system.api.phpundefined
@@ -1232,17 +1247,23 @@ function hook_form_FORM_ID_alter(&$form, &$form_state, $form_id) {
+ * Provide a form-specific alteration for shared ('base') forms.

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.

Ok thanks xjm, I've made the changes...

nicl

Ok, bit embarrassed, but here is another patch which gets the 'Provides' right this time!

Apologies for the slew of patches :(

I 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()?

Ok, 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:

• how to identify whether a form has a base form
• how the base form ID is constructed
• and why base forms can be useful.

This documentation looks really good! I did find a couple of small things that need fixing, and then we can get this committed:

a)

 /**
- * Provide a form-specific alteration instead of the global hook_form_alter().
+ * Provides a form-specific alteration instead of the global hook_form_alter().

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)

+ *
+ * Two example use cases where base forms may be useful are given below:
  *

I think this should end in . instead of :

d)

+ * To identify the base form ID for a particular form (or to determine whether
+ * one exists) check the $form_state. The base form ID is stored under
+ * $form_state['build_info']['base_form_id'].

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.

  * By default, when drupal_get_form() is called, the system will look for a
  * function with the same name as the form ID, and use that function to build
- * the form. This hook allows you to override that behavior in two ways.
+ * the form. This hook allows you to override this behaviour and reroute calls
+ * to certain form IDs in a manner of your choosing. As a result, you can easily
+ * map multiple form IDs to a single form (referred to as a 'base' form).

f) Our standard in Drupal docs is to use American spellings -- behavior not behavior.

Hey, thanks for the feedback. I've tried to make the changes requested! Let me know thoughts...

Patch 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.

Looks pretty good! One more thing to fix:

+ * implementing this hook can then provide their own instructions for mapping
+ * form ids to constructor functions. As a result, you can easily map multiple
+ * form IDs to a single form constructor (referred to as a 'base'form).

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..."?

+ * Using a base form can help to avoid code duplication, by allowing many
+ * similar forms to use the same code base. Another benefit is that it becomes
+ * much easier to apply a general change to the group of forms;
+ * hook_form_BASE_FORM_ID_alter() can be used to easily alter multiple forms at
+ * once by directly targeting the shared base form.

Thanks for the reviews DjebbZ and jhodgson !

Looks good -- thanks for all of the iterations!

Committed to 7.x and 8.x.

So glad to see the documentation keeps getting better and better.

/me happy to contribute to the docs.