[RFC] Improve callbacks in ext/dom and ext/xsl #12627

nielsdos · 2023-11-07T19:29:04Z

RFC link: https://wiki.php.net/rfc/improve_callbacks_dom_and_xsl

Girgias · 2024-01-11T19:21:12Z

ext/dom/tests/DOMXPath_callables.phpt

+$xpath->registerPhpFunctions(null);
+$xpath->evaluate("//a[php:function('var_dump', string(@href))]");
+
+echo "--- Error cases ---\n";


We usually split error cases into a different test file

Girgias · 2024-01-11T19:21:41Z

ext/dom/tests/registerPhpFunctionNS.phpt

+
+$xpath = new DOMXPath($doc);
+
+echo "--- Error cases ---\n";


Girgias · 2024-01-11T19:22:55Z

ext/dom/xpath.c

+
+static void dom_xpath_proxy_factory(xmlNodePtr node, zval *child, dom_object *intern, xmlXPathParserContextPtr ctxt)
+{
+	(void) ctxt;


Don't we have a ZEND_UNUSED macro attribute for those cases?

Girgias · 2024-01-11T19:24:02Z

ext/dom/xpath.c

+	if (!zend_is_executing()) {
 		xmlGenericError(xmlGenericErrorContext,
 		"xmlExtFunctionTest: Function called from outside of PHP\n");
-		error = 1;


Early return to have the happy path without any indentation?

Also maybe UNEXPECTED() ?

Girgias · 2024-01-11T19:25:52Z

ext/dom/xpath.c

+	ZEND_PARSE_PARAMETERS_END();
+
+	if (zend_string_equals_literal(namespace, "http://php.net/xpath")) {
+		zend_argument_value_error(1, "must not be \"http://php.net/xpath\" because it is reserved for PHP");


Suggested change

zend_argument_value_error(1, "must not be \"http://php.net/xpath\" because it is reserved for PHP");

zend_argument_value_error(1, "must not be \"http://php.net/xpath\" because it is reserved by PHP");

Seems more logical?

ext/dom/xpath_callbacks.c

Girgias · 2024-01-11T19:46:13Z

ext/dom/xpath_callbacks.c

+		if (UNEXPECTED(result == FAILURE)) {
+			return FAILURE;
+		}


This looks wrong.

I wouldn't expect result to ever be FAILURE see the comment in zend_API.h:

/* Can only return FAILURE if EG(active) is false during late engine shutdown. * If the call or call setup throws, EG(exception) will be set and the retval * will be UNDEF. Otherwise, the retval will be a non-UNDEF value. */

Good catch, a bit surprising though.

Girgias · 2024-01-11T19:48:08Z

ext/dom/xpath_callbacks.c

+	if (UNEXPECTED(num_args == 0)) {
+		zend_throw_error(NULL, "Function name must be passed as the first argument");
+		goto cleanup_no_obj;
+	}


Is this an API usage failure? As this seems like it should only ever be possible to hit this case if C code does something wrong? Which maybe means this should be an assertion? Or am I missing something?

Yes this is possible to hit by using a wrong XSL template that does: php:function() in its xpath expression instead of php:function('function_name').

Girgias · 2024-01-11T19:48:36Z

ext/dom/xpath_callbacks.c

+	/* Last element of the stack is the function name */
+	xmlXPathObjectPtr obj = valuePop(ctxt);
+	if (obj->stringval == NULL) {
+		zend_type_error("Handler name must be a string");
+		goto cleanup;
+	}


See #12627 (comment) it's possible to hit this using php:function(1234) for example

ext/dom/xpath.c

nielsdos · 2024-01-11T22:26:34Z

I believe I fixed or replied to all issues.

Girgias

LGTM

frederikbosch · 2024-11-22T20:47:56Z

The registerPhpFunctionNS functions are not yet documented, are they? I could provide these. Is it correct that I should add them here and then create a file for them inside the xsltprocessor folder?

nielsdos · 2024-11-22T20:54:54Z

Hi @frederikbosch

The registerPhpFunctionNS functions are not yet documented, are they?

Yes it's not documented yet, we're tracking documentation status in php/doc-en#3872.
Thanks for your interest!

I could provide these. Is it correct that I should add them here and then create a file for them inside the xsltprocessor folder?

Indeed, adding an entry there for the version number, and if I were you I'd make a copy of the registerPHPFunctions xml document as a start for registerPHPFunctionNS and modify from there.

Same thing should be done for DOMXPath, but if you get a PR going for XSLTProcessor that is already great and can serve as a basis for DOMXPath's version of registerPHPFunctionNS.

The docs repo contains instructions on how to build the docs locally.

frederikbosch · 2024-11-22T20:58:56Z

Thanks @nielsdos, as I suggested this change in the internals RFC discussion I am absolutely interested in this addition. It will serve my XSL 2.0 transpiler library very well. So, I'd say it's my turn to thank you for the implementation! And I'll do my best to come up with both PRs.

frederikbosch · 2024-11-22T22:43:13Z

See the PR @ doc-en.

github-actions bot added Category: Build System Extension: dom Extension: xsl labels Nov 7, 2023

nielsdos added the RFC label Nov 7, 2023

nielsdos force-pushed the rfc-better-registerFunctions branch from 14499b1 to 5274a1c Compare November 15, 2023 18:51

nielsdos force-pushed the rfc-better-registerFunctions branch 5 times, most recently from 542ae01 to d219188 Compare November 30, 2023 22:10

nielsdos force-pushed the rfc-better-registerFunctions branch 3 times, most recently from cfc2fe5 to d7ee39b Compare January 1, 2024 21:13

nielsdos added 6 commits January 7, 2024 23:03

Improve callbacks in ext/dom and ext/xsl

84d143f

Refactor code to allow multiple namespaces

b009284

Implement namespaced functions for DOM

e8b9b5e

Implement namespaced functions for XSL

70817b9

Update to the new registerPhpFunctionNS proposal

ce5ab06

Add additional test

48bcd44

nielsdos force-pushed the rfc-better-registerFunctions branch from d7ee39b to 48bcd44 Compare January 7, 2024 22:06

nielsdos requested a review from Girgias January 11, 2024 18:44

Girgias reviewed Jan 11, 2024

View reviewed changes

nielsdos added 3 commits January 11, 2024 23:07

Split error cases off of tests

28a3c40

Address code review comments

552dde0

Split off more tests

32a6270

nielsdos requested a review from kocsismate as a code owner January 11, 2024 22:25

Girgias approved these changes Jan 12, 2024

View reviewed changes

nielsdos added 2 commits January 12, 2024 23:53

[ci skip] NEWS

eb1e449

[ci skip] UPGRADING

657679e

[ci skip] UPGRADING

9561820

nielsdos merged commit 90785dd into php:master Jan 12, 2024
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Improve callbacks in ext/dom and ext/xsl #12627

[RFC] Improve callbacks in ext/dom and ext/xsl #12627

nielsdos commented Nov 7, 2023 •

edited

Loading

Girgias Jan 11, 2024

Girgias Jan 11, 2024

Girgias Jan 11, 2024

Girgias Jan 11, 2024

Girgias Jan 11, 2024

Girgias Jan 11, 2024

nielsdos Jan 11, 2024

Girgias Jan 11, 2024

nielsdos Jan 11, 2024

Girgias Jan 11, 2024

nielsdos Jan 11, 2024 •

edited

Loading

nielsdos commented Jan 11, 2024

Girgias left a comment

frederikbosch commented Nov 22, 2024

nielsdos commented Nov 22, 2024

frederikbosch commented Nov 22, 2024

frederikbosch commented Nov 22, 2024

	zend_argument_value_error(1, "must not be \"http://php.net/xpath\" because it is reserved for PHP");
	zend_argument_value_error(1, "must not be \"http://php.net/xpath\" because it is reserved by PHP");

[RFC] Improve callbacks in ext/dom and ext/xsl #12627

[RFC] Improve callbacks in ext/dom and ext/xsl #12627

Conversation

nielsdos commented Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdos Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

nielsdos commented Jan 11, 2024

Girgias left a comment

Choose a reason for hiding this comment

frederikbosch commented Nov 22, 2024

nielsdos commented Nov 22, 2024

frederikbosch commented Nov 22, 2024

frederikbosch commented Nov 22, 2024

nielsdos commented Nov 7, 2023 •

edited

Loading

nielsdos Jan 11, 2024 •

edited

Loading