PHPC-536: UTCDateTime construction from DateTime or current time #336

jmikola · 2016-06-21T20:01:57Z

https://jira.mongodb.org/browse/PHPC-536

moderndeveloperllc · 2016-06-21T20:15:37Z

Sorry for my programming ignorance, but I noticed that you were using DateTime in the code and tests. Is this code DateTimeInterface-compatible? It may also be advisable to throw in a pre-1970 date in the tests just to make sure that negative numbers are playing nicely. I'm looking forward to this landing soon!

derickr · 2016-06-21T21:04:17Z

src/BSON/UTCDateTime.c

+			/* Initialize UTCDateTime from current time */
+			struct timeval cur_time;
+
+			gettimeofday(&cur_time, NULL);


Windows doesn't have gettimeofday.

I considered the options in http://stackoverflow.com/q/10905892/162228, but then I found gettimeofday and the necessary structs defined in win32/time.h (going back to at least PHP 5.4). I also found evidence of that implementation being used in several core extensions.

The PHP implementation seems to be equivalent to the other solutions (i.e. it uses GetSystemTimeAsFileTime()).

jmikola · 2016-06-21T22:41:01Z

Sorry for my programming ignorance, but I noticed that you were using DateTime in the code and tests. Is this code DateTimeInterface-compatible?

DateTimeInterface only exists in PHP 5.5, but we should be able to use that if it's available and fall back to DateTime for PHP 5.4.

@derick: I noticed that php_date.h only exports class entries for DateTime and DateTimeImmutable:

PHPAPI zend_class_entry *php_date_get_date_ce(void);
PHPAPI zend_class_entry *php_date_get_immutable_ce(void);

Can we get away with defining extern zend_class_entry * date_ce_interface and relying on variable in php_date.c (it's not declared static). Alternatively, do we need to use zend_fetch_class() and resolve the interface name string to a class entry?

It may also be advisable to throw in a pre-1970 date in the tests just to make sure that negative numbers are playing nicely. I'm looking forward to this landing soon!

Great idea. I'll work on getting these in and try to come up with some tests that exercise the range limits of PHP and BSON dates.

jmikola · 2016-06-21T22:43:46Z

@derickr: Regarding the Travis failure, that appears to be bson-utcdatetime_error-001.phpt, which was asserting that UTCDateTime::__construct() threw when no argument was passed. Since that test case is no longer valid, I propose we delete it and decrement the other "-002" and "-003" tests.

moderndeveloperllc · 2016-06-22T05:22:40Z

DateTimeInterface only exists in PHP 5.5, but we should be able to use that if it's available and fall back to DateTime for PHP 5.4.

Sorry. That's just me forgetting that the driver is compatible back to 5.4. Being able to pass in DateTimeInterface classes would be great regardless.

derickr · 2016-06-27T14:05:50Z

Can we get away with defining extern zend_class_entry * date_ce_interface and relying on variable in php_date.c (it's not declared static).

No. This works on Linux, but won't work on Windows.

Alternatively, do we need to use zend_fetch_class() and resolve the interface name string to a class entry?

That sounds kinda crappy... I'd rather export the interface in PHP (from PHP 7.1).

jmikola · 2016-06-27T14:23:59Z

Alternatively, do we need to use zend_fetch_class() and resolve the interface name string to a class entry?

That sounds kinda crappy... I'd rather export the interface in PHP (from PHP 7.1).

Of course it makes sense to start doing that in PHP 7.1, but zend_fetch_class() seems necessary to get this working today.

jmikola · 2016-06-27T18:40:18Z

src/BSON/UTCDateTime.c

+		found_ce = zend_fetch_class("DateTimeInterface", sizeof("DateTimeInterface")-1, ZEND_FETCH_CLASS_AUTO|ZEND_FETCH_CLASS_NO_AUTOLOAD|ZEND_FETCH_CLASS_SILENT TSRMLS_CC);
+	#endif
+
+	return found_ce ? found_ce : php_date_get_date_ce();


@derickr: Would it make sense to memoize this function by saving found_ce in a static global? I also thought about initializing the constructors date_ce in UTCDateTime's MINIT, but that assumes we are always initialized after PHP's date extension. I'd like to respect the edge case where that may not be true.

You can add a dependency on it, and that should make PHP initialise datetime first. Instead of MINIT, I would put it in RINIT.

jmikola · 2016-06-27T20:01:53Z

src/BSON/UTCDateTime.c

+		zend_restore_error_handling(&error_handling TSRMLS_CC);
+		return;
+	}
+#endif


I ran into problems calling zend_fetch_class() to get the class entry for DateTimeInterface during MINIT. This ultimately seemed an easier solution, which uses nothing more than the date extension's public API.

Meanwhile, I've opened PHPC-718 to ensure that we start listing our extension dependencies.

…time

derickr · 2016-06-29T09:10:45Z

tests/bson/bson-utcdatetime_error-001.phpt

 --SKIPIF--
-<?php if (defined("HHVM_VERSION_ID")) exit("skip HHVM handles parameter parsing differently"); ?>


We might have to put this one back in I suppose, if we find that it's still different with HHVM.

I don't think it will be a problem. Your constructor accepts a mixed type and all validation is left to you, so it should be trivial to customize the exception message here.

derickr · 2016-06-29T09:10:54Z

LGTM

Supported since ext-mongodb 1.2 mongodb/mongo-php-driver#336

derickr reviewed Jun 21, 2016
View reviewed changes

Cast return value in Z_PHPDATE_P compatibility macro

2bd3dc0

jmikola force-pushed the phpc-536 branch from d07e0ed to caa7bee Compare June 27, 2016 18:37

jmikola reviewed Jun 27, 2016
View reviewed changes

jmikola force-pushed the phpc-536 branch from caa7bee to 34363c0 Compare June 27, 2016 19:58

jmikola reviewed Jun 27, 2016
View reviewed changes

jmikola mentioned this pull request Jun 28, 2016

Export php_date_get_interface_ce() for extension use php/php-src#1963

Closed

jmikola added 2 commits June 28, 2016 15:24

Wrap argument in phongo_str() compatibility macro

a8473b0

PHPC-536: UTCDateTime construction from DateTimeInterface or current …

29a3729

…time

jmikola force-pushed the phpc-536 branch from 34363c0 to 29a3729 Compare June 28, 2016 19:25

derickr reviewed Jun 29, 2016
View reviewed changes

jmikola merged commit 29a3729 into mongodb:master Jun 29, 2016

jmikola added a commit that referenced this pull request Jun 29, 2016

Merge pull request #336

df07ec5

jmikola deleted the phpc-536 branch June 29, 2016 18:25

moderndeveloperllc mentioned this pull request Aug 4, 2016

Aggregation Framework isDead true mongodb/mongo-php-library#223

Closed

GromNaN added a commit to GromNaN/mongodb-odm that referenced this pull request Feb 27, 2025

Create UTCDateTime from DateTimeInterface directly

08c7269

Supported since ext-mongodb 1.2 mongodb/mongo-php-driver#336

GromNaN mentioned this pull request Feb 27, 2025

Create UTCDateTime from DateTimeInterface directly doctrine/mongodb-odm#2736

Merged

GromNaN added a commit to doctrine/mongodb-odm that referenced this pull request Feb 28, 2025

Create UTCDateTime from DateTimeInterface directly (#2736)

fd0ad45

Supported since ext-mongodb 1.2 mongodb/mongo-php-driver#336

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

PHPC-536: UTCDateTime construction from DateTime or current time #336

PHPC-536: UTCDateTime construction from DateTime or current time #336

Uh oh!

jmikola commented Jun 21, 2016

Uh oh!

moderndeveloperllc commented Jun 21, 2016

Uh oh!

derickr Jun 21, 2016

Uh oh!

jmikola Jun 21, 2016 •

edited

Loading

Uh oh!

derickr Jun 27, 2016

Uh oh!

jmikola commented Jun 21, 2016

Uh oh!

jmikola commented Jun 21, 2016

Uh oh!

moderndeveloperllc commented Jun 22, 2016

Uh oh!

derickr commented Jun 27, 2016

Uh oh!

jmikola commented Jun 27, 2016 •

edited

Loading

Uh oh!

jmikola Jun 27, 2016

Uh oh!

derickr Jun 27, 2016

Uh oh!

jmikola Jun 27, 2016

Uh oh!

derickr Jun 29, 2016

Uh oh!

jmikola Jun 29, 2016

Uh oh!

derickr commented Jun 29, 2016

Uh oh!

Uh oh!

		--SKIPIF--
		<?php if (defined("HHVM_VERSION_ID")) exit("skip HHVM handles parameter parsing differently"); ?>

PHPC-536: UTCDateTime construction from DateTime or current time #336

PHPC-536: UTCDateTime construction from DateTime or current time #336

Uh oh!

Conversation

jmikola commented Jun 21, 2016

Uh oh!

moderndeveloperllc commented Jun 21, 2016

Uh oh!

derickr Jun 21, 2016

Choose a reason for hiding this comment

Uh oh!

jmikola Jun 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derickr Jun 27, 2016

Choose a reason for hiding this comment

Uh oh!

jmikola commented Jun 21, 2016

Uh oh!

jmikola commented Jun 21, 2016

Uh oh!

moderndeveloperllc commented Jun 22, 2016

Uh oh!

derickr commented Jun 27, 2016

Uh oh!

jmikola commented Jun 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmikola Jun 27, 2016

Choose a reason for hiding this comment

Uh oh!

derickr Jun 27, 2016

Choose a reason for hiding this comment

Uh oh!

jmikola Jun 27, 2016

Choose a reason for hiding this comment

Uh oh!

derickr Jun 29, 2016

Choose a reason for hiding this comment

Uh oh!

jmikola Jun 29, 2016

Choose a reason for hiding this comment

Uh oh!

derickr commented Jun 29, 2016

Uh oh!

Uh oh!

jmikola Jun 21, 2016 •

edited

Loading

jmikola commented Jun 27, 2016 •

edited

Loading