Problem/Motivation

Removal of "filtering" checkboxes values has change record https://www.drupal.org/node/1910694

There was a set of issues per module, so this is follow-up to conversion #1703164-26: Convert book settings to the new configuration system

+++ b/core/modules/book/book.admin.inc
@@ -45,14 +47,14 @@ function book_admin_settings() {
   $form['array_filter'] = array('#type' => 'value', '#value' => TRUE);

@@ -66,6 +68,18 @@ function book_admin_settings_validate($form, &$form_state) {
+    ->set('allowed_types', $form_state['values']['book_allowed_types'])
+    ->set('child_type', $form_state['values']['book_child_type'])

This property has supposed to filter out empty values before saving

Proposed resolution

Remove the form element & add filtering for empty values

Remaining tasks

Review, commit

Data model changes

May need update hook to clean-up existing empty values out of child_type key of book.settings

Comments

Dave Reid’s picture

Title: system_settings_form saves undesired 'array_filter' variable » system_settings_form_submit saves undesired 'array_filter' variable

This bug is also present in D6 and D5 and should be backported if this is deemed an acceptable solution.

Dave Reid’s picture

FileSize
1.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch array_filter-D5.patch. Unable to apply patch. See the log in the details link for more information. View
1.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch array_filter-D6.patch. Unable to apply patch. See the log in the details link for more information. View

Here are the D5 and D6 patches for this issue just for kicks. Don't hurt me testing bot...

Dave Reid’s picture

FileSize
2.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch array_filter-2-D7.patch. Unable to apply patch. See the log in the details link for more information. View

Here is another proposed solution, that removes the FAPI hack 'array_filter' and adds a new function drupal_filter_array() in form.inc that can be used with #element_validate:

$form['item']['#element_validate'] = array('drupal_filter_array');
Damien Tournoud’s picture

Status: Needs review » Needs work

I love #3, that's so Form API compliant :)

We should include an update function that remove array_filter from the variable table.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
4.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch array_filter-3-D7.patch. Unable to apply patch. See the log in the details link for more information. View

New patch:
- Adds a function drupal_filter_form_array that replaces the FAPI hack $form['array_filter'] with $form['item']['#element_validate'] = array('drupal_filter_form_array');
- Adds system_update_7011 to delete the unwanted 'array_filter' variable that was accidentally saved.
- Picked up a few coding space standard issues.

Dave Reid’s picture

Title: system_settings_form_submit saves undesired 'array_filter' variable » Remove $form['array_filter'] hack with #element_validate

Renaming for new direction and bump for review or thoughts. Anyone?

Dave Reid’s picture

Title: Remove $form['array_filter'] hack with #element_validate » Replace $form['array_filter'] hack with #element_validate
dropcube’s picture

Status: Needs review » Needs work

According to the recommendations of chx in IRC, it's better to use #process. Any #process handlers attached to a specific element are executed before they are processed.

Dave Reid’s picture

I need some help with this...I have no idea how to get #process working to filter array values on submission... Where's my patch status of 'code needs help'? :)

Dave Reid’s picture

Dave Reid’s picture

Issue tags: +DrupalWTF

Adding WTF tag.

sun’s picture

Unless chx provided some hidden reasoning that's not stated here, I think that #element_validate is perfectly correct here.

Lovely patch! Just a bit old. Anyone up for re-rolling?

drewish’s picture

subscribing

sun’s picture

Version: 7.x-dev » 8.x-dev

Too late for D7.

Dave Reid’s picture

Agreed, although I'm still interested in helping fix.

andypost’s picture

franz’s picture

subscribe

franz’s picture

Status: Needs work » Needs review
FileSize
2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 33,646 pass(es). View

I kind of re-rolled to 8.x

I didn't know what to do with update function on .install, is it needed for D8?

Dave Reid’s picture

FileSize
4.6 KB
PASSED: [[SimpleTest]]: [MySQL] 40,361 pass(es). View

Re-rolled again for HEAD.

sun’s picture

Status: Needs review » Needs work

Thanks for moving this forward! :)

Some remarks on the proposed code:

+++ b/core/includes/form.inc
@@ -4577,6 +4577,22 @@ function _form_set_class(&$element, $class = array()) {
+ * This replaces the FAPI hack $form['array_filter'] with the more elegant
+ * $form['item']['#element_validate'] = array('form_validate_filter_array');

This looks/reads like superfluous historical context to me and should thus be removed.

+++ b/core/includes/form.inc
@@ -4577,6 +4577,22 @@ function _form_set_class(&$element, $class = array()) {
+ * This validation handler is automatically added to any 'checkboxes' FAPI
+ * element. It only needs to be enabled by adding
+ * $form['my_checkboxes']['#filter_array'] = TRUE.

1) "FAPI" does not exist, should be changed to "Form API".

2) Missing "#type" before 'checkboxes'.

3) How about wrapping the code into @code and @endcode?

+++ b/core/includes/form.inc
@@ -4577,6 +4577,22 @@ function _form_set_class(&$element, $class = array()) {
+  if (!isset($element['#filter_array']) || !empty($element['#filter_array'])) {

The first !isset() condition looks wrong to me -- a callback like this shouldn't perform any actions unless explicitly being told so.

+++ b/core/modules/aggregator/aggregator.admin.inc
@@ -503,6 +503,7 @@ function aggregator_admin_form($form, $form_state) {
+      '#filter_array' => TRUE,

I wonder whether there is a more self-descriptive property name than #filter_array?

Since this only applies to checkboxes, what do you think of #submit_enabled_only or similar...?

+++ b/core/modules/book/book.admin.inc
@@ -62,7 +62,7 @@ function book_admin_settings($form, &$form_state) {
 function book_admin_settings_validate($form, &$form_state) {
   $child_type = $form_state['values']['book_child_type'];
-  if (empty($form_state['values']['book_allowed_types'][$child_type])) {
+  if (!in_array($child_type, $form_state['values']['book_allowed_types'])) {
     form_set_error('book_child_type', t('The content type for the %add-child link must be one of those selected as an allowed book outline type.', array('%add-child' => t('Add child page'))));
   }

I wonder whether this entire form validation handler isn't obsolete? It seems to duplicate Form API's built-in options checker...?

+++ b/core/modules/system/system.install
@@ -2047,6 +2047,13 @@ function system_update_8016() {
+function system_update_8017() {

8017 already exists in HEAD.

mgifford’s picture

Assigned: Dave Reid » Unassigned
Issue summary: View changes
Alan D.’s picture

Since this only applies to checkboxes, what do you think of #submit_enabled_only or similar...?

How about #filter_submitted_values, as that is what it is doing? :)

To me, the name #filter_array suggests to me that the input is filtered, and #submit_enabled_only was a bit confusing when I first read it.

+  if (!isset($element['#filter_array']) || !empty($element['#filter_array'])) {

Agree with Sun, this should only kick in if it is expressively defined, which I think makes the "+ '#filter_array' => FALSE," redundant.

@@ -488,6 +488,8 @@ function system_element_info() {
     '#process' => array('form_process_checkboxes'),
     '#theme_wrappers' => array('checkboxes'),
     '#pre_render' => array('form_pre_render_conditional_form_element'),
+    '#element_validate' => array('form_validate_filter_array'),
+    '#filter_array' => FALSE,
   );

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.

joachim’s picture

Version: 8.3.x-dev » 8.4.x-dev
Component: system.module » forms system
Issue tags: +Needs issue summary update

The issue summary is really out of date with the latest work on this issue.

+      '#filter_array' => TRUE,

Why do we need to add an option for whether to filter a checkboxes element's value? What's the use case for keeping the non-selected items in the stored value? Form API doesn't need them when you display the form later on.

andypost’s picture

Status: Needs work » Needs review
FileSize
538 bytes

Patch removes remains but IS still needs update

@joachim the main usecase is storing only checked values on form submit and it makes sense only for checkboxes form element

But since this option no longer used https://www.drupal.org/node/1910694
it was https://api.drupal.org/api/drupal/modules%21system%21system.module/funct...

So suggestion is just get rid of remains and probably separate issue to add this option to checkboxes element

joachim’s picture

Status: Needs review » Needs work

@andypost I think you've misunderstood my question.

I understand that this issue is about storing only the checked values for a checkboxes element.

My point is, why do we need an option on the checkboxes element for this? What is the use case for storing the unchecked ones as well? Why no just always filter out the unchecked values?

andypost’s picture

@joachim out of head - at least to be sure that list is not modified at frontend

andypost’s picture

Title: Replace $form['array_filter'] hack with #element_validate » Replace $form['array_filter'] hack with array_filter in book module
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update +Needs change record updates
Parent issue: » #1703164: Convert book settings to the new configuration system
FileSize
1.56 KB

@joachim thanx a lot for idea, I used to dig history and found that conversion in #1703164: Convert book settings to the new configuration system was incomplete, this falg in d7 used to filter both config arrays, so I added lost filtering

I understand that this issue is about storing only the checked values for a checkboxes element.

Actually it needs to add to https://www.drupal.org/node/1910694 that and I updated IS

andypost’s picture

joachim’s picture

I don't think this is the right way to go at all.

Having empty items in the values array for a checkboxes element is an inconvenience in FormAPI. I thought this issue was about fixing that in a way that can be used anywhere.

andypost’s picture

The code is http://cgit.drupalcode.org/drupal/tree/core/modules/book/src/Form/BookSe...

allowed_types - checkboxes are filtered already
child_type is radios so no filtering required

So #27 is right patch that removes useless remains

joachim’s picture

Ok so is this issue now just about book module?

Because when it was titled 'Replace $form['array_filter'] hack with #element_validate', I thought that meant it was about adding something that covers the whole of FormAPI. That's what I am interested in -- checkboxes values that automatically filter out the pointless unselected values.

If this issue isn't meant to be about that, should I file a new one?

andypost’s picture

Yes, that's why I renamed it.
Support for this property removed long ago before 8.0 but this is the only leftover in core