HenryCan Posted May 21, 2017 Share Posted May 21, 2017 I've just ported a system I wrote a few years back into PHP into a new environment that has PHP 7.1.4 instead of PHP 5.x. My programs used to work perfectly but are now throwing errors, many of which I've never seen before.I am not a fluent PHP programmer although I have programmed in a variety of languages, including Java, so I'm not new to programming by any means.I was reading a short tutorial about PHP error handling and they suggested writing an error-handling function and invoking it as follows: //Error handler function function customError($errno, $errstr, $errfile, $errline, $errcontext) { echo "<b>Error:</b> [$errno] $errstr<br>"; echo "<b>File/Line:</b> $errfile/$errline<br>"; //echo "<b>Error context:</b><br>"; //print_r($errcontext); echo "....Ending Program"; die(); } //Set error handler set_error_handler("customError",E_ALL); I did that and got this: Error: [8] Undefined index: report_layout File/Line: /storage/h13/139/1556139/public_html/SideTrekkedContributions.php/28 In the first line of that, am I correct in assuming that the [8] denotes the severity level of the error, specifically E_NOTICE, which is defined as "Run-time notices. The script found something that might be an error, but could also happen when running a script normally."? Or is the 8 uniquely associated with an "Undefined index" error in a world where every distinct error has a specific number associated with it, like 37 for division by zero and 412 for array index out of bounds? I think it must be the former and that there will be all kinds of errors that have the number 8 but my knowledge of PHP is sketchy so I want to be sure.Now, assuming I'm right and that 8 is the severity of the error, what should one normally do with E_NOTICE errors? Ideally, I'd like PHP not to display this message and I'd like to get rid of the message by doing whatever I need to do to prevent PHP from thinking there's anything wrong (as opposed to telling it not to show me errors below a certain severity). In this particular case, the line of code identified as the source of the error is this: switch($_POST['report_layout']) { I'm darned if I can see what's wrong with this. I never got an error of any kind about this code on the old system. But maybe the old system wanted to show me messages like this but had minor errors suppressed; I really don't know. I tried initializing that variable to null $report_layout = null; but that did NOT keep the error from appearing. I'm at a loss how to satisfy PHP that the variable has been appropriately initialized so it doesn't have to tell me that there's an undefined index.Can anyone enlighten me on the points I've raised? Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted May 21, 2017 Share Posted May 21, 2017 (edited) As explained in the manual, the first parameter of the error handler is for the error type, and if you look at the example code on the same page, you'll see several different types: E_USER_ERROR, E_USER_WARNING, E_USER_NOTICE. So, yes, the type is generic. What should you do with notices? Fix the code. Like in any other language, PHP scripts should be as correct and robust as possible. While it may be tempting to ignore notices out of laziness and hope that they aren't important, this is a bad idea. You'll quickly miss actual errors, you make debugging much harder, and you rely on obscure behavior. Yes, PHP will do something, but it won't necessarily do what you expect, which can lead to very nasty defects. In your specific case, you should validate the user input instead of just assuming that all expected fields are present. If a required field is missing, inform the user and stop the script. If an optional field is missing, set a reasonable default value. In any case, you have to perform an isset() check before accessing $_POST parameters. You cannot fix the problem by initializing some other variable. <?php const HTTP_CODE_BAD_REQUEST = 400; $errors = []; // first check: Did we even receive a POST request? if ($_SERVER['REQUEST_METHOD'] == 'POST') { // check if all required fields are present $required_fields = ['report_layout']; foreach ($required_fields as $field) { if (!isset($_POST[$field])) { $errors[] = 'Missing required field: '.$field; } } // only continue if there are no errors, otherwise set an appropriate error code and display the error messages if (!$errors) { // now you can be sure the $_POST['report_layout'] actually exists } else { http_response_code(HTTP_CODE_BAD_REQUEST); } } ?> <!DOCTYPE html> <html lang="en"> <head> <meta charset="utf-8"> <title>Title</title> </head> <body> <?php if ($errors): ?> The following errors occured: <ul> <?php foreach ($errors as $error): ?> <li><?= htmlspecialchars($error, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') ?></li> <?php endforeach; ?> </ul> <?php endif; ?> </body> </html> Setting up a custom error handler only to display the error message is nonsensical, even though it's somehow very, very popular in the PHP community. PHP already has a perfectly fine standard error handler. Just enable display_errors (during development) or log_errors (in production), and you'll get detailed error messages. There's no need to reinvent that. Custom handlers can only catch a handful of errors which makes it rather useless. Printing PHP errors on a live website is insane. Not only will this leak internal information about your system. It's also very irritating for legitimate users and makes the site look unprofessional. Edited May 21, 2017 by Jacques1 1 Quote Link to comment Share on other sites More sharing options...
HenryCan Posted May 22, 2017 Author Share Posted May 22, 2017 As explained in the manual, the first parameter of the error handler is for the error type, and if you look at the example code on the same page, you'll see several different types: E_USER_ERROR, E_USER_WARNING, E_USER_NOTICE. So, yes, the type is generic. Thank you for confirming my suspicion that the 8 was, in effect, a severity level that would apply to multiple messages, not a specific error. [sorry for changing colour unexpectedly but I don't quite see how to reply to your remarks point by point and make my remarks easily distinguishable from yours otherwise.] What should you do with notices? Fix the code. Like in any other language, PHP scripts should be as correct and robust as possible. While it may be tempting to ignore notices out of laziness and hope that they aren't important, this is a bad idea. You'll quickly miss actual errors, you make debugging much harder, and you rely on obscure behavior. Yes, PHP will do something, but it won't necessarily do what you expect, which can lead to very nasty defects. You're preaching to the choir. I believe I already mentioned that I was trying to prevent PHP from having any reason to generate messages, as opposed to simply suppressing them by the mechanisms PHP has to do exactly that. Speaking of which, I discovered one of those mechanisms when I got into the .htaccess file my hosting provider automatically generated when I created my website. It had a line that said: php_value display_errors 1 As soon as I changed that 1 to 0, the various messages stopped. But, as I said, I'm not trying to suppress the messages, I'm trying to prevent PHP from generating them by writing good code. I just don't know how to do that really well yet. In your specific case, you should validate the user input instead of just assuming that all expected fields are present. If a required field is missing, inform the user and stop the script. If an optional field is missing, set a reasonable default value. In any case, you have to perform an isset() check before accessing $_POST parameters. You cannot fix the problem by initializing some other variable. <?php const HTTP_CODE_BAD_REQUEST = 400; $errors = []; // first check: Did we even receive a POST request? if ($_SERVER['REQUEST_METHOD'] == 'POST') { // check if all required fields are present $required_fields = ['report_layout']; foreach ($required_fields as $field) { if (!isset($_POST[$field])) { $errors[] = 'Missing required field: '.$field; } } // only continue if there are no errors, otherwise set an appropriate error code and display the error messages if (!$errors) { // now you can be sure the $_POST['report_layout'] actually exists } else { http_response_code(HTTP_CODE_BAD_REQUEST); } } ?> <!DOCTYPE html> <html lang="en"> <head> <meta charset="utf-8"> <title>Title</title> </head> <body> <?php if ($errors): ?> The following errors occured: <ul> <?php foreach ($errors as $error): ?> <li><?= htmlspecialchars($error, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') ?></li> <?php endforeach; ?> </ul> <?php endif; ?> </body> </html> I appreciate the example you've provided but it probably isn't the best answer for my situation because I haven't given you enough of my code to fully appreciate the situation. I'd like to do that now. This code, SideTrekkedContributions.php, offers the user a form which lets them select from three different reports; the form is in a separate file, _SideTrekkedContributionsMenu.shtml, which I show next. The user simply uses the radio button to choose one of the three reports which is possible (or resets the form), then the appropriate report is generated within the switch statement. SideTrekkedContributions.php: <!--#include file="fragments/#alfa.shtml"--> <?php include('fragments/_getThemes.php'); ?> </head> <body> <?php //Display the menu that lets the user decide which report they want include ('fragments/_SideTrekkedContributionsMenu.shtml'); //Execute the program that displays the desired report. switch($_POST['report_layout']) { case 'Format1': include('SideTrekkedContributions1.php'); break; case 'Format2': include('SideTrekkedContributions2.php'); break; case 'Format3': include('SideTrekkedContributions3.php'); break; } ?> </body> </html> _SideTrekkedContributionsMenu.shtml: <h2>SideTrekked Contributions</h2> <form method="post" action="<?php echo $_SERVER['PHP_SELF']; ?>"> <p>You can choose from multiple layouts for the report. Please select the one you prefer, then press the Display Report button. To restore the form to its original appearance without submitting it, press the Reset button.</p> <fieldset><legend>Report Layout</legend> <input type="radio" name="report_layout" value="Format1"/>In publication order<br/> <input type="radio" name="report_layout" value="Format2"/>In order by title<br/> <input type="radio" name="report_layout" value="Format3"/>In order by contributor last name<br/> </fieldset> <p><input name="submitForm" id="submitForm" type="submit" value="Display Report" /> <input name="reset" id="reset" type="reset" value="Reset" /></p> <p><em>It may take a few seconds after you click the Display Report button to gather and format all the data. Please be patient.</em></p> </form> Given that report_layout is defined in _SideTrekkedContributionsMenu.shtml and that the form is included before the first use of report_layout in SideTrekkedContributions.php, which is in the switch statement that raises the error, I'm puzzled by why the error is raised. Doesn't the form instantiate the variable adequately? How should I be doing this differently to avoid the error? Setting up a custom error handler only to display the error message is nonsensical, even though it's somehow very, very popular in the PHP community. PHP already has a perfectly fine standard error handler. Just enable display_errors (during development) or log_errors (in production), and you'll get detailed error messages. There's no need to reinvent that. Custom handlers can only catch a handful of errors which makes it rather useless. Printing PHP errors on a live website is insane. Not only will this leak internal information about your system. It's also very irritating for legitimate users and makes the site look unprofessional. All I can say on that point is that I'm absolutely fine with the idea of NOT using a custom error handler. I looked for tutorials on PHP error handling and both of the first two I found showed you how to do that. I got the impression that this was the preferred way of doing things! I also agree that printing PHP errors on a live website is foolish for the reasons you've stated. In my case, the hosting provider apparently insists on it. (The lines before and after the "php_value display_errors 1" line demands that you not modify or remove the line.) Perhaps they're trying to shame people into cleaning up their code so it doesn't produce errors? In any case, I'm fine with leaving that line in the .htaccess file. It helps ensure that I won't be lazy and leave the code the way it is. I don't want those messages on the pages the users of my site will see. I just need to learn proper error handling techniques and how to write my code well enough that all errors are handled cleanly. That's why I signed up for this forum. Can you point me to some kind of "best practices" guide for error handling? Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted May 22, 2017 Share Posted May 22, 2017 My code fits your situation exactly. In case I haven't made myself clear enough: Do not assume that the request contains the parameters you expect. There are all kinds of reasons why you may receive incomplete requests: server-side defects (which is your current problem), client-side defects, human error etc. Those cases need to be handled in a controlled manner. You stop the script, inform the user about the problem (in plain English), set an appropriate response code and log the error if you think it's your fault. This is crucial for correctness and usability. In your specific case above, there's obviously a misunderstanding of the HTTP workflow. When the page is loaded for the first time, this happens with a GET request, so there are no POST parameters. Any attempt of pulling data out of $_POST will fail. Again my code would have prevented this: By actually checking the request method, you can make sure that the processing code only runs when you've received a POST request. As to proper error handling, I think I've already covered the most relevant aspects: validation, display_errors during development, log_errors in production, user feedback. And if that wasn't clear enough either: You must disable display_errors in production. It doesn't matter what your hoster says, nor is this some kind of incentive for writing better code. Error messages can contain highly critical information, including plaintext passwords for your database server or your user accounts. You definitely do not want that on a public website for the whole world to see. Log the error messages. Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.