Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

resolve #158 #159

Merged
merged 1 commit into from
May 18, 2017
Merged

resolve #158 #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Element/Date.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

class Date extends DateTimeElement
{

const DATETIME_FORMAT = 'Y-m-d';

/**
* Seed attributes
*
Expand Down
66 changes: 55 additions & 11 deletions src/Element/DateTime.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use DateInterval;
use DateTime as PhpDateTime;
use Zend\Form\Element;
use Zend\Form\Exception\InvalidArgumentException;
use Zend\InputFilter\InputProviderInterface;
use Zend\Validator\Date as DateValidator;
use Zend\Validator\DateStep as DateStepValidator;
Expand Down Expand Up @@ -120,17 +121,60 @@ protected function getValidators()
$validators = [];
$validators[] = $this->getDateValidator();

if (isset($this->attributes['min'])) {
$validators[] = new GreaterThanValidator([
'min' => $this->attributes['min'],
'inclusive' => true,
]);
if (isset($this->attributes['min']) &&
\DateTime::createFromFormat(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createFromFormat here…

Copy link
Contributor Author

@jackdpeterson jackdpeterson May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please simplify the if statements.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

static::DATETIME_FORMAT,
$this->attributes['min']
) instanceof \DateTimeInterface
) {
$validators[] = new GreaterThanValidator(
[
'min' => $this->attributes['min'],
'inclusive' => true,
]
);
} elseif (isset($this->attributes['min']) &&
! \DateTime::createFromFormat(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…and createFromFormat here.

Please simplify these if statements.

Copy link
Contributor Author

@jackdpeterson jackdpeterson May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

static::DATETIME_FORMAT,
$this->attributes['min']
) instanceof \DateTimeInterface
) {
throw new InvalidArgumentException(
sprintf(
'%1$s expects "min" to conform to %2$s; received "%3$s"',
__METHOD__,
static::DATETIME_FORMAT,
$this->attributes['min']
)
);
}
if (isset($this->attributes['max'])) {
$validators[] = new LessThanValidator([
'max' => $this->attributes['max'],

if (isset($this->attributes['max']) &&
\DateTime::createFromFormat(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same here…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

static::DATETIME_FORMAT,
$this->attributes['max']
) instanceof \DateTimeInterface
) {
$validators[] = new LessThanValidator(
[
'max' => $this->attributes['max'],
'inclusive' => true,
]);
]
);
} elseif (isset($this->attributes['max']) &&
! \DateTime::createFromFormat(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…and here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See other responses. Again, intentional :-P

static::DATETIME_FORMAT,
$this->attributes['max']
) instanceof \DateTimeInterface
) {
throw new InvalidArgumentException(
sprintf(
'%1$s expects "max" to conform to %2$s; received "%3$s"',
__METHOD__,
static::DATETIME_FORMAT,
$this->attributes['max']
)
);
}
if (! isset($this->attributes['step'])
|| 'any' !== $this->attributes['step']
Expand All @@ -145,7 +189,7 @@ protected function getValidators()
/**
* Retrieves a Date Validator configured for a DateTime Input type
*
* @return DateTime
* @return DateValidator
*/
protected function getDateValidator()
{
Expand All @@ -155,7 +199,7 @@ protected function getDateValidator()
/**
* Retrieves a DateStep Validator configured for a DateTime Input type
*
* @return DateTime
* @return DateStepValidator
*/
protected function getStepValidator()
{
Expand Down
3 changes: 3 additions & 0 deletions src/Element/DateTimeLocal.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@

class DateTimeLocal extends DateTime
{

const DATETIME_LOCAL_FORMAT = 'Y-m-d\TH:i';

const DATETIME_FORMAT = self::DATETIME_LOCAL_FORMAT;

/**
* Seed attributes
*
Expand Down
3 changes: 3 additions & 0 deletions src/Element/Month.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@

class Month extends DateTime
{

const DATETIME_FORMAT = 'Y-m';

/**
* Seed attributes
*
Expand Down
3 changes: 3 additions & 0 deletions src/Element/Time.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@

class Time extends DateTime
{

const DATETIME_FORMAT = 'H:i:s';

/**
* Seed attributes
*
Expand Down
34 changes: 34 additions & 0 deletions src/Element/Week.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

use Zend\Validator\DateStep as DateStepValidator;
use Zend\Validator\Regex as RegexValidator;
use Zend\Validator\GreaterThan as GreaterThanValidator;
use Zend\Validator\LessThan as LessThanValidator;

class Week extends DateTime
{
Expand Down Expand Up @@ -52,4 +54,36 @@ protected function getStepValidator()
'step' => new \DateInterval("P{$stepValue}W"),
]);
}

/**
* @see https://bugs.php.net/bug.php?id=74511
* @return array
*/
protected function getValidators()
{
if ($this->validators) {
return $this->validators;
}
$validators = [];
$validators[] = $this->getDateValidator();
if (isset($this->attributes['min'])) {
$validators[] = new GreaterThanValidator([
'min' => $this->attributes['min'],
'inclusive' => true,
]);
}
if (isset($this->attributes['max'])) {
$validators[] = new LessThanValidator([
'max' => $this->attributes['max'],
'inclusive' => true,
]);
}
if (! isset($this->attributes['step'])
|| 'any' !== $this->attributes['step']
) {
$validators[] = $this->getStepValidator();
}
$this->validators = $validators;
return $this->validators;
}
}
30 changes: 30 additions & 0 deletions test/Element/DateTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use DateTime;
use PHPUnit\Framework\TestCase;
use Zend\Form\Element\Date as DateElement;
use Zend\Form\Exception\InvalidArgumentException;

/**
* @covers \Zend\Form\Element\Date
Expand Down Expand Up @@ -158,4 +159,33 @@ public function testStepValidatorIgnoresDaylightSavings()
}
}
}

public function testFailsWithInvalidMinSpecification()
{
$element = new DateElement('foo');
$element->setAttributes(
[
'inclusive' => true,
'min' => '2000-01-01T00',
'step' => '1',
]
);

$this->expectException(InvalidArgumentException::class);
$element->getInputSpecification();
}

public function testFailsWithInvalidMaxSpecification()
{
$element = new DateElement('foo');
$element->setAttributes(
[
'inclusive' => true,
'max' => '2001-01-01T00',
'step' => '1',
]
);
$this->expectException(InvalidArgumentException::class);
$element->getInputSpecification();
}
}
38 changes: 34 additions & 4 deletions test/Element/DateTimeLocalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

use PHPUnit\Framework\TestCase;
use Zend\Form\Element\DateTimeLocal as DateTimeLocalElement;
use Zend\Form\Exception\InvalidArgumentException;

class DateTimeLocalTest extends TestCase
{
Expand All @@ -19,8 +20,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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'max' => '2001-01-01T00:00',
'step' => '1',
]);

Expand All @@ -40,11 +41,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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

break;
case 'Zend\Validator\LessThan':
$this->assertTrue($validator->getInclusive());
$this->assertEquals('2001-01-01T00:00Z', $validator->getMax());
$this->assertEquals('2001-01-01T00:00', $validator->getMax());
break;
case 'Zend\Validator\DateStep':
$dateInterval = new \DateInterval('PT1M');
Expand All @@ -55,4 +56,33 @@ public function testProvidesInputSpecificationThatIncludesValidatorsBasedOnAttri
}
}
}

public function testFailsWithInvalidMinSpecification()
{
$element = new DateTimeLocalElement('foo');
$element->setAttributes(
[
'inclusive' => true,
'min' => '2001-01-01T00:00Z',
'step' => '1',
]
);

$this->expectException(InvalidArgumentException::class);
$element->getInputSpecification();
}

public function testFailsWithInvalidMaxSpecification()
{
$element = new DateTimeLocalElement('foo');
$element->setAttributes(
[
'inclusive' => true,
'max' => '2001-01-01T00:00Z',
'step' => '1',
]
);
$this->expectException(InvalidArgumentException::class);
$element->getInputSpecification();
}
}
30 changes: 30 additions & 0 deletions test/Element/DateTimeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use DateTime;
use PHPUnit\Framework\TestCase;
use Zend\Form\Element\DateTime as DateTimeElement;
use Zend\Form\Exception\InvalidArgumentException;

class DateTimeTest extends TestCase
{
Expand Down Expand Up @@ -119,4 +120,33 @@ public function testSetFormatWithOptions()

$this->assertSame($format, $element->getFormat());
}

public function testFailsWithInvalidMinSpecification()
{
$element = new DateTimeElement('foo');
$element->setAttributes(
[
'inclusive' => true,
'min' => '2000-01-01T00',
'step' => '1',
]
);

$this->expectException(InvalidArgumentException::class);
$element->getInputSpecification();
}

public function testFailsWithInvalidMaxSpecification()
{
$element = new DateTimeElement('foo');
$element->setAttributes(
[
'inclusive' => true,
'max' => '2001-01-01T00',
'step' => '1',
]
);
$this->expectException(InvalidArgumentException::class);
$element->getInputSpecification();
}
}
30 changes: 30 additions & 0 deletions test/Element/TimeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

use PHPUnit\Framework\TestCase;
use Zend\Form\Element\Time as TimeElement;
use Zend\Form\Exception\InvalidArgumentException;

class TimeTest extends TestCase
{
Expand Down Expand Up @@ -58,4 +59,33 @@ public function testProvidesInputSpecificationThatIncludesValidatorsBasedOnAttri
}
}
}

public function testFailsWithInvalidMinSpecification()
{
$element = new TimeElement('foo');
$element->setAttributes(
[
'inclusive' => true,
'min' => '00:00',
'step' => '1',
]
);

$this->expectException(InvalidArgumentException::class);
$element->getInputSpecification();
}

public function testFailsWithInvalidMaxSpecification()
{
$element = new TimeElement('foo');
$element->setAttributes(
[
'inclusive' => true,
'max' => '00:00',
'step' => '1',
]
);
$this->expectException(InvalidArgumentException::class);
$element->getInputSpecification();
}
}
14 changes: 7 additions & 7 deletions test/Element/WeekTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,13 @@ public function testProvidesInputSpecificationThatIncludesValidatorsBasedOnAttri
public function weekValuesDataProvider()
{
return [
// value expected
['2012-W01', true],
['2012-W52', true],
['2012-01', false],
['W12-2012', false],
['2012-W1', false],
['12-W01', false],
// value expected
['2012-W01', true],
['2012-W52', true],
['2012-01', false],
['W12-2012', false],
['2012-W1', false],
['12-W01', false],
];
}

Expand Down