[PoC] PHPC-2379: Add properties to ReadPreference class by alcaeus · Pull Request #1973 · mongodb/mongo-php-driver

alcaeus · 2026-04-07T06:57:14Z

I revisited the topic of proper properties for value objects based on recent work on object comparisons and the get_properties_for handler. For the time being, I've only done the work for the MongoDB\Driver\ReadPreference class, but the concept applies to all other value objects (see list below).

I've left comments in the code itself to elaborate on some of the changes, but the TL;DR is that we can do away with the static get_properties_hash function (which serves no other purpose than to build a list of properties for the various methods that need them) and some of the other handlers. In fact, we no longer need to implement the __serialize method and the get_debug_info object handler as these dump public properties by default. There are multiple test changes associated with this as our own handlers often would not serialise properties that were "unset" or contained default values.

Here is the list of classes this concept would apply to:

MongoDB\Driver\ReadConcern
MongoDB\Driver\WriteConcern
MongoDB\Driver\WriteError
MongoDB\Driver\WriteResult
MongoDB\Driver\Exception\CommandException
MongoDB\Driver\Exception\WriteException
MongoDB\Driver\Monitoring\CommandFailedEvent
MongoDB\Driver\Monitoring\CommandStartedEvent
MongoDB\Driver\Monitoring\CommandSucceededEvent
MongoDB\Driver\Monitoring\ServerChangedEvent
MongoDB\Driver\Monitoring\ServerClosedEvent
MongoDB\Driver\Monitoring\ServerHeartbeatFailedEvent
MongoDB\Driver\Monitoring\ServerHeartbeatStartedEvent
MongoDB\Driver\Monitoring\ServerHeartbeatSucceededEvent
MongoDB\Driver\Monitoring\ServerOpeningEvent
MongoDB\Driver\Monitoring\TopologyChangedEvent
MongoDB\Driver\Monitoring\TopologyClosedEvent
MongoDB\Driver\Monitoring\TopologyOpeningEvent

These changes would also apply to all BSON classes, although they would not be as useful as the corresponding methods (which should eventually be deprecated) are mandated by the interfaces, which can't contain properties until PHP 8.4. For some of the other classes (e.g. MongoDB\Driver\Server), the value of a property may change over time (e.g. the server description or latency) so I'm not sure whether we would want to use hooked properties for those. I would suggest discussing that separately and splitting PHPC-2379 if we decide to go this route.

alcaeus · 2026-04-07T06:57:40Z

src/MongoDB/ReadPreference.c


 zend_class_entry* phongo_readpreference_ce;

+static const char* phongo_readpreference_get_mode_string(const mongoc_read_prefs_t* read_prefs)


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

alcaeus · 2026-04-07T06:58:47Z

src/MongoDB/ReadPreference.c

 			mongoc_read_prefs_set_tags(intern->read_preference, tags);
 			bson_destroy(tags);
-		} else {
+		} else if (Z_TYPE_P(tagSets) != IS_NULL) {


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

alcaeus · 2026-04-07T06:59:07Z

src/MongoDB/ReadPreference.c

 	}
 }

-static HashTable* phongo_readpreference_get_properties_hash(zend_object* object, bool is_temp)


get_properties_hash becomes entirely useless.

alcaeus · 2026-04-07T07:01:06Z

src/MongoDB/ReadPreference.c

+		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;
+			}


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.

alcaeus · 2026-04-07T07:02:41Z

src/MongoDB/ReadPreference.c

+			Z_TRY_ADDREF_P(val);
+			add_assoc_zval(return_value, ZSTR_VAL(string_key), val);


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.

alcaeus · 2026-04-07T07:03:21Z

src/MongoDB/ReadPreference.c

 	return &intern->std;
 }

-static HashTable* phongo_readpreference_get_debug_info(zend_object* object, int* is_temp)


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

alcaeus · 2026-04-07T07:04:22Z

src/MongoDB/ReadPreference.c

 }

-static HashTable* phongo_readpreference_get_debug_info(zend_object* object, int* is_temp)
+static zval* phongo_readpreference_read_property(zend_object* zobj, zend_string* name, int type, void** cache_slot, zval* rv)


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.

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

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.

alcaeus · 2026-04-07T07:05:19Z

src/phongo_structs.h


 typedef struct {
 	mongoc_read_prefs_t* read_preference;
-	HashTable*           properties;


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.

alcaeus · 2026-04-07T07:05:40Z

tests/manager/bug0851-001.phpt

+  ["maxStalenessSeconds"]=>
+  int(-1)
+  ["hedge"]=>
+  NULL


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

alcaeus · 2026-04-07T07:07:32Z

tests/readPreference/readpreference-compare-001.phpt

@@ -0,0 +1,45 @@
+--TEST--
+MongoDB\Driver\ReadPreference equality comparison


This test was lifted from #1971. I changed the way we print the output for easier debugging and moved the deprecation notices to the top of the output as we run all comparisons before printing output.

Other than that, none of the results needed changing. Most importantly though, since we now defer to the standard object comparator, we can rely on a well-defined order between different ReadPreference instances based on the order and values of their internal properties.

I don't see any tests using the > and < operators, so I assume they aren't supported natively with object properties. The value-equal == operator seems perfectly adequate to me as it is.

They are supported -- I merely wanted to make sure that the tests added in #1971 pass as expected with this change. I would actually change the tests to var_dump($rp1 <=> $rp2); so we can assert against the result.

For context, when PHP compares objects of the same type that don't have a compare handler implemented, the engine goes through all properties of the objects comparing their values until a difference has been found, in which case the -1 or 1 value is returned. This follows PHP's standard semantics for value comparisons.

Using spaceship operator in the test would be more specific; that's a good idea if we are confident PHP default sorting behavior will not change.
That would also prevent us from changing properties declaration order.

GromNaN · 2026-04-07T11:37:36Z

src/MongoDB/ReadPreference.stub.php

+    public readonly string $mode;
+    public readonly array|null $tags;
+    public readonly int $maxStalenessSeconds;
+
+    /** @deprecated */
+    public readonly object|null $hedge;


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.

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.

alcaeus · 2026-04-08T13:26:36Z

Continued in #1979.

alcaeus commented Apr 7, 2026

View reviewed changes

alcaeus force-pushed the poc/phpc-2379-typed-properties branch from 43c95d2 to d7330eb Compare April 7, 2026 07:11

Add properties to ReadPreference class

32861f7

alcaeus force-pushed the poc/phpc-2379-typed-properties branch from d7330eb to 32861f7 Compare April 7, 2026 11:22

GromNaN reviewed Apr 7, 2026

View reviewed changes

GromNaN mentioned this pull request Apr 7, 2026

PHPC-1622: Use get_properties_for handler (prototype: ObjectId only) #1969

Closed

alcaeus mentioned this pull request Apr 8, 2026

PHPC-2699: Add typed, read-only properties for value objects and events #1979

Open

alcaeus closed this Apr 8, 2026

alcaeus deleted the poc/phpc-2379-typed-properties branch April 8, 2026 13:26


		zend_class_entry* phongo_readpreference_ce;

		static const char* phongo_readpreference_get_mode_string(const mongoc_read_prefs_t* read_prefs)

		Z_TRY_ADDREF_P(val);
		add_assoc_zval(return_value, ZSTR_VAL(string_key), val);

		@@ -0,0 +1,45 @@
		--TEST--
		MongoDB\Driver\ReadPreference equality comparison

Conversation

alcaeus commented Apr 7, 2026

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!

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!

alcaeus Apr 7, 2026 • 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!

alcaeus commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alcaeus Apr 7, 2026 •

edited

Loading