Completed
Push — new-committers ( 29cb6f...bcba16 )
by Sam
12:18 queued 33s
created

CheckboxSetField::performReadonlyTransformation()   C

Complexity

Conditions 14
Paths 10

Size

Total Lines 48
Code Lines 29

Duplication

Lines 0
Ratio 0 %
Metric Value
dl 0
loc 48
rs 5.0498
cc 14
eloc 29
nc 10
nop 0

How to fix   Complexity   

Long Method

Small methods make your code easier to understand, in particular if combined with a good name. Besides, if your method is small, finding a good name is usually much easier.

For example, if you find yourself adding comments to a method's body, this is usually a good sign to extract the commented part to a new method, and use the comment as a starting point when coming up with a good name for this new method.

Commonly applied refactorings include:

1
<?php
2
/**
3
 * Displays a set of checkboxes as a logical group.
4
 *
5
 * ASSUMPTION -> IF you pass your source as an array, you pass values as an array too. Likewise objects are handled
6
 * the same.
7
 *
8
 * Example:
9
 * <code>
10
 * new CheckboxSetField(
11
 *  $name = "topics",
12
 *  $title = "I am interested in the following topics",
13
 *  $source = array(
14
 *      "1" => "Technology",
15
 *      "2" => "Gardening",
16
 *      "3" => "Cooking",
17
 *      "4" => "Sports"
18
 *  ),
19
 *  $value = "1"
20
 * );
21
 * </code>
22
 *
23
 * <b>Saving</b>
24
 * The checkbox set field will save its data in one of ways:
25
 * - If the field name matches a many-many join on the object being edited, that many-many join will be updated to
26
 *   link to the objects selected on the checkboxes.  In this case, the keys of your value map should be the IDs of
27
 *   the database records.
28
 * - If the field name matches a database field, a comma-separated list of values will be saved to that field.  The
29
 *   keys can be text or numbers.
30
 *
31
 * @todo Document the different source data that can be used
32
 * with this form field - e.g ComponentSet, ArrayList,
33
 * array. Is it also appropriate to accept so many different
34
 * types of data when just using an array would be appropriate?
35
 *
36
 * @package forms
37
 * @subpackage fields-basic
38
 */
39
class CheckboxSetField extends OptionsetField {
40
41
	/**
42
	 * @var array
43
	 */
44
	protected $defaultItems = array();
45
46
	/**
47
	 * @todo Explain different source data that can be used with this field,
48
	 * e.g. SQLMap, ArrayList or an array.
49
	 */
50
	public function Field($properties = array()) {
51
		Requirements::css(FRAMEWORK_DIR . '/css/CheckboxSetField.css');
52
53
		$properties = array_merge($properties, array(
54
			'Options' => $this->getOptions()
55
		));
56
57
		return $this->customise($properties)->renderWith(
58
			$this->getTemplates()
59
		);
60
	}
61
62
	/**
63
	 * @return ArrayList
64
	 */
65
	public function getOptions() {
66
		$odd = 0;
67
68
		$source = $this->source;
69
		$values = $this->value;
70
		$items = array();
71
72
		// Get values from the join, if available
73
		if(is_object($this->form)) {
74
			$record = $this->form->getRecord();
75
			if(!$values && $record && $record->hasMethod($this->name)) {
76
				$funcName = $this->name;
77
				$join = $record->$funcName();
78
				if($join) {
79
					foreach($join as $joinItem) {
80
						$values[] = $joinItem->ID;
81
					}
82
				}
83
			}
84
		}
85
86
		// Source is not an array
87
		if(!is_array($source) && !is_a($source, 'SQLMap')) {
88
			if(is_array($values)) {
89
				$items = $values;
90
			} else {
91
				// Source and values are DataObject sets.
92
				if($values && is_a($values, 'SS_List')) {
93
					foreach($values as $object) {
0 ignored issues
show
Bug introduced by
The expression $values of type object|integer|double|string|boolean is not guaranteed to be traversable. How about adding an additional type check?

There are different options of fixing this problem.

  1. If you want to be on the safe side, you can add an additional type-check:

    $collection = json_decode($data, true);
    if ( ! is_array($collection)) {
        throw new \RuntimeException('$collection must be an array.');
    }
    
    foreach ($collection as $item) { /** ... */ }
    
  2. If you are sure that the expression is traversable, you might want to add a doc comment cast to improve IDE auto-completion and static analysis:

    /** @var array $collection */
    $collection = json_decode($data, true);
    
    foreach ($collection as $item) { /** .. */ }
    
  3. Mark the issue as a false-positive: Just hover the remove button, in the top-right corner of this issue for more options.

Loading history...
94
						if(is_a($object, 'DataObject')) {
95
							$items[] = $object->ID;
96
						}
97
					}
98 View Code Duplication
				} elseif($values && is_string($values)) {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
99
					if(!empty($values)) {
100
						$items = explode(',', $values);
101
						$items = str_replace('{comma}', ',', $items);
102
					} else {
103
						$items = array();
104
					}
105
				}
106
			}
107
		} else {
108
			// Sometimes we pass a singluar default value thats ! an array && !SS_List
109
			if($values instanceof SS_List || is_array($values)) {
110
				$items = $values;
111 View Code Duplication
			} else {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
112
				if($values === null) {
113
					$items = array();
114
				}
115
				else {
116
					if(!empty($values)) {
117
						$items = explode(',', $values);
118
						$items = str_replace('{comma}', ',', $items);
119
					} else {
120
						$items = array();
121
					}
122
				}
123
			}
124
		}
125
126
		if(is_array($source)) {
127
			unset($source['']);
128
		}
129
130
		$options = array();
131
132
		if ($source == null) {
133
			$source = array();
134
		}
135
136
		foreach($source as $value => $item) {
137
			if($item instanceof DataObject) {
138
				$value = $item->ID;
139
				$title = $item->Title;
0 ignored issues
show
Documentation introduced by
The property Title does not exist on object<DataObject>. Since you implemented __set, maybe consider adding a @property annotation.

Since your code implements the magic setter _set, this function will be called for any write access on an undefined variable. You can add the @property annotation to your class or interface to document the existence of this variable.

<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}

Since the property has write access only, you can use the @property-write annotation instead.

Of course, you may also just have mistyped another name, in which case you should fix the error.

See also the PhpDoc documentation for @property.

Loading history...
140
			} else {
141
				$title = $item;
142
			}
143
144
			$itemID = $this->ID() . '_' . preg_replace('/[^a-zA-Z0-9]/', '', $value);
145
			$odd = ($odd + 1) % 2;
146
			$extraClass = $odd ? 'odd' : 'even';
147
			$extraClass .= ' val' . preg_replace('/[^a-zA-Z0-9\-\_]/', '_', $value);
148
149
			$options[] = new ArrayData(array(
150
				'ID' => $itemID,
151
				'Class' => $extraClass,
152
				'Name' => "{$this->name}[{$value}]",
153
				'Value' => $value,
154
				'Title' => $title,
155
				'isChecked' => in_array($value, $items) || in_array($value, $this->defaultItems),
156
				'isDisabled' => $this->disabled || in_array($value, $this->disabledItems)
157
			));
158
		}
159
160
		$options = new ArrayList($options);
161
162
		$this->extend('updateGetOptions', $options);
163
164
		return $options;
165
	}
166
167
	/**
168
	 * Default selections, regardless of the {@link setValue()} settings.
169
	 * Note: Items marked as disabled through {@link setDisabledItems()} can still be
170
	 * selected by default through this method.
171
	 *
172
	 * @param Array $items Collection of array keys, as defined in the $source array
173
	 */
174
	public function setDefaultItems($items) {
175
		$this->defaultItems = $items;
176
		return $this;
177
	}
178
179
	/**
180
	 * @return Array
181
	 */
182
	public function getDefaultItems() {
183
		return $this->defaultItems;
184
	}
185
186
	/**
187
	 * Load a value into this CheckboxSetField
188
	 */
189
	public function setValue($value, $obj = null) {
190
		// If we're not passed a value directly, we can look for it in a relation method on the object passed as a
191
		// second arg
192 View Code Duplication
		if(!$value && $obj && $obj instanceof DataObject && $obj->hasMethod($this->name)) {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
193
			$funcName = $this->name;
194
			$value = $obj->$funcName()->getIDList();
195
		}
196
197
		parent::setValue($value, $obj);
0 ignored issues
show
Unused Code introduced by
The call to OptionsetField::setValue() has too many arguments starting with $obj.

This check compares calls to functions or methods with their respective definitions. If the call has more arguments than are defined, it raises an issue.

If a function is defined several times with a different number of parameters, the check may pick up the wrong definition and report false positives. One codebase where this has been known to happen is Wordpress.

In this case you can add the @ignore PhpDoc annotation to the duplicate definition and it will be ignored.

Loading history...
198
199
		return $this;
200
	}
201
202
	/**
203
	 * Save the current value of this CheckboxSetField into a DataObject.
204
	 * If the field it is saving to is a has_many or many_many relationship,
205
	 * it is saved by setByIDList(), otherwise it creates a comma separated
206
	 * list for a standard DB text/varchar field.
207
	 *
208
	 * @param DataObject $record The record to save into
209
	 */
210
	public function saveInto(DataObjectInterface $record) {
211
		$fieldname = $this->name;
212
		$relation = ($fieldname && $record && $record->hasMethod($fieldname)) ? $record->$fieldname() : null;
213
		if($fieldname && $record && $relation &&
214
			($relation instanceof RelationList || $relation instanceof UnsavedRelationList)) {
215
			$idList = array();
216
			if($this->value) foreach($this->value as $id => $bool) {
217
				if($bool) {
218
					$idList[] = $id;
219
				}
220
			}
221
			$relation->setByIDList($idList);
222 View Code Duplication
		} elseif($fieldname && $record) {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
223
			if($this->value) {
224
				$this->value = str_replace(',', '{comma}', $this->value);
225
				$record->$fieldname = implode(',', (array) $this->value);
226
			} else {
227
				$record->$fieldname = '';
228
			}
229
		}
230
	}
231
232
	/**
233
	 * Return the CheckboxSetField value as a string
234
	 * selected item keys.
235
	 *
236
	 * @return string
237
	 */
238 View Code Duplication
	public function dataValue() {
0 ignored issues
show
Duplication introduced by
This method seems to be duplicated in your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
239
		if($this->value && is_array($this->value)) {
240
			$filtered = array();
241
			foreach($this->value as $item) {
242
				if($item) {
243
					$filtered[] = str_replace(",", "{comma}", $item);
244
				}
245
			}
246
247
			return implode(',', $filtered);
248
		}
249
250
		return '';
251
	}
252
253
	public function performDisabledTransformation() {
254
		$clone = clone $this;
255
		$clone->setDisabled(true);
256
257
		return $clone;
258
	}
259
260
	/**
261
	 * Transforms the source data for this CheckboxSetField
262
	 * into a comma separated list of values.
263
	 *
264
	 * @return ReadonlyField
265
	 */
266
	public function performReadonlyTransformation() {
267
		$values = '';
268
		$data = array();
269
270
		$items = $this->value;
271
		if($this->source) {
272
			foreach($this->source as $source) {
273
				if(is_object($source)) {
274
					$sourceTitles[$source->ID] = $source->Title;
0 ignored issues
show
Coding Style Comprehensibility introduced by
$sourceTitles was never initialized. Although not strictly required by PHP, it is generally a good practice to add $sourceTitles = array(); before regardless.

Adding an explicit array definition is generally preferable to implicit array definition as it guarantees a stable state of the code.

Let’s take a look at an example:

foreach ($collection as $item) {
    $myArray['foo'] = $item->getFoo();

    if ($item->hasBar()) {
        $myArray['bar'] = $item->getBar();
    }

    // do something with $myArray
}

As you can see in this example, the array $myArray is initialized the first time when the foreach loop is entered. You can also see that the value of the bar key is only written conditionally; thus, its value might result from a previous iteration.

This might or might not be intended. To make your intention clear, your code more readible and to avoid accidental bugs, we recommend to add an explicit initialization $myArray = array() either outside or inside the foreach loop.

Loading history...
275
				}
276
			}
277
		}
278
279
		if($items) {
280
			// Items is a DO Set
281
			if($items instanceof SS_List) {
282
				foreach($items as $item) {
283
					$data[] = $item->Title;
284
				}
285
				if($data) $values = implode(', ', $data);
0 ignored issues
show
Bug Best Practice introduced by
The expression $data of type array is implicitly converted to a boolean; are you sure this is intended? If so, consider using ! empty($expr) instead to make it clear that you intend to check for an array without elements.

This check marks implicit conversions of arrays to boolean values in a comparison. While in PHP an empty array is considered to be equal (but not identical) to false, this is not always apparent.

Consider making the comparison explicit by using empty(..) or ! empty(...) instead.

Loading history...
286
287
			// Items is an array or single piece of string (including comma seperated string)
288
			} else {
289
				if(!is_array($items)) {
290
					$items = preg_split('/ *, */', trim($items));
291
				}
292
293
				foreach($items as $item) {
294
					if(is_array($item)) {
295
						$data[] = $item['Title'];
296
					} elseif(is_array($this->source) && !empty($this->source[$item])) {
297
						$data[] = $this->source[$item];
298
					} elseif(is_a($this->source, 'SS_List')) {
299
						$data[] = $sourceTitles[$item];
0 ignored issues
show
Bug introduced by
The variable $sourceTitles does not seem to be defined for all execution paths leading up to this point.

If you define a variable conditionally, it can happen that it is not defined for all execution paths.

Let’s take a look at an example:

function myFunction($a) {
    switch ($a) {
        case 'foo':
            $x = 1;
            break;

        case 'bar':
            $x = 2;
            break;
    }

    // $x is potentially undefined here.
    echo $x;
}

In the above example, the variable $x is defined if you pass “foo” or “bar” as argument for $a. However, since the switch statement has no default case statement, if you pass any other value, the variable $x would be undefined.

Available Fixes

  1. Check for existence of the variable explicitly:

    function myFunction($a) {
        switch ($a) {
            case 'foo':
                $x = 1;
                break;
    
            case 'bar':
                $x = 2;
                break;
        }
    
        if (isset($x)) { // Make sure it's always set.
            echo $x;
        }
    }
    
  2. Define a default value for the variable:

    function myFunction($a) {
        $x = ''; // Set a default which gets overridden for certain paths.
        switch ($a) {
            case 'foo':
                $x = 1;
                break;
    
            case 'bar':
                $x = 2;
                break;
        }
    
        echo $x;
    }
    
  3. Add a value for the missing path:

    function myFunction($a) {
        switch ($a) {
            case 'foo':
                $x = 1;
                break;
    
            case 'bar':
                $x = 2;
                break;
    
            // We add support for the missing case.
            default:
                $x = '';
                break;
        }
    
        echo $x;
    }
    
Loading history...
300
					} else {
301
						$data[] = $item;
302
					}
303
				}
304
305
				$values = implode(', ', $data);
306
			}
307
		}
308
309
		$field = $this->castedCopy('ReadonlyField');
310
		$field->setValue($values);
311
312
		return $field;
313
	}
314
315
	public function Type() {
316
		return 'optionset checkboxset';
317
	}
318
319
	public function ExtraOptions() {
320
		return FormField::ExtraOptions();
0 ignored issues
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class FormField as the method ExtraOptions() does only exist in the following sub-classes of FormField: CheckboxSetField, MemberDatetimeOptionsetField, OptionsetField. Maybe you want to instanceof check for one of these explicitly?

Let’s take a look at an example:

abstract class User
{
    /** @return string */
    abstract public function getPassword();
}

class MyUser extends User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the parent class:

    abstract class User
    {
        /** @return string */
        abstract public function getPassword();
    
        /** @return string */
        abstract public function getDisplayName();
    }
    
Loading history...
321
	}
322
323
	/**
324
	 * Validate this field
325
	 *
326
	 * @param Validator $validator
327
	 * @return bool
328
	 */
329
	public function validate($validator) {
330
		$values = $this->value;
331
		if (!$values) {
332
			return true;
333
		}
334
		$sourceArray = $this->getSourceAsArray();
335
		if (is_array($values)) {
336
			if (!array_intersect_key($sourceArray, $values)) {
337
				$validator->validationError(
338
					$this->name,
339
					_t(
340
						'CheckboxSetField.SOURCE_VALIDATION',
341
						"Please select a value within the list provided. '{value}' is not a valid option",
342
						array('value' => implode(' and ', array_diff($sourceArray, $values)))
0 ignored issues
show
Documentation introduced by
array('value' => implode...sourceArray, $values))) is of type array<string,string,{"value":"string"}>, but the function expects a string.

It seems like the type of the argument is not accepted by the function/method which you are calling.

In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug.

We suggest to add an explicit type cast like in the following example:

function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
Loading history...
343
					),
344
					"validation"
345
				);
346
				return false;
347
			}
348 View Code Duplication
		} else {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
349
			if (!in_array($this->value, $sourceArray)) {
350
				$validator->validationError(
351
					$this->name,
352
					_t(
353
						'CheckboxSetField.SOURCE_VALIDATION',
354
						"Please select a value within the list provided. '{value}' is not a valid option",
355
						array('value' => $this->value)
0 ignored issues
show
Documentation introduced by
array('value' => $this->value) is of type array<string,*,{"value":"*"}>, but the function expects a string.

It seems like the type of the argument is not accepted by the function/method which you are calling.

In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug.

We suggest to add an explicit type cast like in the following example:

function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
Loading history...
356
					),
357
					"validation"
358
				);
359
				return false;
360
			}
361
		}
362
		return true;
363
	}
364
365
}
366