PHPC-2505: Fix foreach after GC invocation

[GromNaN]

PHPC-2505. Fixes #1776.

Problem

Calling gc_collect_cycles() changed how foreach iterates BSON value objects, as well as MongoDB\Driver\ServerApi, ServerDescription, and TopologyDescription. Before a GC run, an ObjectId yielded its oid property. After a GC run, the loop body never executed, with no error or warning.

Root cause: the extension's shared get_gc handler called zend_std_get_properties(). For these classes, which declare no PHP properties, that materialized an empty zend_object.properties table. The ZEND_FE_RESET_R opcode reads zend_object.properties first and, when it is non-NULL, uses it instead of calling the get_properties handler. The empty table then made foreach skip the loop silently.

Approaches considered

1. Remove the shared get_gc handler and rely on the engine default (the first commit of this PR). The engine default invokes the class get_properties handler when it is overridden, which returns the object's cached debug property table (intern->properties). That table is owned by the object and freed by free_object, so exposing it to the collector is the lifetime hazard that PHPC-1598 fixed. It also hides user assigned dynamic properties from the collector, which broke tests/bson/bug1598-002.phpt on PHP 8.1. Rejected.

2. Add a php_properties HashTable plus per-class read_property, write_property, has_property, unset_property, get_property_ptr_ptr, and get_gc handlers (PR Fix foreach after the garbage collector is invoked #1850, revised as PHPC-2505: Fix foreach after GC invocation #1940). This separates user assigned dynamic properties from the debug cache and also stabilizes the set/unset property case. It was abandoned: it duplicates engine property semantics across 19 classes, had a garbage collector refcount leak on the returned table, and conflicted with the typed readonly properties added by PHPC-2699.

3. Replace get_properties with get_properties_for (PHPC-1622, prototype PHPC-1622: Use get_properties_for handler (prototype: ObjectId only) #1969). It does not fix this bug: for a non Traversable object, ZEND_FE_RESET_R calls get_properties directly, so foreach still reads the materialized table.

4. Add typed readonly properties to the BSON classes and drop the custom property handling (PHPC-2705 and PHPC-2706). This is the long term fix and would give native PHP behavior, but those tickets are still in the backlog and this bug is blocked on them. Out of scope here.

Fix

Keep a get_gc handler, but make it narrow. When the class defines a custom get_properties handler, return object->properties without ever materializing it. Otherwise, delegate to zend_std_get_gc. The property cache (intern->properties) is never exposed to the collector, user assigned dynamic properties are still scanned for cycles, and foreach no longer depends on whether the garbage collector ran.

The change is limited to the get_gc handler in phongo.c and a regression test. Replacing the remaining zend_hash_destroy plus FREE_HASHTABLE teardowns with zend_hash_release is handled separately in PHPC-2682.

ServerApi property cache leak

Adding ServerApi to the foreach regression test exposed a separate, pre-existing bug. phongo_serverapi_get_properties_hash() filled the cached property HashTable with zend_hash_str_add(). From the second call on, the keys already exist, the add fails, and the newly allocated version string is never released. foreach calls get_properties several times per object, so each iteration leaked a string. It now uses zend_hash_str_update(), like every other class's property builder. This is a second commit.

Known limitation

Setting and then unsetting a dynamic property ($oid->foo = 'x'; unset($oid->foo);) still leaves foreach yielding nothing. The native write_property handler creates zend_object.properties, and it keeps shadowing get_properties even once emptied. Fixing that requires removing the custom get_properties handling altogether, which is tracked by PHPC-2705. Dynamic properties are deprecated as of PHP 8.2.

Follow-ups

• PHPC-2705 and PHPC-2706: typed readonly properties for BSON classes and libmongoc backed classes.
• PHPC-2682: unify the remaining zend_hash_destroy plus FREE_HASHTABLE teardowns on zend_hash_release.

[Copilot]

Pull request overview

Fixes a GC/foreach inconsistency affecting BSON objects by preventing the engine’s standard property table/GC behavior from interfering with the driver’s cached BSON property HashTables.

Changes:

• Adds per-class property/GC handlers that store dynamic PHP properties separately from BSON “properties” caches.
• Removes the shared get_gc override from phongo.c and updates HashTable teardown to use zend_hash_release.
• Adds regression tests covering gc_collect_cycles() and property set/unset (incl. by-reference) scenarios.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
phongo.c	Removes the shared std get_gc override.
phongo.h	Introduces shared macros to define/assign per-class property + get_gc handlers; updates temp HashTable freeing.
src/phongo_structs.h	Adds php_properties to various internal structs to separate dynamic props from cached BSON properties.
src/BSON/Binary.c	Releases cached HashTables with zend_hash_release; assigns new property handlers.
src/BSON/DBPointer.c	Same as above for DBPointer.
src/BSON/Decimal128.c	Same as above for Decimal128.
src/BSON/Int64.c	Same as above for Int64.
src/BSON/Javascript.c	Same as above for Javascript.
src/BSON/Iterator.c	Same as above for Iterator.
src/BSON/ObjectId.c	Same as above for ObjectId.
src/BSON/PackedArray.c	Same as above for PackedArray.
src/BSON/Regex.c	Same as above for Regex.
src/BSON/Symbol.c	Same as above for Symbol.
src/BSON/Timestamp.c	Same as above for Timestamp.
src/BSON/UTCDateTime.c	Same as above for UTCDateTime.
src/BSON/MaxKey.c	Adds php_properties cleanup; assigns new property handlers.
src/BSON/MinKey.c	Adds php_properties cleanup; assigns new property handlers.
src/BSON/Document.c	Updates cached HashTable teardown to use zend_hash_release.
src/MongoDB/Manager.c	Switches subscriber HashTable teardown to zend_hash_release.
src/MongoDB/ServerApi.c	Adds php_properties cleanup; assigns new property handlers.
src/MongoDB/ServerDescription.c	Adds php_properties cleanup; assigns new property handlers.
src/MongoDB/TopologyDescription.c	Adds php_properties cleanup; assigns new property handlers.
tests/bson/bug2505-001.phpt	New regression test: gc_collect_cycles() doesn’t break foreach.
tests/bson/bug2505-002.phpt	New regression test: set/unset dynamic prop doesn’t break foreach.
tests/bson/bug2505-003.phpt	New regression test: set/unset via reference doesn’t break foreach.
tests/bson/bug1598-002.phpt	Updates existing GC-cycle test to run on newer PHP versions.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Copilot review overview

🟢 Approval recommended

The focused implementation addresses the reported behavior and includes representative regression coverage.

Review effort: Balanced
Findings: None

[kevinAlbs]

LGTM with a possible comment tweak.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The fix changes Zend garbage-collector integration and object-property lifetime behavior, warranting final human review.

Review effort: Balanced
Findings: None

[Copilot]

Copilot review overview

🟢 Approval recommended

The focused implementation addresses the reported behavior and includes appropriate regression coverage.

Review effort: Balanced
Findings: None