New DateTimeFormatter Filter (#3617) #3632

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

davidwindell commented Jan 31, 2013

This PR introduces a simple DateTimeFormatter Filter class that will attempt to format a provided string using the specified date() style format.

The main advantage of this is to allow pre-formatting of a provided datetime value on a Zend\Form\Element\DateTime where the HTML5 spec and different browsers may pass values with or without seconds which can cause the validator to fail.

See #3617

@samsonasik samsonasik commented on an outdated diff Jan 31, 2013

library/Zend/Filter/DateTimeNormalize.php
@@ -0,0 +1,90 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2012 Zend Technologies USA Inc. (http://www.zend.com)
@samsonasik

samsonasik Jan 31, 2013

Contributor

2005-2013 :D

@marc-mabe marc-mabe and 1 other commented on an outdated diff Jan 31, 2013

library/Zend/Filter/DateTimeNormalize.php
+ }
+
+ /**
+ * Filter a datetime string by normalizing it to the filters specified format
+ *
+ * @param string $value
+ * @return string
+ */
+ public function filter($value)
+ {
+ try {
+ $result = $this->normalizeDateTime($value);
+ } catch (Exception $ex) {
+ // DateTime threw an exception, an invalid date string was provided
+ return $value;
+ }
@marc-mabe

marc-mabe Jan 31, 2013

Member

Don't catch the exception!

@davidwindell

davidwindell Jan 31, 2013

Contributor

I didn't think Filters were supposed to throw exceptions? Most of the current filters seem to just return the original value if there's a blocking error.

@marc-mabe marc-mabe commented on an outdated diff Jan 31, 2013

library/Zend/Filter/DateTimeNormalize.php
+
+ return $result;
+ }
+
+ /**
+ * Attempt to convert a string into a valid DateTime object
+ *
+ * @param string $value
+ * @returns DateTime
+ * @throws Exception
+ */
+ protected function normalizeDateTime($value)
+ {
+ if (is_int($value)) {
+ $dateTime = new DateTime();
+ $dateTime = $dateTime->setTimestamp($value);
@marc-mabe

marc-mabe Jan 31, 2013

Member

new DateTime('@' . $value);

@marc-mabe marc-mabe commented on an outdated diff Jan 31, 2013

library/Zend/Filter/DateTimeNormalize.php
+ return $result;
+ }
+
+ /**
+ * Attempt to convert a string into a valid DateTime object
+ *
+ * @param string $value
+ * @returns DateTime
+ * @throws Exception
+ */
+ protected function normalizeDateTime($value)
+ {
+ if (is_int($value)) {
+ $dateTime = new DateTime();
+ $dateTime = $dateTime->setTimestamp($value);
+ } else {
@marc-mabe

marc-mabe Jan 31, 2013

Member

Missing $value instanceof DateTime

@marc-mabe marc-mabe commented on an outdated diff Jan 31, 2013

library/Zend/Filter/DateTimeNormalize.php
+ * Attempt to convert a string into a valid DateTime object
+ *
+ * @param string $value
+ * @returns DateTime
+ * @throws Exception
+ */
+ protected function normalizeDateTime($value)
+ {
+ if (is_int($value)) {
+ $dateTime = new DateTime();
+ $dateTime = $dateTime->setTimestamp($value);
+ } else {
+ $dateTime = new DateTime($value);
+ }
+
+ if ($dateTime) {
@marc-mabe

marc-mabe Jan 31, 2013

Member

An empty $dateTime is impossible

@marc-mabe marc-mabe and 1 other commented on an outdated diff Jan 31, 2013

library/Zend/Form/Element/DateTime.php
@@ -20,8 +20,6 @@
class DateTime extends Element implements InputProviderInterface
{
- const DATETIME_FORMAT = 'Y-m-d\TH:iP';
@marc-mabe

marc-mabe Jan 31, 2013

Member

BC break

@davidwindell

davidwindell Jan 31, 2013

Contributor

This is not a BC break, the filter means that you can pass any date format to the Element without an error.

@marc-mabe

marc-mabe Jan 31, 2013

Member

Constants are public and could be used in user world ;)

@davidwindell

davidwindell Jan 31, 2013

Contributor

Fair point

Member

marc-mabe commented Jan 31, 2013

Changing the default format for Zend\Form should be done within a seperate PR and btw. removing the const is a BC break.

In my opinion the name should be DateTimeFormatter

Member

Maks3w commented Jan 31, 2013

Where are the tests? ;)

@Maks3w Maks3w commented on an outdated diff Jan 31, 2013

library/Zend/Form/Element/DateTime.php
*
* @var string
*/
- protected $format = self::DATETIME_FORMAT;
+ protected $format = PhpDateTime::W3C;
@Maks3w

Maks3w Jan 31, 2013

Member

This is a BC Break

@Maks3w Maks3w commented on an outdated diff Jan 31, 2013

library/Zend/Filter/DateTimeFormatter.php
+use Zend\Filter\AbstractFilter;
+
+class DateTimeFormatter extends AbstractFilter
+{
+ /**
+ * A valid format string accepted by date()
+ *
+ * @var string
+ */
+ protected $format = DateTime::ISO8601;
+
+ /**
+ * Sets filter options
+ *
+ * @param string|array|\Zend\Config\Config $options
+ * @return void
@Maks3w

Maks3w Jan 31, 2013

Member

please remove this

@Maks3w Maks3w commented on an outdated diff Jan 31, 2013

library/Zend/Filter/DateTimeFormatter.php
+ *
+ * @param string|array|\Zend\Config\Config $options
+ * @return void
+ */
+ public function __construct($options = null)
+ {
+ if ($options) {
+ $this->setOptions($options);
+ }
+ }
+
+ /**
+ * Set the format string accepted by date() to use when normalizing a string
+ *
+ * @param string $format
+ * @return \Zend\Filter\DateTimeNormalize
@Maks3w

Maks3w Jan 31, 2013

Member

Fix return class

@Maks3w Maks3w commented on an outdated diff Jan 31, 2013

library/Zend/Filter/DateTimeFormatter.php
@@ -0,0 +1,85 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace Zend\Filter;
+
+use DateTime;
+use Exception;
+use Zend\Filter\AbstractFilter;
@Maks3w

Maks3w Jan 31, 2013

Member

This import is unnecesary since both classes are in the same namespace

Contributor

davidwindell commented Feb 1, 2013

@Maks3w tests attached

This tags are not longer necessary (category, package, subpackage)

@Maks3w Maks3w commented on an outdated diff Feb 1, 2013

library/Zend/Filter/DateTimeFormatter.php
+use DateTime;
+use Exception;
+
+class DateTimeFormatter extends AbstractFilter
+{
+ /**
+ * A valid format string accepted by date()
+ *
+ * @var string
+ */
+ protected $format = DateTime::ISO8601;
+
+ /**
+ * Sets filter options
+ *
+ * @param string|array|\Zend\Config\Config $options
@Maks3w

Maks3w Feb 1, 2013

Member

* @param array|Traversable $options

Member

Maks3w commented Feb 1, 2013

@weierophinney What do you thing about this?

@Freeaqingme Freeaqingme and 2 others commented on an outdated diff Mar 6, 2013

library/Zend/Filter/DateTimeFormatter.php
@@ -0,0 +1,83 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace Zend\Filter;
+
+use DateTime;
+use Exception;
@Freeaqingme

Freeaqingme Mar 6, 2013

Member

Can you please use an exception that is part of the Zend\Filter component? If no suitable exception exists you could create it, and have it extend the appropriate SPL exception.

@weierophinney

weierophinney Mar 11, 2013

Owner

I second the recommendation from @Freeaqingme

@davidwindell

davidwindell Mar 11, 2013

Contributor

Hi both, I am "using" \Exception to catch the Exception thrown by DateTime (http://www.php.net/manual/en/datetime.construct.php) which is just \Exception. Are you saying I should create an Exception in Filter\Exception that Extends \Exception?

@weierophinney

weierophinney Mar 11, 2013

Owner

In this case, I'd remove the import, and use FQCN when catching:

catch (\Exception $e)

This will make it more clear that you actually intend to catch a global exception -- which is a rare occurrence in the framework.

@Freeaqingme Freeaqingme and 1 other commented on an outdated diff Mar 6, 2013

tests/ZendTest/Filter/DateTimeFormatterTest.php
+namespace ZendTest\Filter;
+
+use DateTime;
+use Zend\Filter\DateTimeFormatter;
+
+/**
+ * @category Zend
+ * @package Zend_Filter
+ * @subpackage UnitTests
+ * @group Zend_Filter
+ */
+class DateTimeFormatterTest extends \PHPUnit_Framework_TestCase
+{
+ public function setUp()
+ {
+ date_default_timezone_set('UTC');
@Freeaqingme

Freeaqingme Mar 6, 2013

Member

Could we also have a few tests with other timezones? Just to ensure the code isn't fixated on UTC.

@weierophinney

weierophinney Mar 11, 2013

Owner

Also, this is potentially destructive if the timezone is already set, and other tests rely on it. I'd recommend storing the current setting, and during tearDown(), resetting to that value.

Owner

weierophinney commented Mar 11, 2013

This looks good. @davidwindell - Make the changes requested, remove the "[WIP]" notation, and I'll do final review for merge.

Contributor

davidwindell commented Mar 13, 2013

Thanks @weierophinney, I've made the requested changes.

@marc-mabe marc-mabe and 1 other commented on an outdated diff Mar 13, 2013

library/Zend/Filter/DateTimeFormatter.php
+ return $this;
+ }
+
+ /**
+ * Filter a datetime string by normalizing it to the filters specified format
+ *
+ * @param string $value
+ * @return string
+ */
+ public function filter($value)
+ {
+ try {
+ $result = $this->normalizeDateTime($value);
+ } catch (\Exception $ex) {
+ // DateTime threw an exception, an invalid date string was provided
+ return $value;
@marc-mabe

marc-mabe Mar 13, 2013

Member

The filter should throw the exception else it returns not normalized dates

$filter->filter('now'); // 2013-03-13 10:00
$filter->filter('2013-03-13 10:00'); // 2013-03-13 10:00
$filter->filter('abcdefg'); // abcdefg ? -> please throw an Exception
@davidwindell

davidwindell Mar 13, 2013

Contributor

@marc-mabe this is not the behaviour of other filters, see https://github.com/zendframework/zf2/blob/master/library/Zend/Filter/UriNormalize.php#L105

Filters should not throw exceptions or they will cause problems with InputFilters

@marc-mabe

marc-mabe Mar 13, 2013

Member

Thats interesting because other filters throw exceptions - see https://github.com/zendframework/zf2/blob/master/library/Zend/Filter/HtmlEntities.php

@weierophinney What's the right way to go here? For me a filter should throw exceptions on failures else it's impossible to detect such failures (see my example above). Only implementations of Zend\Validator\ValidatorInterface::isValid shouldn't throw exceptions but the filters should do that.

@davidwindell

davidwindell Mar 20, 2013

Contributor

I'm now re throwing the blanket DateTime \Exception as a InvalidArgumentException

@marc-mabe marc-mabe and 1 other commented on an outdated diff Mar 13, 2013

library/Zend/Filter/DateTimeFormatter.php
+ return $value;
+ }
+
+ return $result;
+ }
+
+ /**
+ * Attempt to convert a string into a valid DateTime object
+ *
+ * @param string $value
+ * @returns DateTime
+ * @throws Exception
+ */
+ protected function normalizeDateTime($value)
+ {
+ if (is_int($value)) {
@marc-mabe

marc-mabe Mar 13, 2013

Member

I'm missing the case of $value instanceof DateTime

@davidwindell

davidwindell Mar 20, 2013

Contributor

Done.

@marc-mabe marc-mabe commented on the diff Mar 21, 2013

library/Zend/Filter/DateTimeFormatter.php
+ }
+
+ /**
+ * Filter a datetime string by normalizing it to the filters specified format
+ *
+ * @param string $value
+ * @throws Exception\InvalidArgumentException
+ * @return string
+ */
+ public function filter($value)
+ {
+ try {
+ $result = $this->normalizeDateTime($value);
+ } catch (\Exception $ex) {
+ // DateTime threw an exception, an invalid date string was provided
+ throw new Exception\InvalidArgumentException($ex);
@marc-mabe

marc-mabe Mar 21, 2013

Member

This should be throw new Exception\InvalidArgumentException("Invalid input date '{$value}'", 0, $ex); to throw the exception of DateTime as previous exception

@weierophinney weierophinney added a commit that referenced this pull request Mar 25, 2013

@weierophinney weierophinney Merge pull request #3632 from davidwindell/zf3617
New DateTimeFormatter Filter (#3617)
cdfd752

@weierophinney weierophinney added a commit that referenced this pull request Mar 25, 2013

@weierophinney weierophinney [#3632] throw previous exception
- Incorporate feedback from @marc-mabe
- Also: use $e as exception (for consistency)
794ab09

@weierophinney weierophinney added a commit that referenced this pull request Mar 25, 2013

@weierophinney weierophinney Merge branch 'feature/3632' into develop
Close #3632
096bbb3
Owner

weierophinney commented Mar 25, 2013

Merged.

davidwindell deleted the davidwindell:zf3617 branch Mar 25, 2013

@weierophinney weierophinney added a commit that referenced this pull request May 23, 2013

@weierophinney weierophinney Merge pull request #4499 from localheinz/hotfix/filter-plugin-manager…
…-unaware-of-datetime-formatter

Add service definition for DateTimeFormatter (related to #3632)
f624728

@weierophinney weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge pull request zendframework/zendframework#3632 from davidwindell…
…/zf3617

New DateTimeFormatter Filter (zendframework/zendframework#3617)
0f73b43

@weierophinney weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

@weierophinney weierophinney [zendframework/zendframework#3632] throw previous exception
- Incorporate feedback from @marc-mabe
- Also: use $e as exception (for consistency)
ecf40ea

@weierophinney weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'feature/3632' into develop 0e07b58

@weierophinney weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge pull request zendframework/zendframework#4499 from localheinz/h…
…otfix/filter-plugin-manager-unaware-of-datetime-formatter

Add service definition for DateTimeFormatter (related to zendframework/zendframework#3632)
fa4baf3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment