Clean-up remains of $form['array_filter'] hack with array_filter in book module

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

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

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.

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?

subscribing

Too late for D7.

Related #1120306: Do not save array_filter in system_settings_form_submit()

subscribe

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?

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.

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,
   );

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.

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

@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?

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

@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

CR updated https://www.drupal.org/node/1910694/revisions/view/8969893/10564998

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.

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

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?

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

Let's simply remove this line. It causes confusion to this day: https://drupal.stackexchange.com/questions/296592/what-does-the-array-fi...

Adding issue credits

Committed 65b6ffc and pushed to 9.1.x. Thanks!

Only took us 12 years to get a resolution here even it was just to remove legacy stuff 👵🧓

@joachim do you still feel that we need a follow-up to be able to opt-in to filtering on checkboxes?

@larowlan thanks!

I think follow-up for checkboxes element makes sense, every form with checkboxes doing array_filter() to exclude empty values, so having flag in element for that is DX++