Remove extra structs for objects with properties#1992
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
alcaeus
force-pushed
the
phpc-2699-remove-extra-structs
branch
from
d9948b0 to
3e2cfd8
Compare
alcaeus
requested review from
Copilot and
paulinevos
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR refactors several “value object” classes in the PHP extension to stop duplicating state in both custom C structs and PHP object properties, relying solely on properties for getters and dropping now-unnecessary structs/handlers.
Changes:
- Introduces
PHONGO_PROPERTY_GETTER(and related) macros to standardize boilerplate getters that return stored properties. - Removes a number of per-class internal structs and custom object handlers for objects that only expose static/stored information.
- Updates WriteError/WriteConcernError and multiple APM Monitoring event classes to initialize readonly properties directly and have getters read from those properties.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/phongo_structs.h | Removes internal C structs for several value-object style classes/events. |
| src/phongo_classes.h | Adds property-getter/object-init macros; adjusts class declarations for removed structs. |
| src/MongoDB/WriteError.c | Drops custom struct/handlers; stores values in properties and uses property-based getters. |
| src/MongoDB/WriteConcernError.c | Drops custom struct/handlers; stores values in properties and uses property-based getters. |
| src/MongoDB/Monitoring/TopologyOpeningEvent.c | Uses property-based getter and initializes properties directly. |
| src/MongoDB/Monitoring/TopologyClosedEvent.c | Uses property-based getter and initializes properties directly. |
| src/MongoDB/Monitoring/ServerOpeningEvent.c | Uses property-based getters and initializes properties directly. |
| src/MongoDB/Monitoring/ServerHeartbeatSucceededEvent.c | Uses property-based getters and initializes properties directly (including reply conversion). |
| src/MongoDB/Monitoring/ServerHeartbeatStartedEvent.c | Uses property-based getters and initializes properties directly. |
| src/MongoDB/Monitoring/ServerHeartbeatFailedEvent.c | Uses property-based getters and initializes properties directly (including error object). |
| src/MongoDB/Monitoring/ServerClosedEvent.c | Uses property-based getters and initializes properties directly. |
| src/MongoDB/Monitoring/CommandSucceededEvent.c | Uses property-based getters and initializes properties directly. |
| src/MongoDB/Monitoring/CommandStartedEvent.c | Uses property-based getters and initializes properties directly. |
| src/MongoDB/Monitoring/CommandFailedEvent.c | Uses property-based getters and initializes properties directly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* TODO: Use MONGOC_NO_SERVER_CONNECTION_ID once it is added to libmongoc's public API (CDRIVER-4176) */ | ||
| if (intern->server_connection_id == -1) { | ||
| zend_update_property_null(phongo_commandfailedevent_ce, &intern->std, ZEND_STRL("serverConnectionId")); | ||
| if (mongoc_apm_command_failed_get_server_connection_id_int64(event) == -1) { | ||
| zend_update_property_null(phongo_commandfailedevent_ce, object, ZEND_STRL("serverConnectionId")); | ||
| } else { | ||
| zend_update_property_long(phongo_commandfailedevent_ce, &intern->std, ZEND_STRL("serverConnectionId"), intern->server_connection_id); | ||
| zend_update_property_long(phongo_commandfailedevent_ce, object, ZEND_STRL("serverConnectionId"), mongoc_apm_command_failed_get_server_connection_id_int64(event)); | ||
| } |
There was a problem hiding this comment.
On 32-bit builds, serverConnectionId is an int64 from libmongoc but is stored into a zend_long here without any range check. The previous getter warned when truncating a 64-bit value; after this change, large values can be silently truncated when assigning the "serverConnectionId" property. Consider restoring the SIZEOF_ZEND_LONG == 4 warning/handling before calling zend_update_property_long.
| /* TODO: Use MONGOC_NO_SERVER_CONNECTION_ID once it is added to libmongoc's public API (CDRIVER-4176) */ | ||
| if (intern->server_connection_id == -1) { | ||
| zend_update_property_null(phongo_commandsucceededevent_ce, &intern->std, ZEND_STRL("serverConnectionId")); | ||
| if (mongoc_apm_command_succeeded_get_server_connection_id_int64(event) == -1) { | ||
| zend_update_property_null(phongo_commandsucceededevent_ce, object, ZEND_STRL("serverConnectionId")); | ||
| } else { | ||
| zend_update_property_long(phongo_commandsucceededevent_ce, &intern->std, ZEND_STRL("serverConnectionId"), intern->server_connection_id); | ||
| zend_update_property_long(phongo_commandsucceededevent_ce, object, ZEND_STRL("serverConnectionId"), mongoc_apm_command_succeeded_get_server_connection_id_int64(event)); | ||
| } |
There was a problem hiding this comment.
The previous implementation warned on 32-bit builds when a 64-bit serverConnectionId would be truncated. This refactor stores the int64 directly into a zend_long without any SIZEOF_ZEND_LONG == 4 range check, which can silently truncate large values. Consider restoring the warning (and/or clamping/using string) before updating the "serverConnectionId" property when sizeof(zend_long) == 4.
| /* TODO: Use MONGOC_NO_SERVER_CONNECTION_ID once it is added to libmongoc's public API (CDRIVER-4176) */ | ||
| if (intern->server_connection_id == -1) { | ||
| zend_update_property_null(phongo_commandstartedevent_ce, &intern->std, ZEND_STRL("serverConnectionId")); | ||
| if (mongoc_apm_command_started_get_server_connection_id_int64(event) == -1) { | ||
| zend_update_property_null(phongo_commandstartedevent_ce, object, ZEND_STRL("serverConnectionId")); | ||
| } else { | ||
| zend_update_property_long(phongo_commandstartedevent_ce, &intern->std, ZEND_STRL("serverConnectionId"), intern->server_connection_id); | ||
| zend_update_property_long(phongo_commandstartedevent_ce, object, ZEND_STRL("serverConnectionId"), mongoc_apm_command_started_get_server_connection_id_int64(event)); | ||
| } |
There was a problem hiding this comment.
The old getServerConnectionId() implementation emitted a warning on 32-bit builds when a 64-bit serverConnectionId would be truncated. This code now writes the int64 directly into the "serverConnectionId" property via zend_update_property_long without a SIZEOF_ZEND_LONG == 4 bounds check, which can silently truncate. Suggest reinstating the warning (or another non-truncating representation) before storing the value.
|
|
||
| PHONGO_RETURN_PROPERTY(commandfailedevent, "serverConnectionId"); | ||
| } | ||
| PHONGO_PROPERTY_GETTER(MongoDB_Driver_Monitoring_CommandFailedEvent, getCommandName, commandfailedevent, "commandName") |
paulinevos
left a comment
paulinevos
left a comment
There was a problem hiding this comment.
LGTM, but is the thing Copilot mentioned worth looking into, also in terms of test coverage?
I didn't want to include it, but common sense prevailed and I added the warnings back in for 32-bit platforms. We should really get rid of that at some point in time (or lobby for PHP's |
With many classes storing internal properties in
zend_object, we're now storing data twice in multiple classes: once in thestructfields, and once in the property. Thestructfields are only used for the getters, which strictly speaking isn't necessary. To be clear, we were doing this before already onceget_properties_hashwas invoked and we populated our ownpropertiestable.This PR does away with that duplicate data handling for everything that returns static information (i.e. where we don't need to keep a reference to an underlying libmongoc structure):
PHONGO_PROPERTY_GETTERmacro is used to declare the boilerplate for getters, consisting of parameter parsing and reading the property value into thereturn_valuezvalNot only does this remove duplicate data handling, but it greatly simplifies those value object classes by doing away with class handlers, allocations, etc.