PHPC-2505: Fix foreach after GC invocation

[GromNaN]

Summary

Fixes a GC/foreach inconsistency for BSON objects: iterating a BSON object with foreach after gc_collect_cycles() (or after setting/unsetting a dynamic property) could return an empty result set.

Root cause

The shared get_gc handler in phongo.c was calling zend_std_get_properties, which populates the zend_object's internal properties table as a side-effect. When the GC later collects that table, it corrupts the cached intern->properties HashTable used by get_properties — causing subsequent foreach iterations to yield no results.

Fix

• Remove the custom phongo_std_get_gc handler (and its registration) from phongo.c
• Add a php_properties HashTable to each affected BSON struct to store dynamic PHP properties separately from the internal properties cache
• Add per-class property handlers (PHONGO_DEFINE_PROPERTY_HANDLERS / PHONGO_ASSIGN_PROPERTY_HANDLERS) so that read_property, write_property, has_property, unset_property, get_property_ptr_ptr, and get_gc all operate exclusively on php_properties — never on the internal BSON properties cache
• Replace zend_hash_destroy + FREE_HASHTABLE with zend_hash_release throughout

Scope

All BSON types are treated uniformly: Binary, DBPointer, Decimal128, Document, Int64, Iterator, Javascript, MaxKey, MinKey, ObjectId, PackedArray, Regex, Symbol, Timestamp, UTCDateTime. MaxKey and MinKey are included because without custom property handlers, dynamic property assignment on them produces deprecation warnings on PHP 8.2+.

Manager, ServerApi, ServerDescription, and TopologyDescription are intentionally out of scope; they can be addressed separately (e.g. via the declared-properties approach from PHPC-2699).

Test plan

• tests/bson/bug2505-001.phpt — gc_collect_cycles() does not affect foreach
• tests/bson/bug2505-002.phpt — set/unset a property does not affect foreach
• tests/bson/bug2505-003.phpt — set/unset via reference does not affect foreach
• tests/bson/bug1598-002.phpt — GC correctly collects cycles through BSON objects (PHP ≥ 8.2 skip removed)

[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]

PHONGO_DEFINE_PROPERTY_HANDLERS's write_property uses zend_hash_add_new(), which will return NULL (and not update the value) if the property already exists. This breaks normal PHP semantics for $obj->prop = ... reassignment and can lead to engine-level issues when a NULL return is unexpected. Consider using an update/replace API (e.g., zend_hash_update) so assignments overwrite existing values and always return a valid zval pointer.

Suggested change

	return zend_hash_add_new(props, name, value); \
	return zend_hash_update(props, name, value); \

[Copilot]

PHONGO_DEFINE_PROPERTY_HANDLERS increments the refcount with Z_TRY_ADDREF_P(value) before inserting into the HashTable. The Zend hash APIs typically copy the zval and manage refcounts themselves; doing an explicit ADDREF here risks over-incrementing and leaking values. Adjust the insertion path so refcounting matches the chosen hash API (e.g., remove the manual ADDREF when using zend_hash_update/zend_hash_add).

[Copilot]

The property-handler helpers always allocate php_properties even for read/isset/unset on objects that have no dynamic properties yet. This creates avoidable allocations and can change object memory behavior just from reading an undefined property. Consider only allocating php_properties on write/reference access (e.g., in write_property and get_property_ptr_ptr) and treating a NULL table as "no dynamic properties" for read/has/unset.

Suggested change

	ALLOC_HASHTABLE(props); \
	zend_hash_init(props, 0, NULL, ZVAL_PTR_DTOR, 0); \
	_intern_extractor(zobj)->php_properties = props; \
	return &EG(uninitialized_zval); \

[Copilot]

With dynamic properties now stored in intern->php_properties instead of zend_object's standard property table, existing clone implementations that call zend_objects_clone_members() will no longer copy dynamic properties. That is a behavioral change for BSON types that support cloning (e.g., Binary/DBPointer/Decimal128/etc.). If dynamic properties are still supported, clone handlers should also duplicate php_properties into the new object (and free it in free_obj).

[Copilot]

The PR description says Manager, ServerApi, ServerDescription, and TopologyDescription are intentionally out of scope, but this change assigns the new property handlers to ServerApi (and adds php_properties/freeing logic). Please confirm whether these Driver classes are intended to be included; if not, either revert these changes or update the PR description/scope accordingly.

[Copilot]

Test name/description mentions setting/unsetting a property interfering with foreach, but this test specifically exercises set/unset via reference ($t = &$test->a). Consider updating the --TEST-- description to reflect the reference use so failures are easier to triage.

Suggested change

	PHPC-2505: Setting and unsetting a property may interfere with using foreach to iterate objects
	PHPC-2505: Setting and unsetting a property via reference may interfere with using foreach to iterate objects