Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
- Some form callbacks return a copy of $element, some others alter &$element by reference.
Goal
- Consistently pass $element to Form API callbacks.
Proposed solution
- Change all Form API callbacks to take $element by reference:
D7:
function form_process_radios($element) { ... $element['foo'] = 'bar'; ... return $element; }
D8:
function form_process_radios(&$element) { ... $element['foo'] = 'bar'; ... }
Notes
- If form elements will ever be objects, they will behave as if they were passed by reference, too. So this is a compatible step into that direction.
- Render API should ideally be changed to follow this consistency, since Form API elements usually interact deeply with Render API.
Comment | File | Size | Author |
---|---|---|---|
#15 | dump.txt | 57.92 KB | Niklas Fiekas |
#5 | drupal8.form-element-reference.5.patch | 30.15 KB | sun |
#3 | drupal8.form-element-reference.3.patch | 29.92 KB | sun |
#1 | drupal8.element-reference.1.patch | 27.08 KB | sun |
Comments
Comment #1
sunThis patch changes #process, #after_build, and #value_callback to consistently take $element by reference, instead of returning a copy.
Comment #3
sunwhoopsie - missed some custom defined ones in modules.
Comment #5
sunAlrighty, one final tweak, and this should come back green.
Comment #6
sunReviews, anyone?
Comment #7
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThis clean-up makes a lot of sense. Since the way the callbacks are invoked is changed to depend on pass-by-reference we'd see test failures all over the place if you didn't catch every occurence.
Thank you, sun! I'd say this is RTBC.
We're probably going to need a change notification.
Comment #8
chx CreditAttribution: chx commentedSo we are reverting #56921: Remove reference magic from form API essentially? I am really really really worried about the consequences, have you guys thought this through and through? Did you try dumping an array referencing itself? Does garbage collection work with that?
The current system came up with me trying (and failing) to remove all references from the system. Reverting back at least needs a review of what's happening and how will things work.
Comment #9
chx CreditAttribution: chx commentedNotes: If form elements will ever be objects, they will behave as if they were passed by reference, too. This is not true. this is a very common misconception which is not true http://www.drupal4hu.com/node/224
Comment #10
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOk, if there's a chance that those recursive arrays might be problematic (which I didn't realize) we should probably have testcoverage. Challenge: Come up with a sane example.
I am not sure how to test the garbage collection. Maybe monitor the memory usage of an experimental script.
While #9 is true what is meant here, is that objects are passed with handles/resources/what-references-are-in-java. Like: How to we change the value?
Comment #11
sunI fail to understand the pushback in #8.
Half of Form API is passing stuff by reference since Drupal 6 already. This issue about making it consistent to decrease confusion.
Witness:
&$form_state
Witness:
$form_state['complete_form'] -> &$form
Witness:
function form_builder($form_id, &$element, &$form_state) {
Additionally, this patch reveals that a couple of callbacks are taking &$element by reference already now.
Comment #12
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedI agree on the -Needs tests. Those examples (especially form_builder) are pretty convincing. I don't see what would be different here or anything that would be a problen. So let's go for it!
I'll wait a moment before I re-RTBC this, to avoid back and forth on it.
Comment #13
chx CreditAttribution: chx commentedHrm the push back was not clear? Let me try to clarify further. I would like to avoid like the plague the case where form or form state arrays become recursive. Did either of you var_dump form and form state arrays after a full cycle (ob_start, var_dump , write to a file) in submit and verified this doesnt happen? That's all. I havent even moved to CNW. I just asked to get this reviewed. All hell breaks lose with recursive arrays (witness https://bugs.php.net/bug.php?id=51445 ) and I *think* that D7 doesnt have that.
Comment #14
chx CreditAttribution: chx commentedLet's be even more clear: I am fine with the patch if it does not push us into *RECURSION* (that's what var_dump displays) territory. (And I hope fervently we are not there yet...)
Comment #15
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks chx!
I added a submit handler to the form_test_checkboxes_radios() form and let it print_r($form) and print_r($form_state). The result of that is attached. Since there are no recursions I don't know anything else to worry about. Neither does chx. Marking RTBC again.
Edit: I also just dumped the same form without the patch. Only some tokens differ - everything else is the same.
Comment #16
sun#5: drupal8.form-element-reference.5.patch queued for re-testing.
Comment #18
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThat this is back to needs work is a good chance to mention that chx suggested to have a test to ensure: For a simple form with checkboxes and radios $form and $form_state that are given to the submit handler have no recursion. Might have a chance to write such a test soon.
Comment #31
smustgrave CreditAttribution: smustgrave at Mobomo commentedCurrently going through the end of the queue to see what could still be relevant for D10.
Wondering if this is still a valid task?