From 24d0335a0a0eba660de0e902cfe4358a1f445e53 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 19 Aug 2020 14:26:55 +0100 Subject: [PATCH] MDL-69507 duration form field: modernise coding style --- lib/form/duration.php | 37 ++++---- lib/form/tests/duration_test.php | 140 ++++++++++++++++++------------- 2 files changed, 101 insertions(+), 76 deletions(-) diff --git a/lib/form/duration.php b/lib/form/duration.php index 20c0d053dde..1884e3ec3f3 100644 --- a/lib/form/duration.php +++ b/lib/form/duration.php @@ -43,11 +43,12 @@ require_once($CFG->libdir . '/form/text.php'); */ class MoodleQuickForm_duration extends MoodleQuickForm_group { /** - * Control the fieldnames for form elements + * Control the field names for form elements * optional => if true, show a checkbox beside the element to turn it on (or off) + * defaultunit => which unit is default when the form is blank (default Minutes). * @var array */ - protected $_options = array('optional' => false, 'defaultunit' => MINSECS); + protected $_options = ['optional' => false, 'defaultunit' => MINSECS]; /** @var array associative array of time units (days, hours, minutes, seconds) */ private $_units = null; @@ -55,7 +56,7 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { /** * constructor * - * @param string $elementName Element's name + * @param ?string $elementName Element's name * @param mixed $elementLabel Label(s) for an element * @param array $options Options to control the element's display. Recognised values are * 'optional' => true/false - whether to display an 'enabled' checkbox next to the element. @@ -66,16 +67,15 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { * @param mixed $attributes Either a typical HTML attribute string or an associative array */ public function __construct($elementName = null, $elementLabel = null, - $options = array(), $attributes = null) { - // TODO MDL-52313 Replace with the call to parent::__construct(). - HTML_QuickForm_element::__construct($elementName, $elementLabel, $attributes); + $options = [], $attributes = null) { + parent::__construct($elementName, $elementLabel, $attributes); $this->_persistantFreeze = true; $this->_appendName = true; $this->_type = 'duration'; // Set the options, do not bother setting bogus ones if (!is_array($options)) { - $options = array(); + $options = []; } $this->_options['optional'] = !empty($options['optional']); if (isset($options['defaultunit'])) { @@ -111,7 +111,7 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { * @deprecated since Moodle 3.1 */ public function MoodleQuickForm_duration($elementName = null, $elementLabel = null, - $options = array(), $attributes = null) { + $options = [], $attributes = null) { debugging('Use of class name as constructor is deprecated', DEBUG_DEVELOPER); self::__construct($elementName, $elementLabel, $options, $attributes); } @@ -123,13 +123,13 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { */ public function get_units() { if (is_null($this->_units)) { - $this->_units = array( + $this->_units = [ WEEKSECS => get_string('weeks'), DAYSECS => get_string('days'), HOURSECS => get_string('hours'), MINSECS => get_string('minutes'), 1 => get_string('seconds'), - ); + ]; } return $this->_units; } @@ -158,14 +158,14 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { */ public function seconds_to_unit($seconds) { if ($seconds == 0) { - return array(0, $this->_options['defaultunit']); + return [0, $this->_options['defaultunit']]; } foreach ($this->get_units_used() as $unit => $notused) { if (fmod($seconds, $unit) == 0) { - return array($seconds / $unit, $unit); + return [$seconds / $unit, $unit]; } } - return array($seconds, 1); + return [$seconds, 1]; } /** @@ -174,12 +174,12 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { function _createElements() { $attributes = $this->getAttributes(); if (is_null($attributes)) { - $attributes = array(); + $attributes = []; } if (!isset($attributes['size'])) { $attributes['size'] = 3; } - $this->_elements = array(); + $this->_elements = []; // E_STRICT creating elements without forms is nasty because it internally uses $this $number = $this->createFormElement('text', 'number', get_string('time', 'form'), $attributes, true); @@ -226,7 +226,7 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { } if (!is_array($value)) { list($number, $unit) = $this->seconds_to_unit($value); - $value = array('number' => $number, 'timeunit' => $unit); + $value = ['number' => $number, 'timeunit' => $unit]; // If optional, default to off, unless a date was provided if ($this->_options['optional']) { $value['enabled'] = $number != 0; @@ -245,7 +245,6 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { } $caller->setType($arg[0] . '[number]', PARAM_FLOAT); return parent::onQuickFormEvent($event, $arg, $caller); - break; default: return parent::onQuickFormEvent($event, $arg, $caller); @@ -270,7 +269,7 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { * * @param HTML_QuickForm_Renderer $renderer An HTML_QuickForm_Renderer object * @param bool $required Whether a group is required - * @param string $error An error message associated with a group + * @param ?string $error An error message associated with a group */ function accept(&$renderer, $required = false, $error = null) { $renderer->renderElement($this, $required, $error); @@ -286,7 +285,7 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { */ function exportValue(&$submitValues, $assoc = false) { // Get the values from all the child elements. - $valuearray = array(); + $valuearray = []; foreach ($this->_elements as $element) { $thisexport = $element->exportValue($submitValues[$this->getName()], true); if (!is_null($thisexport)) { diff --git a/lib/form/tests/duration_test.php b/lib/form/tests/duration_test.php index 4ce92804dd2..9a5a1a66090 100644 --- a/lib/form/tests/duration_test.php +++ b/lib/form/tests/duration_test.php @@ -47,7 +47,7 @@ class core_form_duration_testcase extends basic_testcase { * * @return MoodleQuickForm */ - protected function get_test_form() { + protected function get_test_form(): MoodleQuickForm { $form = new temp_form_duration(); return $form->getform(); } @@ -57,27 +57,26 @@ class core_form_duration_testcase extends basic_testcase { * * @return array with two elements, a MoodleQuickForm and a MoodleQuickForm_duration. */ - protected function get_test_form_and_element() { + protected function get_test_form_and_element(): array { $mform = $this->get_test_form(); $element = $mform->addElement('duration', 'duration'); return [$mform, $element]; } /** - * Testcase for testing contructor. - * - * @expectedException coding_exception + * Test the constructor error handling. */ - public function test_constructor() { + public function test_constructor_rejects_invalid_unit(): void { // Test trying to create with an invalid unit. $mform = $this->get_test_form(); + $this->expectException('coding_exception'); $mform->addElement('duration', 'testel', null, ['defaultunit' => 123, 'optional' => false]); } /** - * Test contructor only some units. + * Test constructor only some units. */ - public function test_constructor_limited_units() { + public function test_constructor_limited_units(): void { $mform = $this->get_test_form(); $mform->addElement('duration', 'testel', null, ['units' => [MINSECS, 1], 'optional' => false]); $html = $mform->toHtml(); @@ -90,7 +89,7 @@ class core_form_duration_testcase extends basic_testcase { /** * Testcase for testing units (seconds, minutes, hours and days) */ - public function test_get_units() { + public function test_get_units(): void { [$mform, $element] = $this->get_test_form_and_element(); $units = $element->get_units(); $this->assertEquals($units, [1 => get_string('seconds'), 60 => get_string('minutes'), @@ -98,66 +97,93 @@ class core_form_duration_testcase extends basic_testcase { } /** - * Testcase for testing conversion of seconds to the best possible unit + * Data provider for {@see test_seconds_to_unit()}. + * + * @return array test cases. */ - public function test_seconds_to_unit() { - [$mform, $element] = $this->get_test_form_and_element(); - $this->assertEquals([0, MINSECS], $element->seconds_to_unit(0)); // Zero minutes, for a nice default unit. - $this->assertEquals([1, 1], $element->seconds_to_unit(1)); - $this->assertEquals([3601, 1], $element->seconds_to_unit(3601)); - $this->assertEquals([1, MINSECS], $element->seconds_to_unit(60)); - $this->assertEquals([3, MINSECS], $element->seconds_to_unit(180)); - $this->assertEquals([1, HOURSECS], $element->seconds_to_unit(3600)); - $this->assertEquals([2, HOURSECS], $element->seconds_to_unit(7200)); - $this->assertEquals([1, DAYSECS], $element->seconds_to_unit(86400)); - $this->assertEquals([25, HOURSECS], $element->seconds_to_unit(90000)); + public function seconds_to_unit_cases(): array { + return [ + [[0, MINSECS], 0], // Zero minutes, for a nice default unit. + [[1, 1], 1], + [[3601, 1], 3601], + [[1, MINSECS], 60], + [[3, MINSECS], 180], + [[1, HOURSECS], 3600], + [[2, HOURSECS], 7200], + [[1, DAYSECS], 86400], + [[25, HOURSECS], 90000], + ]; + } + /** + * Testcase for testing conversion of seconds to the best possible unit. + * + * @dataProvider seconds_to_unit_cases + * @param array $expected expected return value from seconds_to_unit + * @param int $seconds value to pass to seconds_to_unit + */ + public function test_seconds_to_unit(array $expected, int $seconds): void { + [, $element] = $this->get_test_form_and_element(); + $this->assertEquals($expected, $element->seconds_to_unit($seconds)); + } + + /** + * Testcase for testing conversion of seconds to the best possible unit with a non-default default unit. + */ + public function test_seconds_to_unit_different_default_unit() { + $mform = $this->get_test_form(); $element = $mform->addElement('duration', 'testel', null, ['defaultunit' => DAYSECS, 'optional' => false]); - $this->assertEquals([0, DAYSECS], $element->seconds_to_unit(0)); // Zero minutes, for a nice default unit. + $this->assertEquals([0, DAYSECS], $element->seconds_to_unit(0)); + } + + /** + * Data provider for {@see test_export_value()}. + * + * @return array test cases. + */ + public function export_value_cases(): array { + return [ + [10, '10', 1], + [180, '3', MINSECS], + [90, '1.5', MINSECS], + [7200, '2', HOURSECS], + [86400, '1', DAYSECS], + [0, '0', HOURSECS], + [0, '10', 1, 0, true], + [20, '20', 1, 1, true], + [0, '10', 1, 0, true, ''], + [20, '20', 1, 1, true, ''], + ]; } /** * Testcase to check generated timestamp + * + * @dataProvider export_value_cases + * @param int $expected Expected value returned by the element. + * @param string $number Number entered into the element. + * @param int $unit Unit selected in the element. + * @param int $enabled Whether the enabled checkbox on the form was selected. (Only used if $optional is true.) + * @param bool $optional Whether the element has the optional option on. + * @param string|null $label The element's label. */ - public function test_exportValue() { + public function test_export_value(int $expected, string $number, int $unit, int $enabled = 0, + bool $optional = false, ?string $label = null): void { + + // Create the test element. $mform = $this->get_test_form(); - $el = $mform->addElement('duration', 'testel'); - $values = ['testel' => ['number' => 10, 'timeunit' => 1]]; - $this->assertEquals(['testel' => 10], $el->exportValue($values, true)); - $this->assertEquals(10, $el->exportValue($values)); - $values = ['testel' => ['number' => 3, 'timeunit' => MINSECS]]; - $this->assertEquals(['testel' => 180], $el->exportValue($values, true)); - $this->assertEquals(180, $el->exportValue($values)); - $values = ['testel' => ['number' => 1.5, 'timeunit' => MINSECS]]; - $this->assertEquals(['testel' => 90], $el->exportValue($values, true)); - $this->assertEquals(90, $el->exportValue($values)); - $values = ['testel' => ['number' => 2, 'timeunit' => HOURSECS]]; - $this->assertEquals(['testel' => 7200], $el->exportValue($values, true)); - $this->assertEquals(7200, $el->exportValue($values)); - $values = ['testel' => ['number' => 1, 'timeunit' => DAYSECS]]; - $this->assertEquals(['testel' => 86400], $el->exportValue($values, true)); - $this->assertEquals(86400, $el->exportValue($values)); - $values = ['testel' => ['number' => 0, 'timeunit' => HOURSECS]]; - $this->assertEquals(['testel' => 0], $el->exportValue($values, true)); - $this->assertEquals(0, $el->exportValue($values)); + $el = $mform->addElement('duration', 'testel', $label, $optional ? ['optional' => true] : []); - $el = $mform->addElement('duration', 'testel', null, ['optional' => true]); - $values = ['testel' => ['number' => 10, 'timeunit' => 1]]; - $this->assertEquals(['testel' => 0], $el->exportValue($values, true)); - $this->assertEquals(0, $el->exportValue($values)); - $values = ['testel' => ['number' => 20, 'timeunit' => 1, 'enabled' => 1]]; - $this->assertEquals(['testel' => 20], $el->exportValue($values, true)); - $this->assertEquals(20, $el->exportValue($values)); + // Prepare the submitted values. + $values = ['testel' => ['number' => $number, 'timeunit' => $unit]]; + if ($optional) { + $values['testel']['enabled'] = $enabled; + } - // Optional element. - $el2 = $mform->addElement('duration', 'testel', '', ['optional' => true]); - $values = ['testel' => ['number' => 10, 'timeunit' => 1, 'enabled' => 1]]; - $this->assertEquals(['testel' => 10], $el2->exportValue($values, true)); - $this->assertEquals(10, $el2->exportValue($values)); - $values = ['testel' => ['number' => 10, 'timeunit' => 1, 'enabled' => 0]]; - $this->assertEquals(['testel' => 0], $el2->exportValue($values, true)); - $this->assertEquals(null, $el2->exportValue($values)); + // Test. + $this->assertEquals(['testel' => $expected], $el->exportValue($values, true)); + $this->assertEquals($expected, $el->exportValue($values)); } }