1

In https://stackoverflow.com/a/56874892/1491895, @Dharman posted the following comment:

It is a very bad idea to use die(mysqli_error($conn)); in your code, because it could potentially leak sensitive information. See this post for more explanation: mysqli or die, does it have to die?

(This is the second time he's posted that comment to one of my answers. I think I ignored it the first time.)

When I'm answering, I'm trying to address the specific problem in the question, not provide a course on quality software engineering practices. I don't always include error checking code, but when I do, it's usually just simple code like that, so that when the questioner runs into problems trying to apply my answer they'll be able to give useful feedback about the problem.

Am I really expected to go the extra mile and replace simple debugging code with something that would be expected in a real-world application exposed to end users? The vast majority of questions on SO come from students doing exercises, not writing professional code.

10
  • 19
    The vast majority of questions on SO come from students doing exercises, not writing professional code. -- That is a very sad statement to make. Even if it were true, students should aspire to become professionals.
    – Robert Harvey Mod
    Commented Jul 3, 2019 at 18:19
  • 8
    In any case, the author of that comment is not requiring you to change the code in your answer; he's merely making an observation. Your answer can live or die on its own merits.
    – Robert Harvey Mod
    Commented Jul 3, 2019 at 18:20
  • 9
    To follow up with Robert's comment, although the question may come from a student, a professional might run into a similar issue in the line of work and find the question on Stack Overflow, and even a warning "Note that this may leak sensitive information" can save that person from a world of hurt.
    – Davy M
    Commented Jul 3, 2019 at 18:21
  • 11
    Is this really a question about being production ready or should the question be "should we avoid teaching bad habits"? And the answer should be yes. Also who asked the question doesn't matter, you don't answer questions for OP alone, but for everyone reading the question and your answer.
    – Tom
    Commented Jul 3, 2019 at 18:24
  • 9
    i.imgflip.com/34v4ns.jpg
    – Robert Harvey Mod
    Commented Jul 3, 2019 at 18:25
  • 6
    As production ready as you want it to be. but that doesn't make your answer immune from criticism.
    – Kevin B
    Commented Jul 3, 2019 at 18:53
  • 1
    People seem to downvote answers where the question includes code vulnerable to SQL injection code and the answer doesn't tell them to fix the SQL injection. I get it, it's like a helpful PSA. But it is tiresome that you have to include it in an answer instead of addressing what the question is actually about. Commented Jul 3, 2019 at 19:29
  • 6
    @ScottHannen It is tiresome, but if you do not show them how to do it right, they will ask another question with the same mistake. SQL injection is a bug and as such should be fixed or at least pointed out.
    – Dharman Mod
    Commented Jul 3, 2019 at 19:35
  • 1
    @Dharman I often try to address the most important problems, like prepared statements instead of variable substitution, but sometimes this can obscure the answer to the actual question. I prefer to use comments to let them know that there are other issues with their code beside the specific problem they asked about.
    – Barmar
    Commented Jul 3, 2019 at 19:43
  • 1
    @Barmar the only problem with using comments to point out additional errors is that comments are intended to be ephemeral. If you notice a bad practice in a question, while you're answering it, you should include a line or two in your answer about it. For instance, if someone was asking about how to clean a gun, and they mentioned that they started by pointing it at their face, I think it'd be a service to them and the whole community to point out the obvious safety problem at the same time you outline gun-cleaning steps :)
    – HFBrowning
    Commented Jul 3, 2019 at 22:35

2 Answers 2

21

I believe that Stack Overflow is very useful repository of information for students and professionals. When we answer we should strive for best quality, so anyone reading the answer can benefit from it. It does not necessarily need to be production-ready code, but we should strive for correct answers and avoid teaching bad habits.

The reason for my comment on your post is very well explained by @YourCommonSense in the linked post.

So, never use die() with mysql errors, even for the temporary debugging: there are better ways.

Your suggested method for debugging was to insert or die($conn->error) at the end of the line. This will be executed if the prepare function fails due to some errors and it will display the error message to the end-user. This is not error checking, it is error displaying.
You haven't made it clear in your post that this piece of code was added only for debugging purposes. To the OP and whoever else might stumble upon this post, it looks like part of the code logic. They might not know that it is only to display error while coding the logic and that it is there only temporarily.

You are correct saying that I do not put "error checking" in my answers. PHP has functionality of generating errors on its own. If the code fails an error is generated, which will either be logged into error log file or displayed to the user.

error_reporting(E_ALL); // <- enable error generation
ini_set("log_errors", 1); // <- enable error logging to a file
ini_set('display_errors', 1); // <- enable error display
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT); // <- enable errors for MySQLi extension

These settings should be enabled in Development, but display_errors should be switched off in Production.

Taking advantage of PHP's standard error reporting you do not need to use die() to debug your code. If your environment is properly configured you will see the errors. If the standard error reporting is not enough, you can avail of PHP's function to generate user errors called trigger_error which will respect your environment's INI settings.

Killing PHP code using die() is very nasty habit, which we should try to avoid in our answers.

6
  • 1
    If you just use error reporting like that, the script will continue after the error, usually causing additional errors. For instance, if you don't stop after mysqli_connect() fails, you'll get errors because the connection object is null.
    – Barmar
    Commented Jul 3, 2019 at 19:39
  • 3
    If mysqli_connect() fails you should get: Fatal error: Uncaught mysqli_sql_exception: Access denied for.... Once the fatal error is thrown the script will stop execution.
    – Dharman Mod
    Commented Jul 3, 2019 at 19:55
  • @Barmar I don't think I really see anyone telling you not to use die(), I just see them telling you not to combine die() with a function that outputs security-sensitive information.
    – Gimby
    Commented Jul 5, 2019 at 7:57
  • @Gimby Hard to imagine how to debug a program without that information.
    – Barmar
    Commented Jul 5, 2019 at 13:22
  • 1
    @Barmar I don't see what making a page stop rendering has to do with debugging.
    – Gimby
    Commented Jul 5, 2019 at 13:30
  • @Gimby It's the detailed information that helps with debugging. You said that we shouldn't display that information, because it may be security-sensitive. die() is just an easy way to display it, while also preventing the program from continuing with bad data.
    – Barmar
    Commented Jul 5, 2019 at 13:33
2

I think that as long as the provided code is concise and answers the question it is acceptable. However, That doesn't mean that answer is the best answer. If an answer contains an error that doesn't affect its overall validity in context with the question (eg: it works but has a security flaw), it is still a valid answer.

However, if someone notices a security flaw or other problem in an answer, they should at least add a comment to inform future readers of the problem. They could also edit the post with the information, as long as the original meaning of the answer remains in tact (ie: add a note that the answer is insecure and why). However, it may be preferred to add another answer to the question if the change is significant and relevant enough to the question.

In this case, not having production ready code was not wrong, just not the best. I don't think this warrants a second answer because the problem is not relevant enough to the answer. A comment redirecting to a more relevant answer is sufficient in this instance, or possibly an informational edit to the answer.

2
  • 6
    Thank you. I guess I shouldn't consider Dharman's comment to be a criticism, but more like helpful additional info.
    – Barmar
    Commented Jul 3, 2019 at 20:42
  • 1
    I sure hope you end up never being a coworker of mine if you don't care at all about major security vulnerabilities in code you're working on or suggesting for others. That's just cruel.
    – Servy
    Commented Jul 3, 2019 at 21:02

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .