Skip to content

Removing try-catch from examples#1765

Merged
kamil-tekiela merged 2 commits intophp:masterfrom
colshrapnel:patch-3
Sep 23, 2022
Merged

Removing try-catch from examples#1765
kamil-tekiela merged 2 commits intophp:masterfrom
colshrapnel:patch-3

Conversation

@colshrapnel
Copy link
Copy Markdown
Contributor

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.
Copy link
Copy Markdown
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@colshrapnel
Copy link
Copy Markdown
Contributor Author

colshrapnel commented Aug 22, 2022

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 = 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 if(!$con) die(mysql_error()) approach.

@kamil-tekiela
Copy link
Copy Markdown
Member

kamil-tekiela commented Aug 22, 2022 via email

@colshrapnel
Copy link
Copy Markdown
Contributor Author

to explain what effect different pdo error modes have.

I added another example that better demonstrates this point.
While specific information about handling PHP Exceptions in general can be found on the relevant page linked above in the text.

@colshrapnel
Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Member

@kamil-tekiela kamil-tekiela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kamil-tekiela kamil-tekiela merged commit 2310448 into php:master Sep 23, 2022
@cmb69
Copy link
Copy Markdown
Member

cmb69 commented Sep 23, 2022

On the bright side, this PR doesn't change any text, so if accepted, it won't require any translations to be made.

Code examples still need to be adapted by the translators.

tiffany-taylor pushed a commit to tiffany-taylor/doc-en that referenced this pull request Jan 16, 2023
Using try catch is irrelevant to examples and echoing errors out unconditionally is a bad practice.
claudepache pushed a commit to claudepache/php-doc-en that referenced this pull request Jun 1, 2023
Using try catch is irrelevant to examples and echoing errors out unconditionally is a bad practice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants