3.0 - Use DateTime in Time utility 'is' and 'was' comparisons

[bcrowe]

No description provided.

[ravage84]

All this code looks very similar. May be you can put the main code into a protected method and parameterize it?

Something like:

 _isSameDate($dateString, $format1, $format2, $timezone = null)

[markstory]

I like @ravage84's suggestion.

[bcrowe]

@markstory @ravage84

Or, alternatively, do we want to scrap all these methods and have a single public method with an options parameter?

Incomplete, but illustrates the idea:

is($relative, $dateString, $timezone) {
    $dateTime = new \DateTime;
    switch($relative) {
        case 'today':
            $format = 'Y-m-d';
            $relativeTo = 'now';
            break;
        case 'yesterday':
            $format = 'Y-m-d';
            $relativeTo = 'yesterday';
    }
    $timestamp = $dateTime->setTimestamp(static::fromString($dateString, $timezone))
        ->format($format);
    $now = $dateTime->setTimestamp(static::fromString($relativeTo, $timezone))
        ->format($format);
    return $timestamp === $now;
}

Edit: Maybe name the method 'compareTo'?

[markstory]

I think the named methods are nice as they make the calling code more expressive and easier to read. If those methods all use a single method internally that is fine.

[bcrowe]

@markstory Sounds good, will update later this evening.

[bcrowe]

Any comments on this?

[dereuromark]

I am usually a fan of keeping object variables uppercase on first letter ($DateTime).

[lorenzo]

I think having it as lowercase is fine for 3.x. We will be using object more extensively and the code will look like a giant collection of uppercase letters if we follow that.

[markstory]

I used to use $Foo as well, but it got really annoying as code started having more objects.

[dereuromark]

So will we drop if for 3.x completely? Or just for php-internal objects etc?

[Phally]

I like lowercase as well.

Also maybe $dateTime = new \DateTime(); instead of $dateTime = new \DateTime; (without the brackets).

[lorenzo]

Refactoring looks good, although I'm concerned that we are making more difficult to spot and fix this ticket which is related to the functions you are changing #2032

[markstory]

The new refactorings look good.

[Phally]

The default value for $timezone isn't really useful here, because the rest of the parameters are mandatory.

[ADmad]

Yeah optional arguments followed by mandatory ones is generally frowned upon.

[dereuromark]

Maybe $timezone (if optional compared to the others) should then be the last one.

[bcrowe]

Moved the "timezone" parameter to be the last parameter. Renamed the method to be semantically correct.

[markstory]

Looks great.

[ravage84]

Still not fully happy with the name _evaluateTimeSpan but that shouldn't be a total show stopper 👍

[Phally]

Yeah I feel (because it returns a boolean) it should be _isWithinTimeSpan(). The rest of the changes look good! 👍

[bcrowe]

@Phally 👍 I like that better too.

[Phally]

Excellent!