Define default value for all #properties in hook_element_info() [#2560385]

select_or_other seems to be a constant source of notices "Undefined index #other…". To avoid that we should define default values in hook_element_info() for all properties that are then used in select_or_other_element_process().

Comment	File	Size	Author
#5	2560385-select_or_other-defaults-in-hook_element_info-5.patch	2.75 KB	torotil
#5
#2	2560385-select_or_other-defaults-in-hook_element_info-1.patch	551 bytes	torotil
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

31 August 2015 at 13:09

torotil created an issue. See original summary.

Comment #2

torotil CreditAttribution: torotil at more onion commented 31 August 2015 at 13:11

File	Size
2560385-select_or_other-defaults-in-hook_element_info-1.patch	551 bytes

Here is a quick patch that defines defaults to fix the two most common notices for my sites.

todo:

Check that really all properties have defaults.
Decide what the defaults should be.

Comment #3

legolasbo

he/him

Dutch

Middelburg

CreditAttribution: legolasbo at Ibuildings commented 31 August 2015 at 13:16

Issue tags:

+Quick fix

Thanks for your patch, let's complete the list of defaults and get this committed.

Comment #4

torotil CreditAttribution: torotil at more onion commented 31 August 2015 at 19:34

1 file was hidden/shown/deleted

File	Size
2560385-select_or_other-defaults-in-hook_element_info-1.patch	551 bytes

This patch makes the following changes:

First I changed the part of the code that transfers the #properties from $element to $element['select'] to only transfer the properties that are present. After that the following #properties were still used (without checking for existence) in select_or_other_element_process(): #default_value, #options, #other_unknown_defaults, #other_delimiter, #multiple, #other, #attributes, #select_type.

This means that #attributes, #options, #other_unknown_defaults and #other_delimiter were missing. This patch defines them as follows:

array(
      '#options' => array(),
      '#other_unknown_defaults' => 'other',
      '#other_delimiter' => '',
      '#attributes' => array(),
)

Additionally processing related to #disabled can be dropped. It is propagated to child-elements automatically by form_builder(). The special case for #other_size is already handled by the loop a few lines above and can be dropped.

Comment #5

torotil CreditAttribution: torotil at more onion commented 31 August 2015 at 19:15

File	Size
2560385-select_or_other-defaults-in-hook_element_info-5.patch	2.75 KB

Forgot to attach the patch.

Comment #6

torotil CreditAttribution: torotil at more onion commented 31 August 2015 at 19:15

Status:

Active

» Needs review

Comment #7

legolasbo

he/him

Dutch

Middelburg

CreditAttribution: legolasbo as a volunteer commented 1 September 2015 at 08:33

Status:	Needs review	» Needs work
Issue tags:		+Needs tests

+++ b/select_or_other.module
@@ -87,25 +90,11 @@ function select_or_other_element_process($element, &$form_state) {
-    '#size' => isset($element['#size']) ? $element['#size'] : NULL,

I don't see a default setting for #size. Doesn't this cause notices if #size isn't set?

+++ b/select_or_other.module
@@ -87,25 +90,11 @@ function select_or_other_element_process($element, &$form_state) {
+  $pass = array('#title', '#title_display', '#default_value', '#multiple',
+    '#required', '#size', '#options', '#attributes', '#empty_option',
+    '#empty_value');

Let's follow core's coding standards here and either put all values on the same line, or one value per line.

I would also like to see test coverage for this.

Comment #8

torotil CreditAttribution: torotil at more onion commented 1 September 2015 at 09:42

I don't see a default setting for #size. Doesn't this cause notices if #size isn't set?

The line with #size is removed in my patch - so no this won't throw notices ;)

Let's follow core's coding standards here and either put all values on the same line, or one value per line.
I would also like to see test coverage for this.

Both true - I'd love to see test coverage too. But it seems that that's the limit of time that I can invest at this point. If you want anything more you'll need to find someone else to do it. BTW: this function seems to be perfectly unit-testable which is rather seldom in the Drupalverse.

Good luck!