-
-
Notifications
You must be signed in to change notification settings - Fork 17
Add solution providers #127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
xepozz
commented
Jul 18, 2024
Q | A |
---|---|
Is bugfix? | ❌ |
New feature? | ✔️ |
Breaks BC? | ❌ |
Fixed issues | #108 |
Looks cool! But I suggest do it in separate package. |
All the rest implementation, but it's base support for all of them |
@xepozz what's missing here? |
I think I lost some files in the original codebase, some of them already moved to bridge for error handling 😁 |
Possible to update it? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #127 +/- ##
============================================
- Coverage 80.55% 79.76% -0.80%
- Complexity 210 213 +3
============================================
Files 19 20 +1
Lines 679 687 +8
============================================
+ Hits 547 548 +1
- Misses 132 139 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Need a phpdoc for the interface + README + CHANGELOG. Would you please handle these?
@xepozz would you please address static analysis issues? https://github.com/yiisoft/error-handler/actions/runs/16853615512/job/47743450768?pr=127 |
namespace Yiisoft\ErrorHandler\Solution; | ||
|
||
/** | ||
* The interface declares an adapter to render a solution for an event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* The interface declares an adapter to render a solution for an event. | |
* Solution provider finds and renders a solution for a given throwable. |
|
||
/** | ||
* The interface declares an adapter to render a solution for an event. | ||
* Basically, it renders the error message as-is, but possible could render a button with click-to-fix action that will be handled by an HTTP request (with middleware) back to server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Basically, it renders the error message as-is, but possible could render a button with click-to-fix action that will be handled by an HTTP request (with middleware) back to server. | |
* It may render the message as-is, but it could also provide a button with a click-to-fix action that will be handled by an async HTTP request (using a separate middleware). |
|
||
use Yiisoft\FriendlyException\FriendlyExceptionInterface; | ||
|
||
final class FriendlyExceptionSolution implements SolutionProviderInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final class FriendlyExceptionSolution implements SolutionProviderInterface | |
/** | |
* Renders a friendly exception. | |
*/ | |
final class FriendlyExceptionSolution implements SolutionProviderInterface |
interface SolutionProviderInterface | ||
{ | ||
/** | ||
* Returns true if the implementation may suggest more than regular provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Returns true if the implementation may suggest more than regular provider. | |
* Returns true if solution could be provided for a given throwable. |
public function supports(\Throwable $e): bool; | ||
|
||
/** | ||
* Generates an HTML code with solution which will be clean by {@see \Yiisoft\ErrorHandler\Renderer\HtmlRenderer} and shown to the end user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "clean by"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HtmlRenderer has HTML purifier inside, it cleans all the given html tags and attributes except that are in configuration
* The interface declares an adapter to render a solution for an event. | ||
* Basically, it renders the error message as-is, but possible could render a button with click-to-fix action that will be handled by an HTTP request (with middleware) back to server. | ||
*/ | ||
interface SolutionProviderInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SolutionProviderInterface
must be placed into separate package. yiisoft/error-handler
is too heavy dependency for packages, that implement SolutionProviderInterface
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not about any solution in the air. It's about the specific contract between web, user, application and error.
It shouldn't be separated. At least for now.
* The interface declares an adapter to render a solution for an event. | ||
* Basically, it renders the error message as-is, but possible could render a button with click-to-fix action that will be handled by an HTTP request (with middleware) back to server. | ||
*/ | ||
interface SolutionProviderInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interface should define format of solution. Users of interface should know about format of solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML for sure
@vjik noted that HTML doesn't suit console handler well. That is true. |
SolutionProvider's are in use only in the html renderer. |