Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#9 | interdiff.txt | 951 bytes | Xano |
#9 | drupal_2166847_9.patch | 3.14 KB | Xano |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedComment #2
jhodgdonIt 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?
Comment #3
pwolanin CreditAttribution: pwolanin commentedYeah, seems like they all need to be fixed.
Comment #4
jhodgdonComment #5
XanoSomething like this? It's a bit tricky, now the whole API has become so flexible.
Comment #6
jhodgdonThanks! 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:
you want to do this:
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:
should be put into the getFormId() method docs? Although... see (e) below.
d) The original issue report mentioned that
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).
Comment #7
XanoDone.
Done.
buildForm()
does not get$form_arg
, but$form_id
. There is a difference, so I moved this bit of documentation togetForm()
, 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.
Comment #8
jhodgdonLooks pretty good! On the docs for $form_arg:
- 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!
Comment #9
XanoComment #10
jhodgdonThanks! Looks good to me. Committed to 8.x.