Review fixups from @arnaudlb against v0.4.0#14
Merged
nicolas-grekas merged 4 commits intomainfrom Apr 15, 2026
Merged
Conversation
Per @arnaudlb's review: !(pi->flags & ZEND_ACC_VIRTUAL) already implies pi->offset == ZEND_VIRTUAL_PROPERTY_OFFSET (they're the same marker on the property_info side), and IS_HOOKED_PROPERTY_OFFSET() is only meaningful on offsets returned by zend_get_property_offset() — not on the raw pi->offset stored in properties_info. The extra checks were dead code. Collapse to a single bitmask test.
Per @arnaudlb's review: the scalar→backed-enum coercion should not fire when the property type already accepts the scalar literally, e.g. `Suit|string|int`. In that case the caller opted into accepting the raw scalar and promoting it to an enum case would be a surprising silent conversion. Add `(ZEND_TYPE_PURE_MASK(pi->type) & ~MAY_BE_NULL) == 0` so the cast only runs for pure-enum types (`Enum`) and their nullable variant (`?Enum`). Union types get the engine's standard type check.
Per @arnaudlb's review: a lazy ghost has its backing slots uninitialized until the realization hook fires. Writing directly via OBJ_PROP / slot manipulation on such an object would bypass that hook and leave it in a half-initialized state (lazy counters out of sync, lazy-init callback never called). At the top of dc_write_backed_property(), check zend_lazy_object_initialized() and, in the default mode, delegate to the engine's property write path which handles realization. The DEEPCLONE_HYDRATE_NO_LAZY_INIT flag keeps its existing opt-out fast path (skips lazy-init intentionally via Reflection).
Per @arnaudlb's review: zend_std_write_property() skips the object's write_property handler. For internal classes (or user objects that swap handlers at create_object time), that can bypass semantic validation the class expects on property writes. Switch to zend_update_property_ex(scope_ce ?: obj_ce, obj, name, value) which dispatches through the handler, keeping compatibility with overridden write paths. Scope falls back to obj_ce when scope_ce is NULL (stdClass-scoped dynamic prop on a user class).
nicolas-grekas
force-pushed
the
review-fixups
branch
from
7cd8a7e to
55aa4ec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses review feedback from @arnaudlb on the v0.4.0 work (PR #12), split into four semantic commits so each concern can be reviewed (and reverted) independently.
Commits
1. Simplify
dc_is_backed_declared_property()!(pi->flags & ZEND_ACC_VIRTUAL)already impliespi->offset == ZEND_VIRTUAL_PROPERTY_OFFSET(both set together for virtual props on the property_info side), andIS_HOOKED_PROPERTY_OFFSET()is only meaningful on offsets returned byzend_get_property_offset()— not on the rawpi->offset. Collapse to a single bitmask test.2. Narrow backed-enum scalar cast to
Enum/?EnumonlyThe scalar→backed-enum coercion shouldn't fire for union types that already accept the scalar literally (e.g.
Suit|string|int). Add(ZEND_TYPE_PURE_MASK(pi->type) & ~MAY_BE_NULL) == 0so only pure-enum types (nullable or not) trigger the cast.3. Route writes on lazy objects through
zend_update_property_ex()A lazy ghost keeps its backing slots uninitialized until the realization hook fires. Writing directly via
OBJ_PROPwould bypass that hook and leave the object in a half-initialized state. At the top ofdc_write_backed_property(), checkzend_lazy_object_initialized()and delegate to the engine's property write path in the default mode.DEEPCLONE_HYDRATE_NO_LAZY_INITkeeps its opt-out fast path.4. Route dynamic-property fallback through
zend_update_property_ex()zend_std_write_property()skips any overriddenwrite_propertyhandler. For internal classes or user objects that swap handlers atcreate_objecttime, that bypasses semantic validation the class expects. Switch tozend_update_property_ex(scope_ce ?: obj_ce, obj, name, value).Notes on not-applied comments
Test plan
.phpttests green locally (PHP 8.4, NTS)