PHPC-498: ReadPreference, ReadConcern, and WriteConcern should serialize to BSON #413

derickr · 2016-09-19T15:29:45Z

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

jmikola · 2016-09-19T15:51:17Z

php_phongo.c

 	}
 } /* }}} */

 void php_phongo_write_concern_to_zval(zval *retval, const mongoc_write_concern_t *write_concern) /* {{{ */
 {
 	const char *wtag = mongoc_write_concern_get_wtag(write_concern);
 	const int32_t w = mongoc_write_concern_get_w(write_concern);
+	const int32_t wtimeout = mongoc_write_concern_get_wtimeout(write_concern);


Does this count as mixing code and declarations? I actually would have thought the compiler would yell about the existing *wtag and w lines.

I don't think that's a problem for initializers.... but a good point. But we do this in many other places too:

858 php_phongo_server_description_type_t php_phongo_server_description_type(mongoc_server_description_t *sd) 859 { 860 const char* name = mongoc_server_description_type(sd); 861 int i; 862

872 void php_phongo_server_to_zval(zval *retval, mongoc_server_description_t *sd) /* {{{ */ 873 { 874 mongoc_host_list_t *host = mongoc_server_description_host(sd); 875 const bson_t *is_master = mongoc_server_description_ismaster(sd); 876 bson_iter_t iter; 877

in the same file for example.

Confirming that this doesn't appear to be a problem with my local builds.

I would like to have a proper definitive answer on this somewhere. Can't be arsed to read the spec though :-)

jmikola · 2016-09-19T15:55:23Z

src/MongoDB/ReadConcern.c

+   */
+PHP_METHOD(ReadConcern, bsonSerialize)
+{
+	const mongoc_read_concern_t *read_concern = phongo_read_concern_from_zval(getThis() TSRMLS_CC);


For consistency with our other functions that take no arguments, I'd consider adding this:

if (zend_parse_parameters_none() == FAILURE) { return; }

Also, did we need the following suppressions for PHP 5.x?

SUPPRESS_UNUSED_WARNING(return_value_ptr) SUPPRESS_UNUSED_WARNING(return_value_used)

If these warnings didn't trigger for you, you can always omit this change. I'll add them back if I see them later.

These warnings are IMO superfluous. I have never noticed this being necessary, and I have never seen this in other extensions either.

If you configure phongo with --enable-developer-flags it'll add -Wunused-parameter, which would issue a warning for arguments that aren't used.

The PHP_METHOD() macro defines a c function that has all sorts of arguments, including return_value_ptr and return_value_used which are rarely used, which is why those macros meant to suppress them, so the flag can still be helpful for avoid accidentally declaring superfluous arguments in other functions

It appears that -Wno-unused-parameter was introduced in e3e3c9d#diff-788d457a20b110cc38e571dec9ddc68cR126, builds have been ignoring unused variables since then.

I'm fine with not having warnings for unused function arguments though. Catching locally undefined vars is important though.

jmikola · 2016-09-19T15:55:56Z

src/MongoDB/WriteConcern.c

+   */
+PHP_METHOD(WriteConcern, bsonSerialize)
+{
+	const mongoc_write_concern_t *write_concern = phongo_write_concern_from_zval(getThis() TSRMLS_CC);


Same changes as ReadConcern::bsonSerialize().

jmikola · 2016-09-19T15:56:49Z

src/MongoDB/ReadPreference.c

+   */
+PHP_METHOD(ReadPreference, bsonSerialize)
+{
+	const mongoc_read_prefs_t *read_preference = phongo_read_preference_from_zval(getThis() TSRMLS_CC);


Same changes as ReadConcern::bsonSerialize().

jmikola · 2016-09-19T17:31:17Z

src/MongoDB/ReadConcern.c

+   */
+PHP_METHOD(ReadConcern, bsonSerialize)
+{
+	const mongoc_read_concern_t *read_concern = phongo_read_concern_from_zval(getThis() TSRMLS_CC);


It appears that -Wno-unused-parameter was introduced in e3e3c9d#diff-788d457a20b110cc38e571dec9ddc68cR126, builds have been ignoring unused variables since then.

jmikola · 2016-09-19T17:31:40Z

php_phongo.c

 	}
 } /* }}} */

 void php_phongo_write_concern_to_zval(zval *retval, const mongoc_write_concern_t *write_concern) /* {{{ */
 {
 	const char *wtag = mongoc_write_concern_get_wtag(write_concern);
 	const int32_t w = mongoc_write_concern_get_w(write_concern);
+	const int32_t wtimeout = mongoc_write_concern_get_wtimeout(write_concern);


Confirming that this doesn't appear to be a problem with my local builds.

jmikola · 2016-09-19T17:32:48Z

php_phongo.c

+		case MONGOC_READ_PRIMARY_PREFERRED: ADD_ASSOC_STRING(retval, "mode", "primaryPreferred"); break;
+		case MONGOC_READ_SECONDARY: ADD_ASSOC_STRING(retval, "mode", "secondary"); break;
+		case MONGOC_READ_SECONDARY_PREFERRED: ADD_ASSOC_STRING(retval, "mode", "secondaryPreferred"); break;
+		case MONGOC_READ_NEAREST: ADD_ASSOC_STRING(retval, "mode", "nearest"); break;


add_assoc_string_ex() requires a char * for the value string, so each of these will need (char *) casting. Alternatively, I suppose we can just add that in the macro definition.

jmikola · 2016-09-19T17:33:18Z

tests/writeResult/writeresult-debug-001.phpt

-    int(0)
-    ["journal"]=>
-    NULL
+  array(%d) {


Since you're expecting an empty array here, array(0) would be preferred.

jmikola · 2016-09-19T18:46:45Z

LGTM. I'll add the (char *) casts to ADD_ASSOC_STRING() on my own after this is merged.

…ize to BSON

These test fixes were missed in mongodb#413.

jmikola requested changes Sep 19, 2016

View reviewed changes

derickr force-pushed the PHPC-498-read-preference-concern-serialization branch from bbf4389 to ffa4262 Compare September 19, 2016 16:33

jmikola requested changes Sep 19, 2016

View reviewed changes

jmikola approved these changes Sep 19, 2016

View reviewed changes

PHPC-498: ReadPreference, ReadConcern, and WriteConcern should serial…

8f9e6c8

…ize to BSON

derickr force-pushed the PHPC-498-read-preference-concern-serialization branch from ffa4262 to 8f9e6c8 Compare September 19, 2016 19:33

derickr merged commit 8f9e6c8 into mongodb:master Sep 19, 2016

derickr added a commit that referenced this pull request Sep 19, 2016

Merged pull request #413

af45053

jmikola mentioned this pull request Sep 19, 2016

Casting fixes for strict build options #414

Merged

jmikola added a commit to jmikola/mongo-php-driver that referenced this pull request Sep 19, 2016

PHPC-498: Fix expected output for ReadPreference and WriteConcern

11d823f

These test fixes were missed in mongodb#413.

jmikola mentioned this pull request Sep 19, 2016

PHPC-498: Fix expected output for ReadPreference and WriteConcern #415

Merged

PHPC-498: ReadPreference, ReadConcern, and WriteConcern should serialize to BSON #413

PHPC-498: ReadPreference, ReadConcern, and WriteConcern should serialize to BSON #413

Uh oh!

Conversation

derickr commented Sep 19, 2016 • edited by jmikola Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola Sep 19, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola commented Sep 19, 2016

Uh oh!

Uh oh!

derickr commented Sep 19, 2016 •

edited by jmikola

Loading

jmikola Sep 19, 2016 •

edited

Loading