Skip to content

[PoC] PHPC-2379: Add properties to ReadPreference class #1973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alcaeus wants to merge 1 commit into from
Closed

[PoC] PHPC-2379: Add properties to ReadPreference class #1973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
206 changes: 98 additions & 108 deletions src/MongoDB/ReadPreference.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,71 @@

zend_class_entry* phongo_readpreference_ce;

static const char* phongo_readpreference_get_mode_string(const mongoc_read_prefs_t* read_prefs)
Copy link
Copy Markdown
Member Author

@alcaeus alcaeus Apr 7, 2026

Choose a reason for hiding this comment

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

This function isn't new, I merely had to pull it up as it's used in update_properties.

{
switch (mongoc_read_prefs_get_mode(read_prefs)) {
case MONGOC_READ_PRIMARY:
return PHONGO_READ_PRIMARY;
case MONGOC_READ_PRIMARY_PREFERRED:
return PHONGO_READ_PRIMARY_PREFERRED;
case MONGOC_READ_SECONDARY:
return PHONGO_READ_SECONDARY;
case MONGOC_READ_SECONDARY_PREFERRED:
return PHONGO_READ_SECONDARY_PREFERRED;
case MONGOC_READ_NEAREST:
return PHONGO_READ_NEAREST;
default:
return "";
}
}

static void phongo_readpreference_update_properties(phongo_readpreference_t* intern)
{
const bson_t* bson;

zend_update_property_string(phongo_readpreference_ce, &intern->std, ZEND_STRL("mode"), phongo_readpreference_get_mode_string(intern->read_preference));
zend_update_property_long(phongo_readpreference_ce, &intern->std, ZEND_STRL("maxStalenessSeconds"), mongoc_read_prefs_get_max_staleness_seconds(intern->read_preference));

bson = mongoc_read_prefs_get_tags(intern->read_preference);

if (!bson_empty0(bson)) {
phongo_bson_state state;

PHONGO_BSON_INIT_STATE(state);
state.map.root.type = PHONGO_TYPEMAP_NATIVE_ARRAY;

if (!phongo_bson_to_zval_ex(bson, &state)) {
// Exception already thrown
zval_ptr_dtor(&state.zchild);
return;
}

zend_update_property(phongo_readpreference_ce, &intern->std, ZEND_STRL("tags"), &state.zchild);
zval_ptr_dtor(&state.zchild);
} else {
zend_update_property_null(phongo_readpreference_ce, &intern->std, ZEND_STRL("tags"));
}

bson = mongoc_read_prefs_get_hedge(intern->read_preference);

if (!bson_empty0(bson)) {
phongo_bson_state state;

PHONGO_BSON_INIT_STATE(state);

if (!phongo_bson_to_zval_ex(bson, &state)) {
// Exception already thrown
zval_ptr_dtor(&state.zchild);
return;
}

zend_update_property(phongo_readpreference_ce, &intern->std, ZEND_STRL("hedge"), &state.zchild);
zval_ptr_dtor(&state.zchild);
} else {
zend_update_property_null(phongo_readpreference_ce, &intern->std, ZEND_STRL("hedge"));
}
}

/* Initialize the object from a HashTable and return whether it was successful.
* An exception will be thrown on error. */
static bool phongo_readpreference_init_from_hash(phongo_readpreference_t* intern, HashTable* props)
Expand Down Expand Up @@ -83,7 +148,7 @@ static bool phongo_readpreference_init_from_hash(phongo_readpreference_t* intern

mongoc_read_prefs_set_tags(intern->read_preference, tags);
bson_destroy(tags);
} else {
} else if (Z_TYPE_P(tagSets) != IS_NULL) {
Copy link
Copy Markdown
Member Author

@alcaeus alcaeus Apr 7, 2026

Choose a reason for hiding this comment

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

This is necessary as we now serialise null values for empty tag sets, while previously we would omit the tags element entirely.

phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT, "%s initialization requires \"tags\" field to be array", ZSTR_VAL(phongo_readpreference_ce->name));
goto failure;
}
Expand Down Expand Up @@ -134,7 +199,7 @@ static bool phongo_readpreference_init_from_hash(phongo_readpreference_t* intern

mongoc_read_prefs_set_hedge(intern->read_preference, hedge_doc);
bson_destroy(hedge_doc);
} else {
} else if (Z_TYPE_P(hedge) != IS_NULL) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT, "%s initialization requires \"hedge\" field to be an array or object", ZSTR_VAL(phongo_readpreference_ce->name));
goto failure;
}
Expand All @@ -145,6 +210,8 @@ static bool phongo_readpreference_init_from_hash(phongo_readpreference_t* intern
goto failure;
}

phongo_readpreference_update_properties(intern);

return true;

failure:
Expand All @@ -153,24 +220,6 @@ static bool phongo_readpreference_init_from_hash(phongo_readpreference_t* intern
return false;
}

static const char* phongo_readpreference_get_mode_string(const mongoc_read_prefs_t* read_prefs)
{
switch (mongoc_read_prefs_get_mode(read_prefs)) {
case MONGOC_READ_PRIMARY:
return PHONGO_READ_PRIMARY;
case MONGOC_READ_PRIMARY_PREFERRED:
return PHONGO_READ_PRIMARY_PREFERRED;
case MONGOC_READ_SECONDARY:
return PHONGO_READ_SECONDARY;
case MONGOC_READ_SECONDARY_PREFERRED:
return PHONGO_READ_SECONDARY_PREFERRED;
case MONGOC_READ_NEAREST:
return PHONGO_READ_NEAREST;
default:
return "";
}
}

/* Constructs a new ReadPreference */
static PHP_METHOD(MongoDB_Driver_ReadPreference, __construct)
{
Expand Down Expand Up @@ -281,6 +330,8 @@ static PHP_METHOD(MongoDB_Driver_ReadPreference, __construct)
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT, "Read preference is not valid");
return;
}

phongo_readpreference_update_properties(intern);
}

static PHP_METHOD(MongoDB_Driver_ReadPreference, __set_state)
Expand Down Expand Up @@ -384,89 +435,34 @@ static PHP_METHOD(MongoDB_Driver_ReadPreference, getTagSets)
}
}

static HashTable* phongo_readpreference_get_properties_hash(zend_object* object, bool is_temp)
Copy link
Copy Markdown
Member Author

@alcaeus alcaeus Apr 7, 2026

Choose a reason for hiding this comment

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

get_properties_hash becomes entirely useless.

static PHP_METHOD(MongoDB_Driver_ReadPreference, bsonSerialize)
{
phongo_readpreference_t* intern;
HashTable* props;
const bson_t* tags;
const bson_t* hedge;

intern = Z_OBJ_READPREFERENCE(object);

PHONGO_GET_PROPERTY_HASH_INIT_PROPS(is_temp, intern, props, 4);

if (!intern->read_preference) {
return props;
}
PHONGO_PARSE_PARAMETERS_NONE();

tags = mongoc_read_prefs_get_tags(intern->read_preference);
hedge = mongoc_read_prefs_get_hedge(intern->read_preference);
array_init_size(return_value, 4);

{
zval z_mode;

ZVAL_STRING(&z_mode, phongo_readpreference_get_mode_string(intern->read_preference));
zend_hash_str_update(props, "mode", sizeof("mode") - 1, &z_mode);
}

if (!bson_empty0(tags)) {
phongo_bson_state state;

/* Use PHONGO_TYPEMAP_NATIVE_ARRAY for the root type since tags is an
* array; however, inner documents and arrays can use the default. */
PHONGO_BSON_INIT_STATE(state);
state.map.root.type = PHONGO_TYPEMAP_NATIVE_ARRAY;

if (!phongo_bson_to_zval_ex(tags, &state)) {
zval_ptr_dtor(&state.zchild);
goto done;
}

zend_hash_str_update(props, "tags", sizeof("tags") - 1, &state.zchild);
}

if (mongoc_read_prefs_get_max_staleness_seconds(intern->read_preference) != MONGOC_NO_MAX_STALENESS) {
/* Note: valid values for maxStalesnessSeconds will not exceed the range
* of 32-bit signed integers, so conditional encoding is not necessary. */
long maxStalenessSeconds = mongoc_read_prefs_get_max_staleness_seconds(intern->read_preference);
zval z_max_ss;

ZVAL_LONG(&z_max_ss, maxStalenessSeconds);
zend_hash_str_update(props, "maxStalenessSeconds", sizeof("maxStalenessSeconds") - 1, &z_max_ss);
}

if (!bson_empty0(hedge)) {
phongo_bson_state state;

PHONGO_BSON_INIT_STATE(state);
zend_string* string_key;
zval* val;

ZEND_HASH_FOREACH_STR_KEY_VAL_IND(HASH_OF(getThis()), string_key, val)
{
if (
(Z_TYPE_P(val) == IS_NULL) || (!strcmp(ZSTR_VAL(string_key), "maxStalenessSeconds") && Z_TYPE_P(val) == IS_LONG && Z_LVAL_P(val) == MONGOC_NO_MAX_STALENESS)) {
continue;
}
Comment on lines +448 to +453
Copy link
Copy Markdown
Member Author

@alcaeus alcaeus Apr 7, 2026

Choose a reason for hiding this comment

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

bsonSerialize gets a bit of special logic to avoid adding default values to the output.
HASH_OF(getThis()) gets us the property table, which we can then iterate over to skip null values, as well as the default value for maxStalenessSeconds.

Note the IND suffix on the foreach macro above. Without this, the type of each zval is IS_INDIRECT, while the _IND set of foreach macros gives us access to the actual zval so we can do type checking.


if (!phongo_bson_to_zval_ex(hedge, &state)) {
zval_ptr_dtor(&state.zchild);
goto done;
// Increase the refcount of our zval, as add_assoc_zval takes ownership, leading to the property value being
// freed once the return value goes out of scope
Z_TRY_ADDREF_P(val);
add_assoc_zval(return_value, ZSTR_VAL(string_key), val);
Comment on lines +457 to +458
Copy link
Copy Markdown
Member Author

@alcaeus alcaeus Apr 7, 2026

Choose a reason for hiding this comment

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

add_assoc_zval assumes ownership of the zval. Since we're passing the zval from the internal properties table, we have to use Z_TRY_ADDREF_P to add to the refcount of each refcounted zval to avoid the internal property value being freed once the return value goes out of scope. Finding this took me longer than I care to admit.

}

zend_hash_str_update(props, "hedge", sizeof("hedge") - 1, &state.zchild);
ZEND_HASH_FOREACH_END();
}

done:
return props;
}

static PHP_METHOD(MongoDB_Driver_ReadPreference, bsonSerialize)
{
PHONGO_PARSE_PARAMETERS_NONE();

ZVAL_ARR(return_value, phongo_readpreference_get_properties_hash(Z_OBJ_P(getThis()), true));
convert_to_object(return_value);
}

static PHP_METHOD(MongoDB_Driver_ReadPreference, __serialize)
Copy link
Copy Markdown
Member Author

@alcaeus alcaeus Apr 7, 2026

Choose a reason for hiding this comment

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

No longer need to implement __serialize as this now corresponds to the default behaviour.

{
PHONGO_PARSE_PARAMETERS_NONE();

RETURN_ARR(phongo_readpreference_get_properties_hash(Z_OBJ_P(getThis()), true));
}

static PHP_METHOD(MongoDB_Driver_ReadPreference, __unserialize)
{
zval* data;
Expand All @@ -487,11 +483,6 @@ static void phongo_readpreference_free_object(zend_object* object)

zend_object_std_dtor(&intern->std);

if (intern->properties) {
zend_hash_destroy(intern->properties);
FREE_HASHTABLE(intern->properties);
}

if (intern->read_preference) {
mongoc_read_prefs_destroy(intern->read_preference);
}
Expand All @@ -509,15 +500,13 @@ static zend_object* phongo_readpreference_create_object(zend_class_entry* class_
return &intern->std;
}

static HashTable* phongo_readpreference_get_debug_info(zend_object* object, int* is_temp)
Copy link
Copy Markdown
Member Author

@alcaeus alcaeus Apr 7, 2026

Choose a reason for hiding this comment

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

No longer need to implement a get_debug_info handler as this now corresponds to the default behaviour.

static zval* phongo_readpreference_read_property(zend_object* zobj, zend_string* name, int type, void** cache_slot, zval* rv)
Copy link
Copy Markdown
Member Author

@alcaeus alcaeus Apr 7, 2026

Choose a reason for hiding this comment

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

Implementing the read_property handler is necessary to emit a deprecation notice for the hedge property (which is deprecated like the getHedge method). Since PHP does not support marking properties as deprecated internally (the mechanism only exists for functions/methods), we have to work around this and emit a deprecation notice before deferring to zend_std_read_property.

Copy link
Copy Markdown
Member

@GromNaN GromNaN Apr 7, 2026

Choose a reason for hiding this comment

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

Is there an opportunity for a PHP RFC to enable property deprecation?

Copy link
Copy Markdown
Member Author

@alcaeus alcaeus Apr 7, 2026

Choose a reason for hiding this comment

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

This has been previously discussed, and at least for properties it doesn't seem very straightforward: https://externals.io/message/127305. I would definitely appreciate being able to formally deprecate all kinds of symbols in PHP.

{
*is_temp = 1;
return phongo_readpreference_get_properties_hash(object, true);
}
if (!strcmp(ZSTR_VAL(name), "hedge")) {
php_error_docref(NULL, E_DEPRECATED, "Property MongoDB\\Driver\\ReadPreference::hedge is deprecated");
}

static HashTable* phongo_readpreference_get_properties(zend_object* object)
{
return phongo_readpreference_get_properties_hash(object, false);
return zend_std_read_property(zobj, name, type, cache_slot, rv);
}

void phongo_readpreference_init_ce(INIT_FUNC_ARGS)
Expand All @@ -526,10 +515,9 @@ void phongo_readpreference_init_ce(INIT_FUNC_ARGS)
phongo_readpreference_ce->create_object = phongo_readpreference_create_object;

memcpy(&phongo_handler_readpreference, phongo_get_std_object_handlers(), sizeof(zend_object_handlers));
phongo_handler_readpreference.get_debug_info = phongo_readpreference_get_debug_info;
phongo_handler_readpreference.get_properties = phongo_readpreference_get_properties;
phongo_handler_readpreference.free_obj = phongo_readpreference_free_object;
phongo_handler_readpreference.offset = XtOffsetOf(phongo_readpreference_t, std);
phongo_handler_readpreference.read_property = phongo_readpreference_read_property;
phongo_handler_readpreference.free_obj = phongo_readpreference_free_object;
phongo_handler_readpreference.offset = XtOffsetOf(phongo_readpreference_t, std);
}

void phongo_readpreference_init(zval* return_value, const mongoc_read_prefs_t* read_prefs)
Expand All @@ -540,6 +528,8 @@ void phongo_readpreference_init(zval* return_value, const mongoc_read_prefs_t* r

intern = Z_READPREFERENCE_OBJ_P(return_value);
intern->read_preference = mongoc_read_prefs_copy(read_prefs);

phongo_readpreference_update_properties(intern);
}

const mongoc_read_prefs_t* phongo_read_preference_from_zval(zval* zread_preference)
Expand Down
9 changes: 7 additions & 2 deletions src/MongoDB/ReadPreference.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ final class ReadPreference implements \MongoDB\BSON\Serializable
*/
public const SMALLEST_MAX_STALENESS_SECONDS = UNKNOWN;

public readonly string $mode;
public readonly array|null $tags;
public readonly int $maxStalenessSeconds;

/** @deprecated */
public readonly object|null $hedge;
Comment on lines +54 to +59
Copy link
Copy Markdown
Member

@GromNaN GromNaN Apr 7, 2026

Choose a reason for hiding this comment

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

The reason all these properties weren't declared was because we couldn't make them read-only. But that's been possible since PHP 8.1. That explains why we can do it now, and it seems like a cleaner design.
However, as discussed, PHP does not allow modifications to a read-only property. Instead, we would use private(set) for properties whose values may change over time.

Copy link
Copy Markdown
Member Author

@alcaeus alcaeus Apr 7, 2026

Choose a reason for hiding this comment

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

That's correct -- however, private(set) requires us to bump our version requirement to PHP 8.4, while for value objects readonly achieves the same thing on PHP 8.1 already. Since all these classes are final, I don't see a concern to switching to private(set) once we bump the version requirement.


final public function __construct(string $mode, ?array $tagSets = null, ?array $options = null) {}

/** @deprecated Hedged reads are deprecated in MongoDB 8.0 and will be removed in a future release */
Expand All @@ -67,6 +74,4 @@ final public static function __set_state(array $properties): ReadPreference {}
final public function bsonSerialize(): \stdClass {}

final public function __unserialize(array $data): void {}

final public function __serialize(): array {}
}
30 changes: 25 additions & 5 deletions src/MongoDB/ReadPreference_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion src/phongo_structs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ typedef struct {

typedef struct {
mongoc_read_prefs_t* read_preference;
HashTable* properties;
Copy link
Copy Markdown
Member Author

@alcaeus alcaeus Apr 7, 2026

Choose a reason for hiding this comment

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

This struct member was only needed to store our own properties table assembled by get_properties_hash. The internal properties table itself is stored in zend_object.

zend_object std;
} phongo_readpreference_t;

Expand Down
4 changes: 4 additions & 0 deletions tests/manager/bug0851-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,9 @@ object(MongoDB\Driver\ReadPreference)#%d (%d) {
object(stdClass)#%d (%d) {
}
}
["maxStalenessSeconds"]=>
int(-1)
["hedge"]=>
NULL
Comment on lines +54 to +57
Copy link
Copy Markdown
Member Author

@alcaeus alcaeus Apr 7, 2026

Choose a reason for hiding this comment

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

All test changes account for the extra output generated by var_dump which now always includes all properties.

}
===DONE===
Loading
Loading