Use ZPP callable check in spl_autoload_register by Girgias · Pull Request #5301 · php/php-src

Girgias · 2020-03-26T00:16:45Z

This also provides more useful information as to why the callable is invalid.

Adds a warning mentioning that the second parameter is ignored when passed false.

ext/spl/php_spl.c

Girgias · 2020-05-03T01:30:07Z

Changed this to use the callable ZPP check which should make this easier to read.

Girgias · 2020-05-12T15:40:37Z

@nikic can I have your thoughts on this?

nikic · 2020-05-12T16:02:23Z

ext/spl/php_spl.c

+		Z_PARAM_BOOL(prepend)
+	ZEND_PARSE_PARAMETERS_END();
+
+	if (!do_throw) {


Do you know why this argument exists in the first place (e.g. from commit history)? It's very weird, but I feel like there must be some reason it was added...

I'll look into it, because I'm quite baffled by it too.

Seems like it was added 12 years ago without much explanation: 0cfdd9a

Reason for change introduction: https://bugs.php.net/bug.php?id=42823
Thanks to @IMSoP for finding the bug report.

composer defaults to true: composer/composer@c80cb76

Docs: https://getcomposer.org/doc/06-config.md#prepend-autoloader

Seems it's pretty critical, except if we make the prepend behaviour the default and deprecate the last 2 arguments.
Or maybe a better idea is to introduce a new function autoload_register(callable $callable, bool $prepend) but that means changing all SPL related autoload functions which doesn't seem really feasible.

Hmm, isn't this about do_throw? That has been introduced with a5c37f3 ("Add ability to fail silently").

Err, sorry, I think was referring to the do_throw argument here, that gets ignored by this patch. I can see the usefulness of prepend, but not so much of do_throw.

Ah my mistake, I would say the consistent TypeError RFC would back this change, but it does make the signature awkward

Girgias · 2020-05-22T11:54:43Z

If there are no objections I'll merge this in a week

nikic

I'm okay with this.

nikic · 2020-05-22T12:32:52Z

ext/spl/php_spl.c

+
+	if (!do_throw) {
+		php_error_docref(NULL, E_WARNING, "spl_autoload_register will always throw, "
+			"the second argument has been ignored");


Maybe follow the Argument #2 ($do_throw) has been ignored, spl_autoload_register() will always throw style here? I'd also downgrade this to E_NOTICE. By the time we reach this code the exception has already been thrown anyway, so this is purely informational.

@kocsismate Is there some existing function for this, for the non-exception case? Or do we just write out the error message explicitly?

No, we've only added utility functions to handle exceptions yet. So yes, explicitly writing this out is the only solution for now.

nikic · 2020-05-22T12:38:45Z

ext/spl/php_spl.c

-			ZVAL_COPY(&alfi.closure, zcallable);
+		if (fcc.function_handler->type == ZEND_INTERNAL_FUNCTION &&
+			fcc.function_handler->internal_function.handler == zif_spl_autoload_call) {
+			zend_argument_value_error(1, "must not be the spl_autoload_call() function as it cannot be registered");


Suggested change

zend_argument_value_error(1, "must not be the spl_autoload_call() function as it cannot be registered");

zend_argument_value_error(1, "must not be the spl_autoload_call() function");

I feel like the "as it cannot be registered" doesn't add anything here.

Girgias · 2020-05-22T15:47:25Z

I've amended the messages and changed the warning to a notice.

Also changed the Zend/tests/bug78868.phpt test to drop the second argument.

This makes it always throw a TypeError, moreover this makes the error message consistent. Added a warning mentioning that the second parameter is now ignored when passed false.

nikic · 2020-05-29T11:12:26Z

Please don't forget the upgrading note for the $do_throw change.

…gisterThrowFalse` sniff > spl_autoload_register() will now always throw a TypeError on invalid > arguments, therefore the second argument $do_throw is ignored and a > notice will be emitted if it is set to false. Refs: * https://github.com/php/php-src/blob/c0172aa2bdb9ea223c8491bdb300995b93a857a0/UPGRADING#L393-L395 * php/php-src#5301 * php/php-src@2302b14 This sniff addresses that change. Includes unit tests.

Girgias force-pushed the improve-spl-autoload-register branch from 2385641 to de4e7e7 Compare March 26, 2020 00:47

KalleZ reviewed Mar 26, 2020

View reviewed changes

ext/spl/php_spl.c Outdated Show resolved Hide resolved

Girgias force-pushed the improve-spl-autoload-register branch 2 times, most recently from c5fa828 to f4c1638 Compare March 27, 2020 01:33

Girgias force-pushed the improve-spl-autoload-register branch from f4c1638 to 2ea3c95 Compare April 23, 2020 21:41

Girgias force-pushed the improve-spl-autoload-register branch from 2ea3c95 to 01523a2 Compare May 3, 2020 01:26

Girgias changed the title ~~Always throw a TypeError with invalid callable in spl_autoload_register~~ Use ZPP callable check in spl_autoload_register May 3, 2020

Girgias force-pushed the improve-spl-autoload-register branch 2 times, most recently from 6096559 to be3ec82 Compare May 12, 2020 15:29

nikic reviewed May 12, 2020

View reviewed changes

Girgias force-pushed the improve-spl-autoload-register branch from be3ec82 to e3fb0e8 Compare May 15, 2020 21:41

nikic reviewed May 22, 2020

View reviewed changes

Girgias force-pushed the improve-spl-autoload-register branch from e3fb0e8 to 17dafc3 Compare May 22, 2020 15:46

Use ZPP callable check for spl_autoload_register.

239a472

This makes it always throw a TypeError, moreover this makes the error message consistent. Added a warning mentioning that the second parameter is now ignored when passed false.

Girgias force-pushed the improve-spl-autoload-register branch from 17dafc3 to 239a472 Compare May 22, 2020 15:50

nikic approved these changes May 29, 2020

View reviewed changes

php-pulls closed this in 2302b14 May 30, 2020

Girgias deleted the improve-spl-autoload-register branch May 30, 2020 12:01

carusogabriel added this to the PHP 8.0 milestone May 30, 2020

jrfnl mentioned this pull request Jul 19, 2020

PHP 8.0: New PHPCompatibility.ParameterValues.RemovedSplAutoloadRegisterThrowFalse sniff PHPCompatibility/PHPCompatibility#1181

Merged

	zend_argument_value_error(1, "must not be the spl_autoload_call() function as it cannot be registered");
	zend_argument_value_error(1, "must not be the spl_autoload_call() function");

Conversation

Girgias commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Girgias commented May 3, 2020

Uh oh!

Girgias commented May 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias commented May 22, 2020

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias commented May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented May 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Girgias commented Mar 26, 2020 •

edited

Loading

Girgias commented May 22, 2020 •

edited

Loading