Implement LIBXML_NOXMLDECL #11794
Implement LIBXML_NOXMLDECL #11794
Conversation
dff05a7 to
73d8a93
Compare
73d8a93 to
ea00ab9
Compare
ext/dom/document.c
Outdated
| xmlSaveNoEmptyTags = 1; | ||
| } | ||
| saveempty = xmlSaveNoEmptyTags; | ||
| xmlSaveNoEmptyTags = !!(options & LIBXML_SAVE_NOEMPTYTAG); |
There was a problem hiding this comment.
| xmlSaveNoEmptyTags = !!(options & LIBXML_SAVE_NOEMPTYTAG); | |
| xmlSaveNoEmptyTags = (options & LIBXML_SAVE_NOEMPTYTAG); |
I'm confused by the double !! here
There was a problem hiding this comment.
We must put 1 in xmlSaveNoEmptyTags is LIBXML_SAVE_NOEMPTYTAG is set in options. (options & LIBXML_SAVE_NOEMPTYTAG) does not equal 1 because LIBXML_SAVE_NOEMPTYTAG is not 1. By using !! the value is converted to a bool basically.
I can change it to a ternary (options & LIBXML_SAVE_NOEMPTYTAG) ? 1 : 0 though.
ext/dom/document.c
Outdated
| mem = (xmlChar*) xmlBufferContent(buf); | ||
| if (!mem) { | ||
| xmlBufferFree(buf); | ||
| xmlSaveNoEmptyTags = saveempty; |
There was a problem hiding this comment.
Is xmlSaveNoEmptyTags a global libxml2 variable? Because that code makes no sense otherwise
So maybe add some comment about this?
There was a problem hiding this comment.
Is xmlSaveNoEmptyTags a global libxml2 variable?
Yes
So maybe add some comment about this?
Will do
ext/dom/document.c
Outdated
| dom_object *intern, *nodeobj; | ||
| int size, format, saveempty = 0; | ||
| int size, format, saveempty; |
There was a problem hiding this comment.
Is that saveempty basically a bool ? (also maybe rename it because it's not very descriptive ...)
There was a problem hiding this comment.
It's used to store the old value of that libxml2 global. That global is an int so I prefer to keep the same type (also for forwards compatibility, you never know they add some extra flags in there 🤷).
I'll change the name though.
There was a problem hiding this comment.
I'm confused and also what is LIBXML_NOXMLDECL meant to do?
NO XML DECL: No XML declaration
It omits the declaration from the output of calling saveXML().
If this is in master maybe we could add a new constant that has some extra underscores to make the constant easier to read and more descriptive and alias that one?
Unfortunately, for whatever reason, the constants are written without underscore https://www.php.net/manual/en/libxml.constants.php 🙄
I'm not going to change that here though...
ext/dom/document.c
Outdated
| dom_object *intern, *nodeobj; | ||
| int size, format, saveempty = 0; | ||
| int size, format, saveempty; |
There was a problem hiding this comment.
It's used to store the old value of that libxml2 global. That global is an int so I prefer to keep the same type (also for forwards compatibility, you never know they add some extra flags in there 🤷).
I'll change the name though.
ext/dom/document.c
Outdated
| xmlSaveNoEmptyTags = 1; | ||
| } | ||
| saveempty = xmlSaveNoEmptyTags; | ||
| xmlSaveNoEmptyTags = !!(options & LIBXML_SAVE_NOEMPTYTAG); |
There was a problem hiding this comment.
We must put 1 in xmlSaveNoEmptyTags is LIBXML_SAVE_NOEMPTYTAG is set in options. (options & LIBXML_SAVE_NOEMPTYTAG) does not equal 1 because LIBXML_SAVE_NOEMPTYTAG is not 1. By using !! the value is converted to a bool basically.
I can change it to a ternary (options & LIBXML_SAVE_NOEMPTYTAG) ? 1 : 0 though.
ext/dom/document.c
Outdated
| mem = (xmlChar*) xmlBufferContent(buf); | ||
| if (!mem) { | ||
| xmlBufferFree(buf); | ||
| xmlSaveNoEmptyTags = saveempty; |
There was a problem hiding this comment.
Is xmlSaveNoEmptyTags a global libxml2 variable?
Yes
So maybe add some comment about this?
Will do
|
Thanks for the explanations, looks good to me now. |
Fixes GH-11792.
Marking as a draft because I need to review it myself and double check if I messed up.OK nowTargeting master since this is on the border of new feature vs bugfix.