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

  1. 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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Active » Needs review
FileSize
27.08 KB

This patch changes #process, #after_build, and #value_callback to consistently take $element by reference, instead of returning a copy.

Status: Needs review » Needs work

The last submitted patch, drupal8.element-reference.1.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
29.92 KB

whoopsie - missed some custom defined ones in modules.

Status: Needs review » Needs work

The last submitted patch, drupal8.form-element-reference.3.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
30.15 KB

Alrighty, one final tweak, and this should come back green.

sun’s picture

Reviews, anyone?

Niklas Fiekas’s picture

Status: Needs review » Reviewed & tested by the community

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

chx’s picture

Status: Reviewed & tested by the community » Needs review

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

chx’s picture

Notes: 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

Niklas Fiekas’s picture

Issue tags: +Needs tests

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

function f($obj) {
  $obj->value = 'foo';
  return $obj; // <-- No reason to return the object. We changed the passed one anyway.
}
sun’s picture

Issue tags: -Needs tests

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

Niklas Fiekas’s picture

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

chx’s picture

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

chx’s picture

Let'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...)

Niklas Fiekas’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
57.92 KB

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

sun’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +API change, +API clean-up, +consistency

The last submitted patch, drupal8.form-element-reference.5.patch, failed testing.

Niklas Fiekas’s picture

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Issue summary: View changes
Status: Needs work » Postponed (maintainer needs more info)

Currently going through the end of the queue to see what could still be relevant for D10.

Wondering if this is still a valid task?

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.