Skip to content

Gh21768 dom#21773

Open
devnexen wants to merge 2 commits intophp:masterfrom
devnexen:gh21768_dom
Open

Gh21768 dom#21773
devnexen wants to merge 2 commits intophp:masterfrom
devnexen:gh21768_dom

Conversation

@devnexen
Copy link
Copy Markdown
Member

No description provided.

Emit ZEND_ACC_PUBLIC_SET / PROTECTED_SET / PRIVATE_SET from the
corresponding Modifiers::*_SET flags in generated arginfo, gated
to PHP 8.4+ where asymmetric visibility was introduced. Previously
private(set) and friends in stubs parsed without error but produced
no set-visibility flag.
@readonly on DOM property stubs was documentation only and did not
translate to any runtime flag, so reflection reported the properties
as writable while the write_property handler threw on assignment.
Declaring them public private(set) lets the engine reject external
writes via the normal visibility check and lets ReflectionProperty::
isWritable() answer honestly.

Also drops @readonly from the three non-virtual HTMLCollection
$children properties (Element, DocumentFragment, Document) and
applies private(set) there as well.

Depends on the assertion removal from phpGH-21769 for isReadable() to
stop aborting on these properties.
@devnexen devnexen marked this pull request as ready for review April 16, 2026 16:04
@devnexen devnexen requested a review from kocsismate as a code owner April 16, 2026 16:04
Copy link
Copy Markdown
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, given Nora's comment that these can change at runtime.

@ndossche
Copy link
Copy Markdown
Member

ndossche commented Apr 17, 2026

I presume you'll need to change the error of dom_write_property as it now manually emits a readonly property error.
zend_asymmetric_visibility_property_modification_error may need to be used, if it's not already automagically handled by the engine after this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants