Page MenuHomePhabricator

(Temporary) Fix for radiobuttons not working with mapping templates
Open, Needs TriagePublic

Description

As of SF 3.5, radio-buttons use the label as their value. This possibily breaks "show on select" and leads to the label beeing saved instead of the value.

This is a temporary fix. It will not work if a mapping template is used AND the id is numeric. Sadly, PHP cannot distuingish between sequential and associative arrays. To fix this properly, a bigger refactoring would be needed.

Index: includes/forminputs/SF_RadioButtonInput.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- includes/forminputs/SF_RadioButtonInput.php	(revision da3c07aaebe5a6f81e1ebb9716d4b4e9f9ebd3da)
+++ includes/forminputs/SF_RadioButtonInput.php	(revision )
@@ -19,10 +19,12 @@
 	public static function getHTML( $cur_value, $input_name, $is_mandatory, $is_disabled, $other_args ) {
 		global $sfgTabIndex, $sfgFieldNum, $sfgShowOnSelect;
 
+		$possible_values = $other_args['possible_values'];
+
 		// Standardize $cur_value
 		if ( is_null( $cur_value ) ) { $cur_value = ''; }
 
-		if ( ( $possible_values = $other_args['possible_values'] ) == null ) {
+		if ( $possible_values == null ) {
 			// If it's a Boolean property, display 'Yes' and 'No'
 			// as the values.
 			if ( array_key_exists( 'property_type', $other_args ) &&
@@ -64,7 +66,13 @@
 		}
 		$itemAttrs = array( 'class' => $itemClass );
 
-		foreach ( $possible_values as $possible_value ) {
+		foreach ( $possible_values as $value => $possible_value ) {
+
+			// When there's no mapping template
+			if (is_numeric($value)) {
+				$value = $possible_value;
+			}
+
 			$sfgTabIndex++;
 			$sfgFieldNum++;
 			$input_id = "input_$sfgFieldNum";
@@ -84,7 +92,7 @@
 			if ( $is_disabled ) {
 				$radiobutton_attrs['disabled'] = true;
 			}
-			if ( $possible_value === '' ) { // blank/"None" value
+			if ( $value === '' ) { // blank/"None" value
 				$label = wfMessage( 'sf_formedit_none' )->text();
 			} elseif (
 				array_key_exists( 'value_labels', $other_args ) &&
@@ -103,7 +111,7 @@
 				// somehow leads to the string "on" being passed
 				// to the page.
 				//Html::input( $input_name, $possible_value, 'radio', $radiobutton_attrs ) . " $label" ) . "\n";
-				Xml::radio( $input_name, $possible_value, $isChecked, $radiobutton_attrs ) . " $label" ) . "\n";
+				Xml::radio( $input_name, $value, $isChecked, $radiobutton_attrs ) . " $label" ) . "\n";
 		}
 
 		$spanClass = 'radioButtonSpan';

Event Timeline

Fannon created this task.Mar 3 2016, 4:18 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 3 2016, 4:18 PM
Fannon updated the task description. (Show Details)Mar 3 2016, 4:38 PM

Thanks for taking a look at the code!

You are very welcome to use developer access to submit the proposed code changes as a Git branch directly into Gerrit which makes it easier to review them quickly and provide feedback.
If you don't want to set up Git/Gerrit, you can also use the Gerrit Patch Uploader. Thanks again!

Fannon updated the task description. (Show Details)May 13 2016, 7:39 AM

@cicalese Maybe this is of interest to you - the radio button input was broken when using mapping templates / labels. It was correctly displaying the label, but incorrectly stores the label instead of the actual value-id when the form is saved.

I've encountered a big problem here, as the possible values array is now associative and there's no way in PHP to discern it from a normal array.

Therefore, I had to duck-type which is a rather ugly hack:

// When there's no mapping template
+			if (is_numeric($value)) {
+				$value = $possible_value;
+			}

Do you have an oppinion on how to do/fix this correctly?

Interesting. Radio buttons were definitely tested/working when the mapping parameters were initially added to SemanticForms.

Aklapper removed Yaron_Koren as the assignee of this task.Jun 19 2020, 4:16 PM
Aklapper added a subscriber: Yaron_Koren.

This task has been assigned to the same task owner for more than two years. Resetting task assignee due to inactivity, to decrease task cookie-licking and to get a slightly more realistic overview of plans. Please feel free to assign this task to yourself again if you still realistically work or plan to work on this task - it would be welcome!

For tips how to manage individual work in Phabricator (noisy notifications, lists of task, etc.), see https://phabricator.wikimedia.org/T228575#6237124 for available options.
(For the records, two emails were sent to assignee addresses before resetting assignees. See T228575 for more info and for potential feedback. Thanks!)