Refactor error/exception pages into an "Error handling" page#320
Refactor error/exception pages into an "Error handling" page#320Girgias wants to merge 14 commits intophp:masterfrom
Conversation
50e9946 to
116241a
Compare
8da7560 to
995807e
Compare
|
I think this is now ready for review, I hope I didn't remove any information by splitting it up. |
| <warning> | ||
| <para> | ||
| <classname>Throwable</classname> objects cannot be cloned. | ||
| Attempting to <link linkend="language.oop5.cloning">clone</link> such an | ||
| object will result in an <classname>Error</classname> being thrown with | ||
| the following message: | ||
| <literal>Trying to clone an uncloneable object of class Exception</literal>. | ||
| </para> | ||
| </warning> |
There was a problem hiding this comment.
I wonder if this warning is relevant here? As it would be pretty unusual to clone an exception, and this behaviour is described on their respective docs.
There was a problem hiding this comment.
I agree that this is a generic behavior that is already described in a central location. I see no reason to mention that in every place where it applies. Should the user see such an error, they will be able to find the correct docs through a search.
9c7354a to
3bee578
Compare
| </example> | ||
|
|
||
| <para> | ||
| When the call stack is unwound after a <classname>Throwable</classname> error |
There was a problem hiding this comment.
I think trying to reword this in a way that's more beginner-friendly would be nice. I don't think call stack unwinding is mentioned elsewhere in the docs.
Maybe along the lines of "If there are nested finally blocks, they will be executed in the order they are declared"
There was a problem hiding this comment.
I'll keep the technical language (as this is copied from the previous document) but will attempt to add a simpler explanation along side it.
| <sect1 xml:id="language.error-handling.throwable"> | ||
| <title><classname>Throwable</classname> errors</title> | ||
| <para> | ||
| Most errors in PHP are of this type. |
There was a problem hiding this comment.
This statement is highly version-specific (without using set_error_handler, at least).
There was a problem hiding this comment.
I can add an As of PHP 8, before it, but even in PHP 7 many of the warnings generated by the Engine are catchable Errors
| <programlisting role="php"> | ||
| <![CDATA[ | ||
| <?php | ||
| function exceptions_error_handler($severity, $message, $filename, $lineno) { |
There was a problem hiding this comment.
I'd argue that doing this is considered best practices by many people. It may be worth calling more attention to this, or adding a best practices section to this document.
There was a problem hiding this comment.
Not sure the documentation should decide what are considered best practices, I'll try to emphasis this more however. :-)
3bee578 to
f276137
Compare
| <sect1 xml:id="control-structures.finally" xmlns="http://docbook.org/ns/docbook" xmlns:xlink="http://www.w3.org/1999/xlink"> | ||
| <title>finally</title> | ||
| <?phpdoc print-version-for="finally"?> | ||
| <para> |
There was a problem hiding this comment.
I feel like it would be better to front-load the logic of how finally behaves vis a vis throws, returns, yields, exit, etc., perhaps with a bulleted list, so it's easier to reference. Then proceed with a bunch of examples demonstrating it.
|
Various small issues noted above. My biggest concern is that this puts exceptions under flow-of-control... but exceptions, as we are told repeatedly, are not meant to be used for flow of control. Yet, they do change the flow of control. My concern is that, without any statement to the contrary, this could tacitly imply that using exceptions the same way you would use if, foreach, or goto is a-OK, when that's very much not the case. Otherwise, everything else here seems reasonable. |
This renames traditional errors to diagnostic errors Create a page about error handling in PHP describing how to handle Throwable Errors (Exceptions) and diagnostics. > Inspired from the Rust docs. Create dedicated pages for: - throw - try-catch - finally
Usage without it is explained in the following section
Also fix part of the set_error_handler() docs
f276137 to
0b089b1
Compare
Co-authored-by: George Peter Banyard <girgias@php.net>
|
@Girgias I resolved the outdated comments and provided my feedback on your own questions. I hope this helps to move this PR forward. I also applied the code suggestion with the syntax fix. |
|
Thanks, I'll try to get back to this, but I need to sort out HTTP redirection for php-web and rebase, I should have time again in September. :-) |
|
This branch is getting old and has many merge conflicts. Suggestions:
|
|
@Girgias Is it worth to keep this PR open? This looks like a slice that is too big to be feasible. It can be referred to in a ticket (todo) instead, so that you don't lose sight of some of the wording that might be reusable. |
I will try to work on this PR this months as I'm planning to start working on the Migration guide and do more doc works this month (as I also won't have access to my desktop with 24 cores which makes php-src work more annoying) |
|
This PR hasn't moved in years. Would it make sense to open a new PR for just the added pages so it can be merged quickly, then work on changing existing pages over time, whenever people have time? |
Yes, reviewing smaller PRs is much easier. There seems to be very little reviewers available anyway, so the easier it is the better. |
This renames traditional errors to diagnostic errors
Attempts to unify
ThrowableErrors with ExceptionsMove
throw,try,catch, andfinallycontrol structures to the control structure sectionFeedback on this is very welcome and I'm open to suggestion, as some of the text still needs to be updated/improved.
This is partly based on how Rust talks about error handling.
Some ideas I have also been considering:
error-handling.xmlinto two files in a new subfoldererror-handling/A to-do list of sort:
throwMemo of things to do when this lands: