Promote warnings to exceptions in the standard lib #4937

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.

ext/standard/password.c

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.

ext/standard/string.c

ext/standard/tests/general_functions/bug72920.phpt

ext/standard/tests/general_functions/gettype_settype_error.phpt

ext/standard/tests/general_functions/proc_open_pipes3.phpt

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

ext/standard/basic_functions.stub.php

@@ -361,7 +361,7 @@ function is_uploaded_file(string $path): bool {}

 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

ext/standard/scanf.c

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

ext/standard/scanf.c

ext/standard/uuencode.c

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

ext/standard/basic_functions.c

@@ -3202,7 +3202,7 @@ static int user_shutdown_function_call(zval *zv) /* {{{ */
 	if (!zend_is_callable(&shutdown_function_entry->arguments[0], 0, NULL)) {
 		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().

ext/standard/filestat.c

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: 5898e8e ad4d663 04deb53 d2868ed

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 19a2cc3 Compare November 20, 2019 13:50

Girgias reviewed Nov 20, 2019

View reviewed changes

ext/standard/password.c Show resolved Hide resolved

Girgias requested changes Nov 20, 2019

View reviewed changes

cmb69 added the Waiting on Author label Nov 21, 2019

kocsismate force-pushed the standard-exceptions branch 5 times, most recently from 5611f2b to b0de09a Compare 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 2e15b92 Compare 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 ddd8362 Compare 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

ext/standard/scanf.c Show resolved Hide resolved

nikic reviewed Dec 5, 2019

View reviewed changes

ext/standard/uuencode.c Show resolved Hide resolved

nikic reviewed Dec 5, 2019

View reviewed changes

nikic reviewed Dec 6, 2019

View reviewed changes

ext/standard/filestat.c Show resolved Hide resolved

nikic reviewed Dec 6, 2019

View reviewed changes

ext/standard/filestat.c Show resolved Hide resolved

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

Promote warnings to exceptions in the standard lib #4937

Promote warnings to exceptions in the standard lib #4937

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!

nikic Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

kocsismate Dec 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!

nikic Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nikic Dec 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!

Uh oh!

kocsismate commented Nov 20, 2019 •

edited

Loading

kocsismate Dec 5, 2019 •

edited

Loading

kocsismate commented Dec 6, 2019 •

edited

Loading

kocsismate commented Dec 10, 2019 •

edited

Loading