Removing try-catch from examples#1765
Conversation
Using try catch is irrelevant to examples and echoing errors out unconditionally is a bad practice.
cmb69
left a comment
There was a problem hiding this comment.
Thank you for the PR! Generally, I agree, but in this case which is about error handling, wouldn't removing the try-catch blocks defeat the purpose of these examples?
@kamil-tekiela, thoughts?
|
First of all, I must admit that entire article is not very helpful and doesn't explain the error handling as it should. Rather, currently it's just an extended description of error reporting constants/modes. Regarding examples, for the second one the answer is definitely not: the intended output will be exactly the same, as the example is focused on the Warning and not on Exception. The only objection against removing try-catch is rather a potential leak of database credentials. This is actually the point that is very desirable to have explained in the article. Still, if we are going into such details, I would prefer something like $ignore_args = ini_get('zend.exception_ignore_args');
ini_set('zend.exception_ignore_args', 1);
$dbh = new PDO($dsn, $user, $password);
ini_set('zend.exception_ignore_args', $ignore_args);As it will keep the Exception but remove the credentials. But it's questionable still because the main purpose of the error message is to help us with debugging and the actual credentials used could be the key in pinpointing the problem. And in a few months a code snippet that implements https://wiki.php.net/rfc/redact_parameters_in_back_traces can be added as well. Without going into such details I'll prefer to just remove the try-catch, because catching exception only to echo it out is just a bad practice that shouldn't be endorsed by PHP manual. Echoing errors is bad by itself; and early catch prevent us from catching the Exception elsewhere. Basically we are ruining all the benefits the Exception gives us, effectively writing a code similar to that old |
|
I will review it once I get back to the computer in two weeks.
The idea is not to show best practices but to explain what effect different
pdo error modes have. Echoing the error message is stupid and useless but
it could be removed and only the try-catch left. It's just to show that
it's a PDOException and nothing else
…On Mon, Aug 22, 2022, 12:59 colshrapnel ***@***.***> wrote:
First of all, I must admit that entire article is not very helpful and
doesn't explain the error handling as it should. Rather, currently it's
just an extended description of error reporting constants/modes.
Regarding examples, for the second one the answer is definitely not: the
intended output will be exactly the same, as the example is focused on the
Warning and not on Exception.
For the first example it depends, but still I am inclined to think that
without try-catch it's better than with.
The only objection against removing try-catch is rather a potential leak
of database credentials. This is actually the point that is very desirable
to have explained in the article. Still, if we are going into such details,
I would prefer something like
$ignore_args = ini_get('zend.exception_ignore_args');
ini_set('zend.exception_ignore_args', 1);$dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
ini_set('zend.exception_ignore_args', $ignore_args);
As it will keep the Exception but remove the credentials. But it's
questionable still because the main purpose of the error message is to help
us with debugging and the actual credentials used could be the key in
pinpointing the problem.
And in a few months a code snippet that implements
https://wiki.php.net/rfc/redact_parameters_in_back_traces can be added as
well.
Without going into such details I'll prefer to just remove the try-catch,
because catching exception only to echo it out is just a bad practice that
shouldn't be endorsed by PHP manual. Echoing errors is bad by itself; and
early catch prevent us from catching the Exception elsewhere. Basically we
are ruining all the benefits the Exception gives us, effectively writing a
code similar to that old if(!$con) die(mysql_error()) approach.
—
Reply to this email directly, view it on GitHub
<#1765 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSHGGDD2VWSY4IM6ETTQG3V2NFQTANCNFSM57GQWVQA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I added another example that better demonstrates this point. |
|
On the bright side, this PR doesn't change any text, so if accepted, it won't require any translations to be made. In a nutshell, it just displays the different outcome for the different modes. While actual handling is left for the user and can be learned from the article already linked in the text. |
kamil-tekiela
left a comment
There was a problem hiding this comment.
Thanks. I completely forgot about this PR. It looks good. I am sure there could be other things that could be added or improved on this page.
Code examples still need to be adapted by the translators. |
Using try catch is irrelevant to examples and echoing errors out unconditionally is a bad practice.
Using try catch is irrelevant to examples and echoing errors out unconditionally is a bad practice.
Using try catch is irrelevant to examples and echoing errors out unconditionally is a bad practice.