Skip to content

Gh21768 dom #21773

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
devnexen merged 7 commits into from
Apr 18, 2026
Merged

Gh21768 dom #21773

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6fc440b
build/gen_stub: support asymmetric visibility modifiers.
devnexen Apr 16, 2026
c8155e9
ext/dom: use private(set) instead of @readonly for virtual properties.
devnexen Apr 16, 2026
5bb2eec
Emit asymmetric visibility modifiers in field synopsis.
devnexen Apr 17, 2026
3326690
Emit asymmetric visibility error in dom_write_property.
devnexen Apr 17, 2026
50bfa3a
fix tests
devnexen Apr 17, 2026
8e29897
feedback
devnexen Apr 18, 2026
7e46550
revert back
devnexen Apr 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion build/gen_stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2354,7 +2354,17 @@ protected function getFlagsByPhpVersion(): VersionFlags
$flags = "ZEND_ACC_PRIVATE";
}

return new VersionFlags([$flags]);
$versionFlags = new VersionFlags([$flags]);

if ($this->flags & Modifiers::PUBLIC_SET) {
$versionFlags->addForVersionsAbove("ZEND_ACC_PUBLIC_SET", PHP_84_VERSION_ID);
} elseif ($this->flags & Modifiers::PROTECTED_SET) {
$versionFlags->addForVersionsAbove("ZEND_ACC_PROTECTED_SET", PHP_84_VERSION_ID);
} elseif ($this->flags & Modifiers::PRIVATE_SET) {
$versionFlags->addForVersionsAbove("ZEND_ACC_PRIVATE_SET", PHP_84_VERSION_ID);
}
Comment thread
kocsismate marked this conversation as resolved.

return $versionFlags;
}

protected function getTypeCode(string $variableLikeName, string &$code): string
Expand Down Expand Up @@ -2450,6 +2460,17 @@ protected function addModifiersToFieldSynopsis(DOMDocument $doc, DOMElement $fie
$fieldsynopsisElement->appendChild(new DOMText("\n$indentation"));
$fieldsynopsisElement->appendChild($doc->createElement("modifier", "private"));
}

if ($this->flags & Modifiers::PUBLIC_SET) {
$fieldsynopsisElement->appendChild(new DOMText("\n$indentation"));
$fieldsynopsisElement->appendChild($doc->createElement("modifier", "public(set)"));
} elseif ($this->flags & Modifiers::PROTECTED_SET) {
$fieldsynopsisElement->appendChild(new DOMText("\n$indentation"));
$fieldsynopsisElement->appendChild($doc->createElement("modifier", "protected(set)"));
} elseif ($this->flags & Modifiers::PRIVATE_SET) {
$fieldsynopsisElement->appendChild(new DOMText("\n$indentation"));
$fieldsynopsisElement->appendChild($doc->createElement("modifier", "private(set)"));
}
}

}
Expand Down
15 changes: 10 additions & 5 deletions ext/dom/php_dom.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,11 +412,6 @@ zval *dom_write_property(zend_object *object, zend_string *name, zval *value, vo
const dom_prop_handler *hnd = dom_get_prop_handler(obj, name, cache_slot);

if (hnd) {
if (UNEXPECTED(!hnd->write_func)) {
zend_readonly_property_modification_error_ex(ZSTR_VAL(object->ce->name), ZSTR_VAL(name));
return &EG(error_zval);
}

zend_property_info *prop = NULL;
if (cache_slot) {
ZEND_ASSERT(*cache_slot == obj->prop_handler);
Expand All @@ -429,6 +424,16 @@ zval *dom_write_property(zend_object *object, zend_string *name, zval *value, vo
}
}

if (UNEXPECTED(!hnd->write_func)) {
Copy link
Copy Markdown
Member

@ndossche ndossche Apr 18, 2026

Choose a reason for hiding this comment

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

Why did you move this down?

Copy link
Copy Markdown
Member Author

@devnexen devnexen Apr 18, 2026

Choose a reason for hiding this comment

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

The prop lookup has to happen before the !write_func error branch because the asymmetric-visibility error message depends on prop->flags. Moving the early-out above
the lookup would mean duplicating the lookup. Reply:

The prop lookup is needed to distinguish between asymmetric-visibility and readonly error messages, so the !write_func check had to move below it. Happy to factor the
lookup into a helper if you prefer.

Copy link
Copy Markdown
Member

@ndossche ndossche Apr 18, 2026

Choose a reason for hiding this comment

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

Right of course, code is fine.

if (prop && (prop->flags & ZEND_ACC_PPP_SET_MASK) &&
!zend_asymmetric_property_has_set_access(prop)) {
zend_asymmetric_visibility_property_modification_error(prop, "modify");
} else {
Copy link
Copy Markdown
Member

@ndossche ndossche Apr 18, 2026

Choose a reason for hiding this comment

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

Hmm, but is this else branch executable? Do we still have readonly properties left??

Copy link
Copy Markdown
Member Author

@devnexen devnexen Apr 18, 2026

Choose a reason for hiding this comment

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

After your c8155e9 conversion, no DOM stub uses @readonly anymore (grep confirms zero). Every handler registered with a NULL write_func now corresponds to a
private(set) property, so prop should always have ZEND_ACC_PPP_SET_MASK. The else branch is effectively dead.

Copy link
Copy Markdown
Member Author

@devnexen devnexen Apr 18, 2026

Choose a reason for hiding this comment

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

You were right. CI confirmed the else is reachable: Dom\XMLDocument::$textContent inherits Node's NULL-write handler but the stub leaves it plain public (not
private(set)) because subclasses override with a real write_func. For XMLDocument it's readonly-by-implementation, not by visibility, so the fallback is needed.
Reverted.

zend_readonly_property_modification_error_ex(ZSTR_VAL(object->ce->name), ZSTR_VAL(name));
}
return &EG(error_zval);
}

ZEND_ASSERT(prop && ZEND_TYPE_IS_SET(prop->type));
zval tmp;
ZVAL_COPY(&tmp, value);
Expand Down
Loading
Loading