In FormBuilderInterface::getForm()

It's very unclear that one can pass a class name as well as a class instance in.

In addition, it references 2 implementations of hook_forms() that no longer exist.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.41 KB
jhodgdon’s picture

It looks like the work done in the FormBuilder class implementation of this method is done by calling getFormId() and buildForm(). So maybe those methods' doc needs an update as well? Actually, most of the methods in this interface have similar docs?

pwolanin’s picture

Yeah, seems like they all need to be fixed.

jhodgdon’s picture

Status: Needs review » Needs work
Xano’s picture

Status: Needs work » Needs review
FileSize
4.44 KB

Something like this? It's a bit tricky, now the whole API has become so flexible.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! This is looking better... A few things to clean up and improve:

a) I like the docs for $form_arg -- they seem pretty clear. But they need to use our list formatting standards: https://drupal.org/node/1354#lists
So instead of:

    * @param \Drupal\Core\Form\FormInterface|string $form_arg
+   *   The value must be
+   *   - the name of a class that implements \Drupal\Core\Form\FormInterface
+   *   or
+   *   - an object that implements \Drupal\Core\Form\FormInterface
+   *   or
+   *   - the name of a function that builds the form.

you want to do this:

    * @param \Drupal\Core\Form\FormInterface|string $form_arg
+   *   The form argument, which must be one of the following:
+   *   - The name of a class that implements \Drupal\Core\Form\FormInterface
+   *   - An object that implements \Drupal\Core\Form\FormInterface
+   *   - The name of a function that builds the form.

b) Perhaps once this information about $form_arg is in the getFormId() method, rather than copy/pasting it all over the place, we could refer to it. Something like "The form argument, as in FormBuilderInterface::getFormId()." ... Or maybe rather than putting it into getFormId() we should put it in a method more likely to be called, like buildForm()?

c) In which case, I think this bit:

+   *   Modules that need to generate the same form (or very similar forms) using
+   *   different $form_ids can implement hook_forms(), which maps different
+   *   $form_id values to the proper form constructor function. Examples may be
+   *   found in node_forms() and search_forms().

should be put into the getFormId() method docs? Although... see (e) below.

d) The original issue report mentioned that

+   *       constructor function. Examples may be found in node_forms(), and
+   *       search_forms().
 

node_forms() and search_forms() do not exist any more, so they cannot be referenced. Actually, there are zero implementations in core any more.

e) I was getting confused about whether hook_forms() was even relevant any more and where the docs in (c) really belong, so I took a look at the actual FormBuilder stuff. Here's what actually happens.

1. The first time you build a form you're going to be running buildForm(). It calls getFormId(), which tries to instantiate it as a class, and then if either an object was passed in or a class name, it figures out the form callback from the class (if it's a form object), and if not it assumes the passed-in argument is the form ID.

2. buildForm() calls retrieveForm(). That tries to figure out what function/method to call. If getFormId() found a class, the callback will be the buildForm() method there. If not, it will try to use the $form_arg string as a function. And if that function doesn't exist, then it will invoke hook_forms() and see if anyone made a mapping between $form_arg and a callback function.

So... Can we revise the documentation around hook_forms() and $form_arg so that this information is conveyed? Let's not talk about "$form_id" when the argument to most of these functions is now called "$form_arg" -- we can talk about "the form ID" but I don't think we should throw around $form_id when that is not really used anywhere. And let's only put in the information about hook_forms() where it is most relevant.

f) Shouldn't the first argument in buildForm() really be called $form_arg and not $form_id? Changing parameter names to be more relevant is a valid thing to do in a documentation patch (although in this case it would also require changing it in the FormBuilder class as well as this interface -- and it doesn't look like any other classes/interfaces would be affected).

Xano’s picture

Status: Needs work » Needs review
FileSize
3.12 KB

I like the docs for $form_arg -- they seem pretty clear. But they need to use our list formatting standards: https://drupal.org/node/1354#lists

Done.

Perhaps once this information about $form_arg is in the getFormId() method, rather than copy/pasting it all over the place, we could refer to it.

Done. buildForm() does not get $form_arg, but $form_id. There is a difference, so I moved this bit of documentation to getForm(), which is called often and where the context makes the documentation make more sense, as $form_arg is not specific to getting the form ID.

hook_forms() has been removed in #2110951: Remove hook_forms(), so the feedback that concerns that part of the API is no longer relevant. I did remove one last comment about the hook that was still left.

Re-roll, so no interdiff.

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good! On the docs for $form_arg:

+   *   The value must be one of the following:
+   *   - the name of a class that implements \Drupal\Core\Form\FormInterface.
+   *   - an object that implements \Drupal\Core\Form\FormInterface.
+   *   - the name of a function that builds the form.

- List items should normally start with capital letters.
- I don't think objects implement interfaces. Classes implement interfaces, and objects are instances of those classes. It's kind of a subtle distinction, but you'll notice for instance on http://us2.php.net/manual/en/language.operators.type.php if you search for "interface" in your browser window, they use wording like:
"Lastly, instanceof can also be used to determine whether a variable is an instantiated object of a class that implements an interface: ". So here, maybe we should say "An object that is an instance of a class implementing [interface name]"?

Other than that, looks good!

Xano’s picture

Status: Needs work » Needs review
FileSize
3.14 KB
951 bytes
jhodgdon’s picture

Status: Needs review » Fixed

Thanks! Looks good to me. Committed to 8.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.