-
Notifications
You must be signed in to change notification settings - Fork 7.9k
[RFC] Add a way to opt-in ext/dom spec compliance #13031
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.
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
Conversation
ff1e704
to
a20c0e9
Compare
fad5375
to
2c41dc7
Compare
b27984b
to
e2a9ea5
Compare
839a918
to
b09690d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
ad5cb06
to
495c80b
Compare
1e0de69
to
371a6c7
Compare
371a6c7
to
2bab732
Compare
2bab732
to
6dd522a
Compare
if (mode == DOM_LOAD_FILE) { | ||
zend_throw_exception_ex(NULL, 0, "Cannot open file '%s'", source); | ||
if (lxml_doc == DOM_DOCUMENT_MALFORMED) { | ||
zend_throw_exception_ex(NULL, 0, "XML document is malformed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this throw a DOM/XML specific Exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will already have been warnings from libxml; and I don't think any of the DOMException types fit here (see domexception.h/c).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are nearly there
ext/dom/element.c
Outdated
bool should_free_result; | ||
const xmlChar *result = dom_get_attribute_ns(intern, elemp, uri, uri_len, name, &should_free_result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah seems this hasn't been done yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I think this as good as a review that I'll be able to do.
Thanks for the work!
This reverts commit f0af56f.
Hey hi ! I think this PR breaks
|
@drupol ext/xsl has a dependency on DOM, for some reason it can't find the The only thing I spot, but this is also true for older branches, is that I don't see an explicit dependency added in the config.m4, which the following patch should resolve:
but given that this has been wrong for years, it probably does not explain your build problem. |
Thanks Niels, I applied your patch and rebuild, with the very last commit:
|
@drupol I don't understand the Nix build system, but I think that for some reason HAVE_DOM is not defined. |
Indeed, setting the var |
I'll be glad to tell you more in a visio at your best convenience! |
I have the same issue, reported as #13764. |
RFC: https://wiki.php.net/rfc/opt_in_dom_spec_compliance
In short: this PR aims to fix all the spec bugs in ext/dom. It does so in a backwards compatible way. This means that existing PHP code will keep working and not notice any changes. When the developer uses the new
DOM\XMLDocument
andDOM\HTMLDocument
classes that I introduced in https://wiki.php.net/rfc/domdocument_html5_parser then the DOM extension switches to a "spec compliant" mode.This gets rid of many issues that have been plaguing the DOM extension since forever and fixes many bugs that are otherwise unfixable.
What's missing in the current implementation is fixes to the property/method types. For example, the fact thatDOM\Node::$prefix
returns the empty string instead of NULL when no prefix is set (and more of course, this is just an example).