Promote warnings to exceptions in the standard lib by kocsismate · Pull Request #4937 · php/php-src

kocsismate · 2019-11-20T09:36:30Z

I tried to choose warnings where promotion didn't seem problematic. I committed all the promotions separately, but I can also explode() the PR into smaller ones.

Girgias

I'm not exactly sure if the ones related to files/ftp should be changed to Errors so I'll hand of the decision to somebody more knowledgable.

Girgias · 2019-11-20T14:39:34Z

Test failures are legit, mostly about fscanf which need to be updated.

…s() function

kocsismate · 2019-11-22T14:14:48Z

Huh, finally the tests are going to be green after my latest fix...

nikic · 2019-12-05T06:37:07Z

 function move_uploaded_file(string $path, string $new_path): bool {}

-function parse_ini_file(string $filename, bool $process_sections = false, int $scanner_mode = INI_SCANNER_NORMAL): array {}
+function parse_ini_file(string $filename, bool $process_sections = false, int $scanner_mode = INI_SCANNER_NORMAL): array|false {}


Isn't this change the wrong way around?

I believe it's not, because as far as I saw parse_ini_file() can also return false:
php-src/ext/standard/basic_functions.c
Line 4236 in 2adb1bb
RETURN_FALSE;

nikic · 2019-12-05T06:43:04Z

A lot in here that is not very clear-cut...

Feel free to already commit the log and settype changes, and the htmlspecialchars capitalization fix.

nikic · 2019-12-05T06:47:28Z

The file_get_contents change is fine as well.

nikic · 2019-12-05T06:50:51Z

@@ -618,7 +618,7 @@ PHPAPI int php_sscanf_internal( char *string, char *format,
 	if (numVars) {
 for (i = varStart;i < argCount;i++){
 if ( ! Z_ISREF(args[ i ] ) ) {


Convert this into ZEND_ASSERT(Z_ISREF(args[i])), it should never fail.

nikic · 2019-12-05T06:56:34Z

 zend_string *function_name
 = zend_get_callable_name(&shutdown_function_entry->arguments[0]);
-php_error(E_WARNING, "(Registered shutdown functions) Unable to call %s() - function does not exist", ZSTR_VAL(function_name));
+zend_value_error("(Registered shutdown functions) Unable to call %s() - function does not exist", ZSTR_VAL(function_name));


A potential problem here is that this warnings is triggered during shutdown, so there is actually no way to catch the exception. Maybe that's okay though, I'm not sure.

kocsismate · 2019-12-05T08:33:51Z

Thanks Nikita for the review! I admit, these changes only seemed "clear cut" for the first sight. I believe though that warning promotions for the password_*() and chgrp() functions should be fine as well. And it might be the case for the ones too related to extension handling, ini file parsing, and to proc_open().

nikic · 2019-12-06T05:37:41Z

@kocsismate chgrp is fine modulo two nits. password_* will require corresponding changes in ext/sodium, some of the code is duplicated there.

kocsismate · 2019-12-06T09:07:48Z

@nikic OK, I'll close this PR then, and will split off these two ones + the changes for sscanf into separate PRs. The following changes were committed to the master: 5898e8ead4d66304deb53d2868ed

nikic · 2019-12-10T15:59:23Z

Btw, I'm not saying the rest here can't land, I just didn't review it in detail. I can take another look after a rebase.

kocsismate · 2019-12-10T16:26:14Z

OK, sorry, I misunderstood your words a little bit. I've just rebased to current master (I hope it worked :S).

kocsismate added 4 commits November 20, 2019 10:30

Promote warning to exception in log() function
441b326

Promote warning to exception in ini_get_all() function
ee3b345

Promote warning to exception in parse_ini_file() function
e5a8bae

Promote warnings to exceptions in dl() function
c30e234

kocsismate force-pushed the standard-exceptions branch from 1709d41 to 19a2cc3Compare November 20, 2019 13:50

Girgias reviewed Nov 20, 2019
View reviewed changes

Comment threadext/standard/password.c Outdated

Girgias requested changes Nov 20, 2019
View reviewed changes

Comment threadext/standard/string.c Outdated
Comment threadext/standard/tests/general_functions/bug72920.phpt Outdated
Comment threadext/standard/tests/general_functions/gettype_settype_error.phpt Outdated
Comment threadext/standard/tests/general_functions/proc_open_pipes3.phpt Outdated

cmb69 added the Waiting on Author label Nov 21, 2019

kocsismate force-pushed the standard-exceptions branch 5 times, most recently from 5611f2b to b0de09aCompare November 22, 2019 11:21

kocsismate added 2 commits November 22, 2019 12:58

Promote warnings to exceptions in password_*() functions
9b4cf61

Promote warning to exception in get_browser() function
7adf0ce

kocsismate force-pushed the standard-exceptions branch 2 times, most recently from 1ad3b16 to 2e15b92Compare November 22, 2019 13:12

kocsismate added 12 commits November 22, 2019 15:12

Promote warning to exception in dns_check_record() function
b550ea3

Promote warning to exception in convert_uudecode() function
716c677

Promote warnings to exceptions in settype() function
d2d9f6f

Promote warrnings to exceptions in chgrp() function
2895a86

Promote warning to exception in constant() function
174cb29

Promote warning to exception in file_get_contents() function
1833afc

Promote warnings to exceptions in proc_open() function
b87ecf3

Capitalize the initial letter of the error message of htmlspecialchar…
b610f2e
…s() function

Promote warning to exception in the levenshtein() function
b2b219c

Promote warnings to exceptions in some URL and file related functions
474bbc6

Promote warnings to exceptions in string search related functions
bc6bb44

Promote warnings to exceptions in *scanf() functions
49b87c0

kocsismate added 3 commits November 22, 2019 15:12

Promote warning to exception in the convert_uudecode() function
5025c88

Promote warnings to exceptions in register_shutdown_handler() function
023f629

Promote warnings to exceptions in register_tick_function() function
ddd8362

kocsismate force-pushed the standard-exceptions branch from 2e15b92 to ddd8362Compare November 22, 2019 14:12

cmb69 added Waiting on Review and removed Waiting on Author labels Nov 22, 2019

nikic reviewed Dec 5, 2019
View reviewed changes

Comment threadext/standard/scanf.c

nikic reviewed Dec 5, 2019
View reviewed changes

Comment threadext/standard/uuencode.c

nikic reviewed Dec 5, 2019
View reviewed changes

nikic reviewed Dec 6, 2019
View reviewed changes

Comment threadext/standard/filestat.c

nikic reviewed Dec 6, 2019
View reviewed changes

Comment threadext/standard/filestat.c

kocsismate closed this Dec 6, 2019

This was referenced Dec 9, 2019
Promote warnings to exceptions in *scanf() functions #4993
Closed
Promote warnings to exceptions in password_*() functions #4994
Closed

kocsismate mentioned this pull request Dec 11, 2019
Convert warnings to exceptions in standard lib (reopened) #5004
Closed

kocsismate added Warning promotion and removed Waiting on Review labels Jan 28, 2020

Uh oh!

Conversation

kocsismate commented Nov 20, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Girgias commented Nov 20, 2019

Uh oh!

kocsismate commented Nov 22, 2019

Uh oh!

nikicDec 5, 2019

Choose a reason for hiding this comment

Uh oh!

kocsismateDec 5, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Dec 5, 2019

Uh oh!

nikic commented Dec 5, 2019

Uh oh!

nikicDec 5, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nikicDec 5, 2019

Choose a reason for hiding this comment

Uh oh!

kocsismate commented Dec 5, 2019

Uh oh!

Uh oh!

Uh oh!

nikic commented Dec 6, 2019

Uh oh!

kocsismate commented Dec 6, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Dec 10, 2019

Uh oh!

kocsismate commented Dec 10, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kocsismate commented Nov 20, 2019•
edited
Loading

kocsismateDec 5, 2019•
edited
Loading

kocsismate commented Dec 6, 2019•
edited
Loading

kocsismate commented Dec 10, 2019•
edited
Loading