DateTime: createFromFormat() returns Nette\Utils\DateTime instance #6

redhead · 2014-04-10T09:40:47Z

Static factory method createFromFormat() now returns Nette\Utils\DateTime instance instead of PHP's \DateTime.

This fixes somewhat unexpected behaviour when calling factory method on a Nette\Utils\DateTime class and receiving an instance of different class.

Closes nette/nette#1258.

Note: I used call_user_func_array() because the third parameter is optional and cannot be NULL for the parent method. I didn't want to use an if condition just for this (but if it is found ugly I will update it).

fprochazka · 2014-04-10T10:01:14Z

src/Utils/DateTime.php

+		if (func_num_args() === 3 && !$timezone instanceof \DateTimeZone) {
+			throw new Nette\InvalidArgumentException('Nette\Utils\DateTime::createFromFormat() expects parameter 3 to be DateTimeZone, ' . gettype($timezone) . ' given');
+		}
+		$datetime = call_user_func_array('parent::createFromFormat', func_get_args());


this cannot be done without call_user_func_array ?

It can, see the note in the PR description, I will change it to if then, I suppose.

I see, it doesn't work when you pass NULL in variable? It has to be called without the third parameter?

Exactly. The 3rd parameter has to be a DateTimeZone instance or it must not be passed at all. (I hate PHP so much in these regards)

The other variant has a condition if ($timezone !== NULL) then call parent method with 3 params, otherwise call with it with 2 (both filling $datetime variable, the last line remains the same).

Which is better?

I guess this one is fine. But it didn't work in some versions of PHP (I guess the old ones). But travis passes, so fuck it :)

Very well.

If you mean the "parent::foo", I searched for similar uses in Nette, and it is used e.g. in Nette\Database, so I figured it shouldn't be a problem.

Just use 3v4l.org if you are not sure which version does what.

milo · 2014-04-10T16:57:54Z

👍 I hit this problem too. I personally prefer the 3rd argument can be NULL so:

$datetime = $zone === NULL
    ? parent::createFromFormat($format, $time)
    : parent::createFromFormat($format, $time, $zone);

return static::from($datetime);

What others?

And I think there can be annotations there it has meening.

redhead · 2014-04-10T17:29:52Z

@milo Me too. It is pretty annoying how PHP handles this, but I didn't want to change the behaviour of the parent method. So, they act the same just the new one returns the correct instance.

What annotations do you mean?

fprochazka · 2014-04-10T17:36:31Z

Just a thought: you could add the timezone created using date_default_timezone_get() if it's null

xificurk · 2014-04-10T17:40:41Z

@fprochazka Or even better: allow passing string as timezone and automatically create the DateTimeZone object.

redhead · 2014-04-10T17:42:10Z

@fprochazka I thought of that too.

@xificurk It changes the parent's default behaviour a lot but I like it.

Majkl578 · 2014-04-10T18:31:43Z

src/Utils/DateTime.php

@@ -86,4 +86,14 @@ public function getTimestamp()
 		return is_float($tmp = $ts * 1) ? $ts : $tmp;
 	}

+
+	public static function createFromFormat($format, $time, $timezone = NULL)


3rd parameter should be typehinted as DateTimeZone, shouldn't it?

It would be nice, but I cannot change the signature - it has to be compatible with the parent method.

redhead · 2014-04-11T12:18:30Z

What about this? I find it pretty good now, including the phpdocs.

xificurk · 2014-04-11T12:54:58Z

src/Utils/DateTime.php

+
+	/**
+	 * Returns new DateTime object formatted according to the specified format.
+	 * @param string The format of the passed in $time parameter should be in


The format ~~of the passed in~~ $time parameter should be in

milo · 2014-04-11T18:33:45Z

What would you expect from these calls?

createFromFormat('Y-m-d H:i:s', '2014-04-11 12:00:00', 'Pacific/Fiji')
createFromFormat('Y-m-d H:i:sP', '2014-04-11 12:00:00+02:00', 'Pacific/Fiji')
createFromFormat('Y-m-d H:i:sP', '2014-04-11 12:00:00+02:00')
createFromFormat('U', '1397240808')
createFromFormat('U', '1397240808', 'Europe/Prague')

redhead · 2014-04-11T18:39:35Z

I think it should do the same as what the docs say about it:
http://www.php.net/manual/en/datetime.createfromformat.php (see $timezone param description and note below)

rostenkowski · 2014-04-12T09:48:19Z

If timezone is omitted and time contains no timezone, the current timezone will be used.

@redhead Good point

redhead · 2014-04-27T11:45:37Z

Is it good to go? (as it is a school assignment I would like it to be merged by the end of the semester.. :-/ if possible )

dg · 2014-04-27T11:56:49Z

I am not sure that changing method signature (3rd parameter) is good idea. In fact, I am suprised that PHP allowes this.

redhead · 2014-04-27T12:04:41Z

There is no type hint, so there is no change in the signature. It is just possible to pass DateTimeZone as well as a string (which is then made into a DateTimeZone instance).

Would you like it to be strictly compatible with the parent method (in behaviour)?

dg · 2014-04-27T13:44:56Z

According to documentation of DateTime, 3rd argument has type hint, so it is change. Or isn't?

milo · 2014-04-27T18:33:14Z

@dg It is somehow wrong PHP implementation/documentation. bug 60302, bug 55407

When you add in descendant

public static function createFromFormat($format, $time, \DateTimeZone $tz = NULL)

PHP Strict Standards:  Declaration of Milo\Dt::createFromFormat() should be compatible
with DateTime::createFromFormat($format, $time, $object = NULL)

And when you call

\DateTime::createFromFormat('Y-m-d', '2014-04-27', NULL);

PHP Warning:  DateTime::createFromFormat() expects parameter 3 to be DateTimeZone, null given

Imho It is buggy as much it can be.

If we extends createFromFormat() we are risking future fixed PHP incompatibiliy. I don't know what to think about it. Maybe add 2nd argument to Nette\Datetime::from($time, $format = NULL)?

redhead · 2014-04-28T09:55:16Z

Yes, it's quite broken. Adding parameter $format to method from() could be a solution, but it's not that nice since you can pass \DateTime or int value in parameter $time where it doesn't make much sense. A special method for formatted date string is better imho.

mishak87 · 2014-04-28T10:50:38Z

So far I feel like these problems can be solved using proxy pattern. (Mask returned objects with proxy and include per PHP version logic that preserves expected behaviour of createformat.)

@milo Nette\Datetime is already not future compatible, definitely not binary compatible and breaking single responsibility rule (it also serves as and unix timestamp enum). This is just mine opinion, no need to discuss it. That bridge will be crossed when PHP gets there (5.7+).

I just hope that any solution won't become like Nette\Callback. (replacing native functionality, and being removed in next version)

dg · 2014-04-28T11:47:43Z

src/Utils/DateTime.php

+			$timezone = new \DateTimeZone($timezone);
+		}
+
+		if (!$timezone instanceof \DateTimeZone) {


Here should be elseif

redhead · 2014-04-28T18:06:05Z

Updated according to David's notes.

dg · 2014-04-28T20:02:02Z

Can you squash it to simple commit?

redhead · 2014-04-28T20:43:52Z

Done.

DateTime: createFromFormat() returns Nette\Utils\DateTime instance

fprochazka reviewed Apr 10, 2014
View reviewed changes

Majkl578 reviewed Apr 10, 2014
View reviewed changes

xificurk reviewed Apr 11, 2014
View reviewed changes

dg reviewed Apr 28, 2014
View reviewed changes

src/Utils/DateTime.php

$timezone = new \DateTimeZone($timezone);

}

if (!$timezone instanceof \DateTimeZone) {

Copy link

Member

dg Apr 28, 2014

Choose a reason for hiding this comment

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

Here should be elseif

DateTime: createFromFormat() returns Nette\Utils\DateTime instance

a7303e8

dg added a commit that referenced this pull request Apr 29, 2014

Merge pull request #6 from redhead/DateTime-fix

3c9cf0d

DateTime: createFromFormat() returns Nette\Utils\DateTime instance

dg merged commit 3c9cf0d into nette:master Apr 29, 2014

driade mentioned this pull request Dec 11, 2020

Exception thrown while running phpstan #242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DateTime: createFromFormat() returns Nette\Utils\DateTime instance #6

DateTime: createFromFormat() returns Nette\Utils\DateTime instance #6

redhead commented Apr 10, 2014

fprochazka Apr 10, 2014

redhead Apr 10, 2014

fprochazka Apr 10, 2014

redhead Apr 10, 2014

fprochazka Apr 10, 2014

redhead Apr 10, 2014

mishak87 Apr 10, 2014

milo commented Apr 10, 2014

redhead commented Apr 10, 2014

fprochazka commented Apr 10, 2014

xificurk commented Apr 10, 2014

redhead commented Apr 10, 2014

Majkl578 Apr 10, 2014

redhead Apr 10, 2014

redhead commented Apr 11, 2014

xificurk Apr 11, 2014

milo commented Apr 11, 2014

redhead commented Apr 11, 2014

rostenkowski commented Apr 12, 2014

redhead commented Apr 27, 2014

dg commented Apr 27, 2014

redhead commented Apr 27, 2014

dg commented Apr 27, 2014

milo commented Apr 27, 2014

redhead commented Apr 28, 2014

mishak87 commented Apr 28, 2014

dg Apr 28, 2014

redhead commented Apr 28, 2014

dg commented Apr 28, 2014

redhead commented Apr 28, 2014

DateTime: createFromFormat() returns Nette\Utils\DateTime instance #6

DateTime: createFromFormat() returns Nette\Utils\DateTime instance #6

Conversation

redhead commented Apr 10, 2014

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

milo commented Apr 10, 2014

redhead commented Apr 10, 2014

fprochazka commented Apr 10, 2014

xificurk commented Apr 10, 2014

redhead commented Apr 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redhead commented Apr 11, 2014

Choose a reason for hiding this comment

milo commented Apr 11, 2014

redhead commented Apr 11, 2014

rostenkowski commented Apr 12, 2014

redhead commented Apr 27, 2014

dg commented Apr 27, 2014

redhead commented Apr 27, 2014

dg commented Apr 27, 2014

milo commented Apr 27, 2014

redhead commented Apr 28, 2014

mishak87 commented Apr 28, 2014

Choose a reason for hiding this comment

redhead commented Apr 28, 2014

dg commented Apr 28, 2014

redhead commented Apr 28, 2014