PHPC-1491: Add ReadPreference::getModeString #1062
Conversation
src/MongoDB/ReadPreference.c
Outdated
| return; | ||
| } | ||
|
|
||
| PHONGO_RETURN_STRING(php_phongo_readpreference_get_mode_string(mongoc_read_prefs_get_mode(intern->read_preference))); |
There was a problem hiding this comment.
php_phongo_readpreference_get_mode_string returns NULL when encountering an unknown value for read preference. The Server Selection spec specifies that driver MUST support the 5 modes, but doesn't limit the possible modes for read preferences.
While I suppose that PHONGO_RETURN_STRING (or rather zend_string_init) can handle the NULL value, I'm not sure if returning an empty string is the preferred option here - we may rather want to fall back to returning the int value (as string for type safety).
There was a problem hiding this comment.
I would be more concerned about passing NULL to strlen() in the ZVAL_STRING macro:
- http://lxr.jpauli.tech/source/xref/php-5.6/Zend/zend_API.h#ZVAL_STRING
- http://lxr.jpauli.tech/source/xref/php-7.4/Zend/zend_API.h#ZVAL_STRING
I believe strlen(NULL) is undefined in most implementations:
In practice, I don't think it's possible for the read preference to have a different mode unless it was added to the enum and the PHP driver was not updated to handle it. That said, a defensive solution may be to simply throw an exception here if php_phongo_readpreference_get_mode_string returns NULL and include the numeric code in the message.
There was a problem hiding this comment.
I was just reminded of https://github.com/mongodb/mongo-php-driver/pull/1061/files#r345720246. For consistency, perhaps you'd rather throw from php_phongo_readpreference_get_mode_string directly instead of getModeString()?
| @@ -733,6 +755,12 @@ void php_phongo_readpreference_init_ce(INIT_FUNC_ARGS) /* {{{ */ | |||
| zend_declare_class_constant_long(php_phongo_readpreference_ce, ZEND_STRL("RP_NEAREST"), MONGOC_READ_NEAREST TSRMLS_CC); | |||
| zend_declare_class_constant_long(php_phongo_readpreference_ce, ZEND_STRL("NO_MAX_STALENESS"), MONGOC_NO_MAX_STALENESS TSRMLS_CC); | |||
| zend_declare_class_constant_long(php_phongo_readpreference_ce, ZEND_STRL("SMALLEST_MAX_STALENESS_SECONDS"), MONGOC_SMALLEST_MAX_STALENESS_SECONDS TSRMLS_CC); | |||
|
|
|||
| zend_declare_class_constant_string(php_phongo_readpreference_ce, ZEND_STRL("PRIMARY"), PHONGO_READ_PRIMARY TSRMLS_CC); | |||
There was a problem hiding this comment.
As in #1061, I'm exposing constants here. Since the class name is ReadPreference, I decided to avoid using any prefix. This also resolves a naming conflict with the RP_ constants that expose the integer values for read preference modes.
I'm not too happy about having two very similar constants (RP_PRIMARY and PRIMARY) in the same class, as this may confuse users. We don't need to expose these values as constants now and may opt to only do that in 2.0 when the RP_ constants are dropped.
There was a problem hiding this comment.
I'm OK with adding the constants now, as it's consistent with what we're already doing in ReadConcern. If we don't already have tickets to deprecate and remove the RP_ constants, feel free to create those.
There was a problem hiding this comment.
Deprecation is tracked in PHPC-1489. I've added tests for all constants, including int ones for good measure.
|
I'd also suggest one additional test for the new constants, similar to I suggest adding one more test for these constants, similar to https://github.com/mongodb/mongo-php-driver/blob/master/tests/readConcern/readconcern-constants.phpt. Feel free to add the numeric constants if you like, but I'm not too concerned about those. |
|
Oh, my apologies, I totally missed that message. |
https://jira.mongodb.org/browse/PHPC-1491