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.

Sign up for GitHub

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

Jump to bottom

Use ZPP callable check in spl_autoload_register #5301

Closed
wants to merge 1 commit into from
Closed

Use ZPP callable check in spl_autoload_register #5301

wants to merge 1 commit into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Mar 26, 2020 •
edited

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.

@Girgias Girgias force-pushed the improve-spl-autoload-register branch from 2385641 to de4e7e7 Compare March 26, 2020 00:47
KalleZ
KalleZ reviewed Mar 26, 2020
View reviewed changes
ext/spl/php_spl.c Outdated Show resolved Hide resolved
@Girgias Girgias force-pushed the improve-spl-autoload-register branch 2 times, most recently from c5fa828 to f4c1638 Compare March 27, 2020 01:33
@Girgias Girgias force-pushed the improve-spl-autoload-register branch from f4c1638 to 2ea3c95 Compare April 23, 2020 21:41
@Girgias Girgias force-pushed the improve-spl-autoload-register branch from 2ea3c95 to 01523a2 Compare May 3, 2020 01:26
@Girgias 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
Copy link
Member Author

Girgias commented May 3, 2020

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

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

Girgias commented May 12, 2020

@nikic can I have your thoughts on this?

nikic
nikic reviewed May 12, 2020
View reviewed changes
ext/spl/php_spl.c
Z_PARAM_BOOL(prepend)
ZEND_PARSE_PARAMETERS_END();

if (!do_throw) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Girgias commented May 22, 2020

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

nikic
nikic reviewed May 22, 2020
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this.

ext/spl/php_spl.c Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ext/spl/php_spl.c Outdated
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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Girgias force-pushed the improve-spl-autoload-register branch from e3fb0e8 to 17dafc3 Compare May 22, 2020 15:46
@Girgias
Copy link
Member Author

Girgias commented May 22, 2020 •
edited

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.

@Girgias
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 Girgias force-pushed the improve-spl-autoload-register branch from 17dafc3 to 239a472 Compare May 22, 2020 15:50
@nikic
Copy link
Member

nikic commented May 29, 2020

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

@php-pulls php-pulls closed this in 2302b14 May 30, 2020
@Girgias Girgias deleted the improve-spl-autoload-register branch May 30, 2020 12:01
@carusogabriel carusogabriel added this to the PHP 8.0 milestone May 30, 2020
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Jul 19, 2020
@jrfnl
✨ PHP 8.0: New `PHPCompatibility.ParameterValues.RemovedSplAutoloadRe…
999239d 
…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.
@jrfnl jrfnl mentioned this pull request Jul 19, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KalleZ KalleZ KalleZ left review comments

@cmb69 cmb69 cmb69 left review comments

@kocsismate kocsismate kocsismate left review comments

@nikic nikic nikic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
PHP 8.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Girgias @nikic @KalleZ @cmb69 @kocsismate @carusogabriel