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

DateTimeFormatterStrategy passes special characters used by date_create_from_format to date_format #69

Merged
merged 3 commits into from
Nov 19, 2018
Merged

Conversation

boesing
Copy link
Member

@boesing boesing commented Feb 5, 2018

Hey guys,

I would like to use special chars for the date format, e.g. to ensure that if I pass a MySQL date string to the hydrator, an object with time reset to 0 is being returned.

Therefore, I had to strip un-escaped special chars (*, !, | and +) (documentation) from the format being passed to the DateTimeInterface::format method.

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

I guess I just don't quite understand the purpose of this. Why would the format you provide to the DateTimeFormatterStrategy contain these characters?

Additionally, this feels very much like a project-specific requirement which could be handled by either:

  • Decoration (likely by decorating the constructor to filter the format before assignment)
  • Domain-level code that triggers before creating the strategy instance.

I need more background to assess whether this is something we want to support.

@@ -48,7 +48,8 @@ public function __construct($format = DateTime::RFC3339, DateTimeZone $timezone
public function extract($value)
{
if ($value instanceof DateTime) {
return $value->format($this->format);
$extractionFormat = preg_replace('/(?<![\\\\])[+|!\*]/', '', $this->format);
Copy link
Member

Choose a reason for hiding this comment

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

For those reviewing the issue later to understand what it does, the above matches an of the characters +|!* NOT preceded by a \\.

@@ -48,7 +48,8 @@ public function __construct($format = DateTime::RFC3339, DateTimeZone $timezone
public function extract($value)
{
if ($value instanceof DateTime) {
Copy link
Member

Choose a reason for hiding this comment

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

Why we don't accept DateTimeInterface @weierophinney?

Copy link
Member

Choose a reason for hiding this comment

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

@malukenho We do as of #70. ;-)

@boesing
Copy link
Member Author

boesing commented May 1, 2018

@weierophinney Well, the thing is, if you use the Strategies to e.g. transport data via JSON by an API, the clients hydrate that value back into a datetime object and always uses the the current time if neither "!" at the beginning or "|" at the end is specified. This actually is pretty annoying if you want to use a date for comparison reasons.

Lets assume the following example with the current time (10:14 pm):

$data = [
    'birthdate' => (new DateTime('-18 years'))->format('Y-m-d'),
];

$user = new class
{

    /**
     * @var DateTime
     */
    public $birthdate;

    public function isEighteen(): bool
    {
        $today = new DateTime();
        $diff = $this->birthdate->diff($today, true);

        return $diff->y >= 18;
    }

    public function wasEighteen(DateTime $specificDate): bool
    {
        $diff = $this->birthdate->diff($specificDate, true);

        return $diff->y >= 18;
    }
};

$hydrator = new Zend\Hydrator\Reflection;
$hydrator->addStrategy('birthdate', new Zend\Hydrator\Strategy\DateTimeFormatterStrategy('Y-m-d'));
$hydrator->hydrate($data, $user);

var_dump($user->isEighteen());
var_dump($user->wasEighteen(new DateTime('-5 minutes')));
bool(true)
bool(false)

Ofcourse, the user was already 18 even 5 minutes ago.
If I would use DateTimeFormatterStrategy('Y-m-d|'), the strategy would hydrate the birthdate without the current time set to that object. Yes, I could change the business logic and force that user::birthdate->setTime(0,0) by myself, but I actually dont see any disadvantages of parsing the format during extraction.

@boesing
Copy link
Member Author

boesing commented Oct 24, 2018

So, is there any possibility that this change is being merged? If not, please feel free to either close this PR or give me a short feedback.
I am still struggling with that problem and probably need to replace all zend-hydrator DateTimeFormatterStrategy usages within our code if this aint going forward.

Thanks in advance.

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, @boesing !

The only change I'd ask for now is a note in the documentation detailing that you can do this when you need to be concerned about comparisons.

@weierophinney weierophinney merged commit b59d7d5 into zendframework:master Nov 19, 2018
weierophinney added a commit that referenced this pull request Nov 19, 2018
DateTimeFormatterStrategy passes special characters used by `date_create_from_format` to `date_format`
weierophinney added a commit that referenced this pull request Nov 19, 2018
weierophinney added a commit that referenced this pull request Nov 19, 2018
weierophinney added a commit that referenced this pull request Nov 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants