Problem

  • Form API implicitly invokes magic #value_callback functions for many elements, of which developers are not aware of.
  • Developers mistakenly override the #value_callback property for an element without knowing that they disable/destroy functionality that's essential for security.
  • The magic form_type_TYPE_value() callbacks require a function_exists() check and thus decrease performance.
  • If user input for an element cannot be handled through a defined but non-existing #value_callback, form processing must not silently ignore this error.
  • The auto-suggested form_type_TYPE_value() function name is outside of any module's namespace to begin with.

Goal

  • Increase security and performance by making #value_callback definitions explicit.

Details

  • _form_builder_handle_input_element() contains two function_exists() checks for #value_callback handlers to test whether an explicitly registered or implicitly suggested default form_type_TYPE_value() callback exists.
  • The #value_callback can however easily be defined in hook_element_info(). Then we don't need the function_exists() checks (#process and #after_build callbacks aren't checked either).

API change

form_type_TYPE_value() callbacks are no longer invoked automagically. They need to be explicitly declared in hook_element_info().

CommentFileSizeAuthor
#12 drupal8.form-value-callback.12.patch5.08 KBthomas.fleming
#5 drupal8.form-value-callback.5.patch5 KBsun
#4 946570-value-callback.patch4.73 KBcasey
form-value-callback.patch4.8 KBcasey

Comments

casey’s picture

casey commented
Title: Improve performance » Improve performance of _form_builder_handle_input_element()

Oops

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Version: 7.x-dev » 8.x-dev

I would think this patch would offer only a minimal improvement. And since it means an API change, would have to wait till D8.

casey’s picture

casey commented
Title: Improve performance of _form_builder_handle_input_element() » Use #value_vallback instead of magic form_type_$type_value()
Issue tags: +API change

I agree (I should stop posting issues as performance improvements, while I actually have other intentions).

casey’s picture

casey commented
StatusFileSize
new 946570-value-callback.patch4.73 KB

Reroll

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Use #value_vallback instead of magic form_type_$type_value() » Use and call explicit #value_callback instead of magic form_type_TYPE_value()
Assigned: Unassigned » sun
Issue tags: -Performance +API clean-up
StatusFileSize
new drupal8.form-value-callback.5.patch5 KB

Marked #1013722: Drop #value_callback auto-suggestions, declare them explicitly like everyone else as 99% duplicate. The typo in the title did not yield this issue. :-/

Re-rolled my patch from over there, incorporated the additional proposal of skipping the function_exists(), and added an inline comment that explains why it is omitted.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I've also rewritten the issue summary.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue summary: View changes

Updated issue summary.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

This looks ready to fly for me. Can't be tested, unless we have "expected fail (fatal error)" support in the testing system.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: -API change, -API clean-up

#5: drupal8.form-value-callback.5.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal8.form-value-callback.5.patch, failed testing.

thomas.fleming’s picture

thomas.fleming commented
Status: Needs work » Needs review

#5: drupal8.form-value-callback.5.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +API change, +API clean-up

The last submitted patch, drupal8.form-value-callback.5.patch, failed testing.

thomas.fleming’s picture

thomas.fleming commented
StatusFileSize
new drupal8.form-value-callback.12.patch5.08 KB

rerolled

thomas.fleming’s picture

thomas.fleming commented
Assigned: sun » Unassigned
Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -API change, -API clean-up

The last submitted patch, drupal8.form-value-callback.12.patch, failed testing.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.

Issue tags: +API change, +API clean-up

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

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.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: +Needs architectural review

This looks nice, I wonder how feasible it is for 8.x?

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.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Closed (duplicate)

This was made redundant by #2311393: Remove hook_element_info() and all references to it