Skip to content

dom: fix attribute namespaces from foreign content in the HTML parser - #23598

Closed
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:promote/dom-foreign-id
Closed

dom: fix attribute namespaces from foreign content in the HTML parser#23598
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:promote/dom-foreign-id

Conversation

@iliaal

@iliaal iliaal commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Two defects in lexbor_libxml2_bridge_convert(). The id registration tested for lexbor's HTML namespace, so Dom\HTMLDocument::getElementById() never found ids on SVG or MathML elements. Separately, the xlink, xml and xmlns branches tested a field that lexbor initializes from the element's namespace and overrides only through the foreign-attribute adjust table, which the parser wires up for SVG and MathML alone, so a fragment parsed with an xlink, xml or xmlns context element came back as <z xlink:id="q" xlink:foo="1">.

Restricting those branches to attributes lexbor actually adjusted fixes the second, and leaves lxml_attr->ns as the right thing for the id check to read.

@iliaal
iliaal requested a review from devnexen as a code owner September 6, 2026 15:07
@iliaal
iliaal force-pushed the promote/dom-foreign-id branch from c2bb921 to 25a8e7d Compare September 6, 2026 15:08
iliaal added a commit to iliaal/php-src that referenced this pull request Sep 6, 2026
@iliaal
dom: register unprefixed id attributes from foreign content in HTML
25a8e7d 
The HTML5 parser bridge only marked an id attribute as XML_ATTRIBUTE_ID
when the attribute sat in the HTML namespace, so ids on SVG and MathML
elements never reached getElementById(). The condition the bridge wants
is that the attribute itself is unprefixed, which lxml_attr->ns already
records: it is only set for the xmlns, xlink and xml namespaces.

Closes phpGH-23598
@iliaal iliaal mentioned this pull request Sep 6, 2026
Closed
devnexen
devnexen requested changes Sep 8, 2026
View reviewed changes
Comment thread ext/dom/html5_parser.c

/* xmlIsID does some other stuff too that is irrelevant here. */
if (local_name_length == 2 && local_name[0] == 'i' && local_name[1] == 'd' && attr->node.ns == LXB_NS_HTML) {
if (local_name_length == 2 && local_name[0] == 'i' && local_name[1] == 'd' && lxml_attr->ns == NULL) {

@devnexen devnexen Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (local_name_length == 2 && local_name[0] == 'i' && local_name[1] == 'd' && lxml_attr->ns == NULL) {
if (local_name_length == 2 && local_name[0] == 'i' && local_name[1] == 'd') {

@iliaal iliaal Sep 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That also registers a genuinely namespaced id. lxml_attr->ns is non-NULL once a fragment is parsed into a foreign context:

$d = Dom\HTMLDocument::createEmpty();
$z = $d->createElementNS('http://www.w3.org/1999/xlink', 'z');
$d->appendChild($z);
$z->innerHTML = '<z id="q"></z>';
var_dump($d->getElementById('q'));

Without the check that returns the element, with it NULL. The test didn't cover the case, which is why the guard read as redundant; pinned in 3e45b39.

@devnexen devnexen Sep 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what about this test ?

--TEST--
Dom\HTMLDocument::getElementById() finds ids of SVG and MathML elements
--EXTENSIONS--
dom
--FILE--
<?php
$html = '<!DOCTYPE html><html><body><svg id="s"><rect xml:id="r"/></svg><math id="m"></math><p id="p"></p></body></html>';
$d = Dom\HTMLDocument::createFromString($html, LIBXML_NOERROR);
var_dump([
    'svg #s' => $d->getElementById('s')?->tagName,
    'math #m' => $d->getElementById('m')?->tagName,
    'html #p' => $d->getElementById('p')?->tagName,
    'xml:id #r' => $d->getElementById('r')?->tagName,
]);

/* An unprefixed id is in no namespace whatever namespace its element is in. */
$d2 = Dom\HTMLDocument::createEmpty();
$z = $d2->createElementNS('http://www.w3.org/1999/xlink', 'z');
$d2->appendChild($z);
$z->innerHTML = '<z id="q"></z>';
var_dump($d2->getElementById('q')?->tagName);
?>
--EXPECT--
array(4) {
  ["svg #s"]=>
  string(3) "svg"
  ["math #m"]=>
  string(4) "math"
  ["html #p"]=>
  string(1) "P"
  ["xml:id #r"]=>
  NULL
}
string(1) "Z"

then it might need ... && !attr->node.prefix) wdyt ?

@iliaal iliaal Sep 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ugh, you're right and my last answer was wrong. That id isn't namespaced, it's mislabeled: lexbor initializes every attribute node with its element's namespace, and before_append_attr is wired only for SVG and MathML, so in an xlink, xml or xmlns fragment context the same branch also turned foo="1" into xlink:foo.

That is a second defect alongside the id one, so both are fixed here: the three namespace branches now require a prefix or the bare xmlns declaration, and the id check reads the namespace the bridge ended up assigning. Your expectations hold. The two conditions are not interchangeable in general though, bare xmlns carries no lexbor prefix but is namespaced; they only coincide for an attribute named id.

iliaal added a commit to iliaal/php-src that referenced this pull request Sep 8, 2026
@iliaal
dom: register id attributes from foreign content in HTML
f28a034 
The HTML5 parser bridge only marked an id attribute as XML_ATTRIBUTE_ID
when the attribute sat in the HTML namespace, so ids on SVG and MathML
elements never reached getElementById(). No namespace test is needed at
all: lexbor assigns an attribute the xml, xlink or xmlns namespace only
through its adjust-foreign-attributes table, and no entry there carries
the local name id, so an attribute named id is always unprefixed.

Closes phpGH-23598
@iliaal
iliaal force-pushed the promote/dom-foreign-id branch from 25a8e7d to f28a034 Compare September 8, 2026 12:22
iliaal added a commit to iliaal/php-src that referenced this pull request Sep 8, 2026
@iliaal
dom: register unprefixed id attributes from foreign content in HTML
3e45b39 
The HTML5 parser bridge only marked an id attribute as XML_ATTRIBUTE_ID
when the attribute sat in the HTML namespace, so ids on SVG and MathML
elements never reached getElementById(). The condition the bridge wants
is that the attribute itself is unprefixed, which lxml_attr->ns already
records: it is set only for the xmlns, xlink and xml namespaces. That
guard stays load-bearing because a fragment parsed into a foreign
context inherits the context namespace, which the test now pins.

Closes phpGH-23598
@iliaal
iliaal force-pushed the promote/dom-foreign-id branch from f28a034 to 3e45b39 Compare September 8, 2026 12:27
@iliaal
iliaal requested a review from devnexen September 8, 2026 12:29
@iliaal
dom: fix attribute namespaces from foreign content in the HTML parser
6761e4f 
The bridge registered an id as XML_ATTRIBUTE_ID only when lexbor
reported the HTML namespace, so ids on SVG and MathML elements never
reached getElementById(). Separately, lexbor initializes every attribute
node with its element's namespace and overrides that only through the
foreign-attribute adjust table, which the parser wires up for SVG and
MathML alone, so a fragment parsed with an xlink, xml or xmlns context
element came back as <z xlink:id="q" xlink:foo="1">. Restrict the three
namespace branches to attributes lexbor adjusted, and key the id
registration off the namespace the bridge assigned.

Closes phpGH-23598
@iliaal
iliaal force-pushed the promote/dom-foreign-id branch from 3e45b39 to 6761e4f Compare September 16, 2026 20:02
@iliaal iliaal changed the title dom: register unprefixed id attributes from foreign content in HTML dom: fix attribute namespaces from foreign content in the HTML parser Sep 16, 2026
@iliaal iliaal closed this in 92b0894 Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devnexen devnexen devnexen approved these changes

Assignees

No one assigned

Labels

Extension: dom

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@iliaal @devnexen