[go: up one dir, main page]
Skip to content
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

Jump to bottom

Setting DOMAttr::textContent results in an empty attribute value. #10234

Closed
ThomasWeinert opened this issue Jan 5, 2023 · 2 comments · Fixed by ThePHPF/thephp.foundation#90
Closed

Setting DOMAttr::textContent results in an empty attribute value. #10234

ThomasWeinert opened this issue Jan 5, 2023 · 2 comments · Fixed by ThePHPF/thephp.foundation#90
Labels
Bug Extension: dom Status: Verified

Comments

@ThomasWeinert
Copy link
Contributor

Description

The following code:

<?php
$document = new DOMDocument();
$document->loadXML('<element attribute="value"/>');
$attribute = $document->documentElement->getAttributeNode('attribute');

// output loaded attribute value
var_dump($attribute->textContent);

// change value
$attribute->textContent = 'new value';

// output changed value
var_dump($attribute->textContent);

Resulted in this output:

string(5) "value"
string(0) ""

But I expected this output instead:

string(5) "value"
string(9) "new value"

https://3v4l.org/ouRV0

This work in PHP Versions 5.6.1 to 5.6.13, it is broken for any PHP version after that.

PHP Version

PHP 8.2.1

Operating System

No response
@cmb69
Copy link
Member
cmb69 commented Jan 5, 2023

Almost certainly caused by b2954c6.

@nielsdos
Copy link
Member
nielsdos commented Jan 5, 2023 •
edited
Loading

The following patch works for me. Although I'm not 100% sure this is the right way. I just mimicked the libxml2 source code to get the "else if" condition (https://github.com/GNOME/libxml2/blob/master/tree.c#L5927). EDIT: I'm missing the properties update I think so the "else if" case is not 100% correct yet. I guess the "else if" case can be solved by calling the xmlNodeSetContent function.

diff --git a/ext/dom/node.c b/ext/dom/node.c
index d55ac99efe..78023389a9 100644
--- a/ext/dom/node.c
+++ b/ext/dom/node.c
@@ -769,17 +769,23 @@ int dom_node_text_content_write(dom_object *obj, zval *newval)
 		return FAILURE;
 	}
 
-	if (nodep->type == XML_ELEMENT_NODE || nodep->type == XML_ATTRIBUTE_NODE) {
+	const xmlChar *xmlChars = (const xmlChar *) ZSTR_VAL(str);
+	int type = nodep->type;
+
+	if (type == XML_ELEMENT_NODE || type == XML_ATTRIBUTE_NODE) {
 		if (nodep->children) {
 			node_list_unlink(nodep->children);
 			php_libxml_node_free_list((xmlNodePtr) nodep->children);
 			nodep->children = NULL;
 		}
+
+		xmlNode *textNode = xmlNewText(xmlChars);
+		xmlAddChild(nodep, textNode);
+	} else if (type == XML_TEXT_NODE || type == XML_CDATA_SECTION_NODE || type == XML_ENTITY_REF_NODE || type == XML_ENTITY_NODE || type == XML_PI_NODE || type == XML_COMMENT_NODE || type == XML_NOTATION_NODE) {
+		xmlFree(nodep->content);
+		nodep->content = xmlStrdup(xmlChars);
 	}
 
-	/* we have to use xmlNodeAddContent() to get the same behavior as with xmlNewText() */
-	xmlNodeSetContent(nodep, (xmlChar *) "");
-	xmlNodeAddContent(nodep, (xmlChar *) ZSTR_VAL(str));
 	zend_string_release_ex(str, 0);
 
 	return SUCCESS;

nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 6, 2023
@nielsdos
Fix phpGH-10234: Setting DOMAttr::textContent results in an empty att…
6aa33f2 
…ribute value.

We can't directly call xmlNodeSetContent, because it might encode the string
through xmlStringLenGetNodeList for types
XML_DOCUMENT_FRAG_NODE, XML_ELEMENT_NODE, XML_ATTRIBUTE_NODE.
In these cases we need to use a text node to avoid the encoding.
For the other cases, we *can* rely on xmlNodeSetContent because it is either
a no-op, or handles the content without encoding and clears the properties
field if needed.
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 6, 2023
@nielsdos @ThomasWeinert
Add a regression test for phpGH-10234
e5e8857 
Co-authored-by: ThomasWeinert <thomas@weinert.info>
@nielsdos nielsdos mentioned this issue Jan 6, 2023
Closed
nielsdos added a commit to nielsdos/php-src that referenced this issue May 6, 2023
@nielsdos
Fix phpGH-10234: Setting DOMAttr::textContent results in an empty att…
bb4c5f7 
…ribute value.

We can't directly call xmlNodeSetContent, because it might encode the string
through xmlStringLenGetNodeList for types
XML_DOCUMENT_FRAG_NODE, XML_ELEMENT_NODE, XML_ATTRIBUTE_NODE.
In these cases we need to use a text node to avoid the encoding.
For the other cases, we *can* rely on xmlNodeSetContent because it is either
a no-op, or handles the content without encoding and clears the properties
field if needed.

The test was taken from the issue report, for the test:
Co-authored-by: ThomasWeinert <thomas@weinert.info>

Closes phpGH-10245.
nielsdos added a commit that referenced this issue May 29, 2023
@nielsdos
Merge branch 'PHP-8.1' into PHP-8.2
9ff1ea6 
* PHP-8.1:
  Fix GH-10234: Setting DOMAttr::textContent results in an empty attribute value
nielsdos added a commit that referenced this issue May 29, 2023
@nielsdos
Merge branch 'PHP-8.2'
99ec0c1 
* PHP-8.2:
  Fix GH-10234: Setting DOMAttr::textContent results in an empty attribute value
@pronskiy pronskiy mentioned this issue Jun 6, 2023
Merged
@pitbulk pitbulk mentioned this issue Sep 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Extension: dom Status: Verified
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add Roundup #13 pronskiy/thephp.foundation
3 participants
@ThomasWeinert @cmb69 @nielsdos