I just noticed that empty "cdata" nodes have an empty "value" even if XML_OPTION_SKIP_WHITE is set; this patch doesn't change that. Not sure if it should, though. Probably not, since this is actually a different issue, but it makes it harder to document the details. :(

PS: it is actually different, see https://3v4l.org/ue9dU and https://3v4l.org/GTZtn. So, if XML_OPTION_SKIP_WHITE is set, and the cdata node is empty or contains whitespace only, the whole element is skipped. This should likely be the same for "complete" elements, but for BC reasons I wouldn't want to change that in a stable version. Sigh.

I just noticed that empty "cdata" nodes have an empty "value" even if XML_OPTION_SKIP_WHITE is set; this patch doesn't change that. Not sure if it should, though. Probably not, since this is actually a different issue, but it makes it harder to document the details. :(

Unfortunately I was not able to find any information regarding XML_OPTION_SKIP_WHITE behavior as well. I may try to work on skipping the whole value object if it makes sense, but it will introduce BC break as you have already mentioned.

Unfortunately I was not able to find any information regarding XML_OPTION_SKIP_WHITE behavior as well. I may try to work on skipping the whole value object if it makes sense, but it will introduce BC break as you have already mentioned.

I suggest to leave this as is for now, but I noticed a change due to this PR regarding cdata element. Please see https://3v4l.org/L4vTC; this has no "cdata" element, but with this PR there is one now. I think that needs to fixed. This extension's test suite is definitely not comprehensive enough. :(

Unfortunately I was not able to find any information regarding XML_OPTION_SKIP_WHITE behavior as well. I may try to work on skipping the whole value object if it makes sense, but it will introduce BC break as you have already mentioned.

I suggest to leave this as is for now, but I noticed a change due to this PR regarding cdata element. Please see https://3v4l.org/L4vTC; this has no "cdata" element, but with this PR there is one now. I think that needs to fixed. This extension's test suite is definitely not comprehensive enough. :(

Ok I will leave this PR as it is. Does it make sense to work on a separate PR for skipping an empty "cdata" element as it was before?

Ok I will leave this PR as it is. Does it make sense to work on a separate PR for skipping an empty "cdata" element as it was before?

I think this needs to be fixed in this PR; otherwise we would introduce a BC break.

By the way, do you test locally with libxml2 or libexpat? I don't think that we have CI testing with libexpat, but the behavior may be different.

Ok I will leave this PR as it is. Does it make sense to work on a separate PR for skipping an empty "cdata" element as it was before?

I think this needs to be fixed in this PR; otherwise we would introduce a BC break.

By the way, do you test locally with libxml2 or libexpat? I don't think that we have CI testing with libexpat, but the behavior may be different.

Ok, will try to fix this issue in the current PR.

Based on compile info it is libxml2. Sorry, I am really new in C and do not know how to change library and test using libexpat. I need to dig deeper into code to find a way.

No need for you to check with libexpat; I can do that. :)

No need for you to check with libexpat; I can do that. :)

Thank you :)

Something like the following might be sufficient to solve the cdata issue:

 ext/xml/xml.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ext/xml/xml.c b/ext/xml/xml.c
index 45fd06971d..273e523d94 100644
--- a/ext/xml/xml.c
+++ b/ext/xml/xml.c
@@ -948,7 +948,9 @@ void _xml_characterDataHandler(void *userData, const XML_Char *s, int len)
 
                     add_assoc_string(&tag, "tag", SKIP_TAGSTART(parser->ltags[parser->level-1]));
                     add_assoc_str(&tag, "value", decoded_value);
-                    add_assoc_string(&tag, "type", "cdata");
+					if (!parser->skipwhite) {
+                    	add_assoc_string(&tag, "type", "cdata");
+					}
                     add_assoc_long(&tag, "level", parser->level);
 
                     zend_hash_next_index_insert(Z_ARRVAL(parser->data), &tag);

This diff won't cleanly apply, because it's based on an earlier version of this PR, but you can apply manually. :)

PS: Nope, disregard this. Sorry for the noise.

Maybe we should go for a simpler fix?

 ext/xml/xml.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ext/xml/xml.c b/ext/xml/xml.c
index fd8aebe03a..df052b8910 100644
--- a/ext/xml/xml.c
+++ b/ext/xml/xml.c
@@ -900,7 +900,6 @@ void _xml_characterDataHandler(void *userData, const XML_Char *s, int len)
 					break;
 				}
 			}
-			if (doprint || (! parser->skipwhite)) {
 				if (parser->lastwasopen) {
 					zval *myval;
 
@@ -915,7 +914,7 @@ void _xml_characterDataHandler(void *userData, const XML_Char *s, int len)
 						add_assoc_str(parser->ctag, "value", decoded_value);
 					}
 
-				} else {
+				} else if (doprint || (! parser->skipwhite)) {
 					zval tag;
 					zval *curtag, *mytype, *myval;
 
@@ -949,7 +948,6 @@ void _xml_characterDataHandler(void *userData, const XML_Char *s, int len)
 					} else if (parser->level == (XML_MAXLEVEL + 1)) {
 											php_error_docref(NULL, E_WARNING, "Maximum depth exceeded - Results truncated");
 					}
-				}
 			} else {
 				zend_string_release_ex(decoded_value, 0);
 			}

And then fix the indentation, and int → size_t. I have not tested carefully, but it seems to work, and not to introduce any BC breaks.

Maybe we should go for a simpler fix?

 ext/xml/xml.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ext/xml/xml.c b/ext/xml/xml.c
index fd8aebe03a..df052b8910 100644
--- a/ext/xml/xml.c
+++ b/ext/xml/xml.c
@@ -900,7 +900,6 @@ void _xml_characterDataHandler(void *userData, const XML_Char *s, int len)
 					break;
 				}
 			}
-			if (doprint || (! parser->skipwhite)) {
 				if (parser->lastwasopen) {
 					zval *myval;
 
@@ -915,7 +914,7 @@ void _xml_characterDataHandler(void *userData, const XML_Char *s, int len)
 						add_assoc_str(parser->ctag, "value", decoded_value);
 					}
 
-				} else {
+				} else if (doprint || (! parser->skipwhite)) {
 					zval tag;
 					zval *curtag, *mytype, *myval;
 
@@ -949,7 +948,6 @@ void _xml_characterDataHandler(void *userData, const XML_Char *s, int len)
 					} else if (parser->level == (XML_MAXLEVEL + 1)) {
 											php_error_docref(NULL, E_WARNING, "Maximum depth exceeded - Results truncated");
 					}
-				}
 			} else {
 				zend_string_release_ex(decoded_value, 0);
 			}

And then fix the indentation, and int → size_t. I have not tested carefully, but it seems to work, and not to introduce any BC breaks.

Tag <c>\n \t</c> will not be skipped in this case and will be present in resulting array with value:
array(4) { ["tag"]=> string(1) "C" ["type"]=> string(8) "complete" ["level"]=> int(2) ["value"]=> string(3) " "

Oh, right! Just when I thought I understand what the function is currently doing, I have to learn more about it. :)

Oh, right! Just when I thought I understand what the function is currently doing, I have to learn more about it. :)

Latest commit returns an original behavior for skipping empty "cdata" elements. I think all cases are covered now and there is no BC break anymore :)

@Flashwade1990 - This introduces a bug, a CRLF and multiple spaces gets added to the structure.

using the following code snipet

$p = xml_parser_create('UTF-8');
xml_parser_set_option($p, XML_OPTION_CASE_FOLDING, 0);
xml_parser_set_option($p, XML_OPTION_SKIP_WHITE, true);
xml_parse_into_struct($p, $xml, $vals, $index);
$errcode = xml_get_error_code($p);

echo $errcode;
xml_parser_free($p);
var_dump($vals[7]);

result on onlinephp.io:

having the next tag on the same line as the ending CDATA tag is a workaround:

@radu-barbu-sage, this is deliberate. XML_OPTION_SKIP_WHITE is documented to "Whether to skip values consisting of whitespace characters." If you want to remove surrounding whitespace, use trim().