resolve #158 #159

jackdpeterson · 2017-04-27T21:52:23Z

Throws an InvalidArgumentException when one attempts to create a DateTime element with a string for min or max that does not conform to the expected \DateTime format.

jackdpeterson · 2017-04-27T21:56:24Z

Looks like the unit tests need to be updated now to expect the element to barf when it gets garbage rather than allowing junk in that magically and mysteriously fails.

froschdesign · 2017-04-28T19:53:22Z

src/Element/DateTime.php

@@ -53,7 +54,6 @@ class DateTime extends Element implements InputProviderInterface
    public function setOptions($options)
    {
        parent::setOptions($options);
-


Please undo all code formatting changes.

froschdesign · 2017-04-28T19:53:38Z

src/Element/DateTime.php

@@ -81,6 +81,7 @@ public function getValue($returnFormattedValue = true)
            return $value;
        }
        $format = $this->getFormat();
+


Please undo all code formatting changes.

froschdesign · 2017-04-28T19:53:42Z

src/Element/DateTime.php

@@ -92,7 +93,8 @@ public function getValue($returnFormattedValue = true)
     */
    public function setFormat($format)
    {
-        $this->format = (string) $format;
+        $this->format = (string)$format;


Please undo all code formatting changes.

froschdesign · 2017-04-28T19:55:42Z

src/Element/DateTime.php

-                'inclusive' => true,
-            ]);
+        if (isset($this->attributes['min']) &&
+            \DateTime::createFromFormat(


createFromFormat here…

This logic is correct and intended to be this way. No further simplifications are necessary. Are you proposing creating and referencing unnecessary variables simply for the sake of reducing the length of the evaluation?

You can remove the DateTime::createFromFormat() from the condition and the elseif can be eliminated.

At the moment the entire block is hard to read.

Please simplify the if statements.

Honestly, I agree with @froschdesign here. The same multiline statement is used in multiple conditions, making those conditions really hard to read. Capturing to a variable makes the conditions easier to read.

I can do that on merge.

froschdesign · 2017-04-28T19:56:25Z

src/Element/DateTime.php

+                ]
+            );
+        } elseif (isset($this->attributes['min']) &&
+            ! \DateTime::createFromFormat(


…and createFromFormat here.

Please simplify these if statements.

This logic is correct and intended to be this way. No further simplifications are necessary. Are you proposing creating and referencing unnecessary variables simply for the sake of reducing the length of the evaluation?

froschdesign · 2017-04-28T19:56:34Z

src/Element/DateTime.php

-                'max'       => $this->attributes['max'],
+
+        if (isset($this->attributes['max']) &&
+            \DateTime::createFromFormat(


The same here…

Same response as above -- this logic is intended to be this way and has backing unit tests added.

froschdesign · 2017-04-28T19:56:41Z

src/Element/DateTime.php

+                ]
+            );
+        } elseif (isset($this->attributes['max']) &&
+            ! \DateTime::createFromFormat(


…and here.

See other responses. Again, intentional :-P

froschdesign · 2017-04-28T19:57:04Z

src/Element/DateTime.php

     */
    protected function getStepValidator()
    {
-        $format    = $this->getFormat();
+        $format = $this->getFormat();


Please undo all code formatting changes.

froschdesign · 2017-04-28T20:06:13Z

test/Element/DateTest.php

@@ -124,8 +124,8 @@ public function testCorrectFormatPassedToDateValidator()
    {
        $element = new DateElement('foo');
        $element->setAttributes([
-            'min'       => '2012-01-01',
-            'max'       => '2012-12-31',
+            'min'       => '01-01-2012',


The first rule: Do not change existing unit tests! Changing a unit tests always means a change in behavior.

Can you explain the differences in the values?

The change here is that right below this section is a setFormat(d-m-Y) ... which means that the previous behavior here would yield a broken scenario with values provided that do not properly construct a valid DateTime object -- something I didn't directly test with the Date element; however, since Date extends from DateTime element and uses the parent's methods this test is impacted. This is the behavior that was previously unhandled and the whole point of this PR -- the element expects a very specific format and when it doesn't get that format it fails silently rather than throwing exceptional conditions when handed garbage. If I'm going to keep this unit test mostly the same then I would make this test have an expectedException and create an additional test where the right date is provided and input is properly validated.

froschdesign · 2017-04-28T20:06:22Z

test/Element/DateTimeLocalTest.php

@@ -19,8 +19,8 @@ public function testProvidesInputSpecificationThatIncludesValidatorsBasedOnAttri
        $element = new DateTimeLocalElement('foo');
        $element->setAttributes([
            'inclusive' => true,
-            'min'       => '2000-01-01T00:00Z',
-            'max'       => '2001-01-01T00:00Z',
+            'min'       => '2000-01-01T00:00',


The same here.

Same story -- this is the format that the createFromFormat expects.

This test was updated to be consistent w/ the documentation. Additional test added when an invalid min/max specification like the one above is added that would result in an InvalidArgumentException.

froschdesign · 2017-04-28T20:06:39Z

test/Element/DateTimeLocalTest.php

@@ -40,11 +40,11 @@ public function testProvidesInputSpecificationThatIncludesValidatorsBasedOnAttri
            switch ($class) {
                case 'Zend\Validator\GreaterThan':
                    $this->assertTrue($validator->getInclusive());
-                    $this->assertEquals('2000-01-01T00:00Z', $validator->getMin());
+                    $this->assertEquals('2000-01-01T00:00', $validator->getMin());


The same here.

This is the format used in both the documentation and what conforms to the requested \DateTime::createFromFormat string.

This test was updated to be consistent w/ the documentation. Additional test added when an invalid min/max specification like the one above is added that would result in an InvalidArgumentException.

I think the test changes make sense. The originals were based on an invalid assumption.

froschdesign · 2017-04-28T20:07:07Z

test/Element/MonthTest.php

@@ -24,6 +24,8 @@ public function testProvidesInputSpecificationThatIncludesValidatorsBasedOnAttri
            'step'      => '1',
        ]);

+        $element->setFormat('Y-m');


Why is this needed?

froschdesign · 2017-04-28T20:07:12Z

test/Element/WeekTest.php

@@ -24,6 +25,8 @@ public function testProvidesInputSpecificationThatIncludesValidatorsBasedOnAttri
            'step'      => '1',
        ]);

+        $element->setFormat('Y-\WW');


Why is this needed?

jackdpeterson · 2017-04-29T03:23:47Z

I suppose there's also a broader conversation to be had about the changes that I'm implementing here. In the case of the setFormat() bits, I'm assuming that the min and max validation are to be ran off of the same format as the actual input format. This may be an incorrect approach. Rather, I think I'm going to modify this a little bit where the min and max MUST follow the convention for self::DATETIME_FORMAT = 'Y-m-d\TH:iP'; as was the case with my first pass prior to squashing in various other changes. The benefit of this approach would be that the date is always specified in a consistent manner rather than evaluated based on the then-current format.

Either way, the current implementation has problems and I think the above should be discussed. Locking down to one format for the min / max seems to be the most appropriate choice. Either way, unit tests must be modified.

DateTime, Date, Month element with 'min' or 'max' string specification(s) that does not conform to the expected \DateTime format for the given element rather than mysteriously fail when given valid data on input.

jackdpeterson · 2017-05-01T18:39:15Z

Branch updated w/ additional unit tests to reflect which scenarios fail. Also fixed one unit test related to the DateTimeLocalElement where an invalid specification was provided that was inconsistent w/ the documentation.

jackdpeterson · 2017-05-02T18:47:07Z

Are there other comments on this PR or can we move this into the next minor release of Zend-Form?

froschdesign · 2017-05-04T06:34:11Z

@jackdpeterson
Throw an exception is a new behaviour.
Can we cover the old and the new behaviour with unit tests to prevent any problems in existing applications / forms?

jackdpeterson · 2017-05-04T16:00:53Z

@froschdesign -- see the changes for the tests folder. I also added a unit tests for specific areas where invalid specifications are provided.

froschdesign · 2017-05-08T14:51:59Z

Btw. if the documentation is wrong or a bug is included, then we must fix this. See:

$dateTimeLocal->setAttributes([
    'min'  => '2010-01-01T00:00:00',
    'max'  => '2020-01-01T00:00:00',
    'step' => '1', // minutes; default step interval is 1 min
]);
$dateTimeLocal->setOptions([
    'format' => 'Y-m-d\TH:i',
]);

https://docs.zendframework.com/zend-form/element/date-time-local/#basic-usage

weierophinney · 2017-05-18T14:37:10Z

src/Element/DateTime.php

-                'inclusive' => true,
-            ]);
+        if (isset($this->attributes['min']) &&
+            \DateTime::createFromFormat(


Honestly, I agree with @froschdesign here. The same multiline statement is used in multiple conditions, making those conditions really hard to read. Capturing to a variable makes the conditions easier to read.

I can do that on merge.

weierophinney · 2017-05-18T14:38:48Z

test/Element/DateTimeLocalTest.php

@@ -40,11 +40,11 @@ public function testProvidesInputSpecificationThatIncludesValidatorsBasedOnAttri
            switch ($class) {
                case 'Zend\Validator\GreaterThan':
                    $this->assertTrue($validator->getInclusive());
-                    $this->assertEquals('2000-01-01T00:00Z', $validator->getMin());
+                    $this->assertEquals('2000-01-01T00:00', $validator->getMin());


I think the test changes make sense. The originals were based on an invalid assumption.

resolve #158

Added a new method, `valueIsValidDateTimeFormat()`, that accepts a value and then passes it to `DateTime::createFromFormat` with the current instance's `DATETIME_FORMAT` in order to determine if a valid value is returned. The various new conditionals added in #159 are then modified to call on this method instead of doing the calls inline. The result is less code, and more readable conditionals.

Close #159 Fixes #158

Forward port #159

weierophinney · 2017-05-18T14:55:53Z

Thanks, @jackdpeterson

jackdpeterson force-pushed the 158 branch 2 times, most recently from fc3e5ff to b520f1a Compare April 28, 2017 18:38

froschdesign reviewed Apr 28, 2017

View reviewed changes

jackdpeterson force-pushed the 158 branch 3 times, most recently from c871a5d to bc72ba9 Compare May 1, 2017 18:24

Throw an InvalidArgumentException when one attempts to create a

22ba199

DateTime, Date, Month element with 'min' or 'max' string specification(s) that does not conform to the expected \DateTime format for the given element rather than mysteriously fail when given valid data on input.

jackdpeterson force-pushed the 158 branch from bc72ba9 to 22ba199 Compare May 1, 2017 18:28

weierophinney approved these changes May 18, 2017

View reviewed changes

weierophinney merged commit 22ba199 into zendframework:master May 18, 2017

weierophinney added a commit that referenced this pull request May 18, 2017

Merge pull request #159 from jackdpeterson/158

6efc5c6

resolve #158

weierophinney added a commit that referenced this pull request May 18, 2017

Added CHANGELOG for #159

4cc04fa

weierophinney added a commit that referenced this pull request May 18, 2017

Merge branch 'hotfix/159'

95beb4d

Close #159 Fixes #158

weierophinney added a commit that referenced this pull request May 18, 2017

Merge branch 'hotfix/159' into develop

5318953

Forward port #159

jackdpeterson deleted the 158 branch June 2, 2017 22:46

resolve #158 #159

resolve #158 #159

Conversation

jackdpeterson commented Apr 27, 2017 • edited Loading

jackdpeterson commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackdpeterson May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackdpeterson May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackdpeterson Apr 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackdpeterson commented Apr 29, 2017

jackdpeterson commented May 1, 2017 • edited Loading

jackdpeterson commented May 2, 2017

froschdesign commented May 4, 2017

jackdpeterson commented May 4, 2017

froschdesign commented May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented May 18, 2017

jackdpeterson commented Apr 27, 2017 •

edited

Loading

jackdpeterson May 1, 2017 •

edited

Loading

jackdpeterson May 1, 2017 •

edited

Loading

jackdpeterson Apr 29, 2017 •

edited

Loading

jackdpeterson commented May 1, 2017 •

edited

Loading

froschdesign commented May 8, 2017 •

edited

Loading