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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ext/dom: Add global registerNodeNS flag on DOMXPath. #4740

Closed
wants to merge 3 commits into from

Conversation

beberlei
Copy link
Contributor

@beberlei beberlei commented Sep 23, 2019

PHP's DOMXpath automatically registers namespaces for xpath, which can be disabled with arguments $registerNodeNS to evaluate() and query() that defaults to true.

With this PR you can set this flag on the constructor as second argument of the xpath object globally for all evaluate/query calls: 馃槅

$xpath = new DOMXPath($doc, false);

In addition you can also use a property to read and write this value 馃殌

$xpath = new DOMXPath($doc);
$xpath->registerNodeNamespaces = true;

Both approaches are needed, because the option is reasonably important to be set directly in the constructor, but also code that is "downstream" of the actual construction of DOMXPath may need to inspect or modify the setting before use. Without knowing which value this property has at the call site of query/evaluate it would cause problems.

Setting the $registerNodeNS argument on query/evaluate methods will overwrite the default options.

Fixes https://bugs.php.net/bug.php?id=55700

@beberlei beberlei closed this Sep 23, 2019
@beberlei beberlei reopened this Sep 23, 2019
@beberlei
Copy link
Contributor Author

Related: https://github.com/php/php-src/pull/779/files - is a public property better than a constructor param?

@beberlei
Copy link
Contributor Author

Ok, I think while the constructor is the best way to configure this, without a property code that has no access to the constructing site (dependency inversion) might need to find out what mode the DOMXpath instance is in. So we need a property anyways. I think i will be going to add both and make the property read-only.

@beberlei
Copy link
Contributor Author

beberlei commented Oct 5, 2019

Fixed in 6c96369

@beberlei beberlei closed this Oct 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants