RPP: Added error states #7739
Open
RPP: Added error states #7739
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 1.44 MB ℹ️ View Unchanged
|
jessy-p
reviewed
Nov 20, 2023
| * | ||
| * @return bool True if the error should be logged, otherwise false. | ||
| */ | ||
| protected function should_log_error(): bool { |
There was a problem hiding this comment.
Shouldn't all errors be logged by default?
There was a problem hiding this comment.
I think they need to, but I wasn't 100% certain, so I made it to false. We can easily set that all error states needs to be logged, by the default.
jessy-p
reviewed
Nov 20, 2023
I have updated the test instructions and added a small description to each error state. |
RadoslavGeorgiev
requested changes
Nov 23, 2023
There was a problem hiding this comment.
I have not reviewed all the details regarding the PR, but I think we need a significant change in direction. Keep this line from the issue in mind:
The new payment process should handle exceptions at a more granular level, and switch to a failure state immediately.
The current implementation in the PR goes against granularity, as we're back to the same state as the current payment process, where all exceptions are caught in the same place.
A few notes:
StateTransitionExceptions were a placeholder for exceptions in specific scenarios. We didn't want to create too many separate exception classes, in case they needed to be replaced by this stream. However, all but thecreate_state()'sStateTransitionExceptions should be replaced by something else.- Please merge
developinto the PR, there are some more exceptions after Dat merged some PRs.
Presently we're not storing the context anywhere, but keep this question in mind: If we were to store payments, how would we approach erroneous states when we need to retry those payments?
Comment on lines
+40
to
+45
| /** | ||
| * Stores exception that is triggered before error state is created. | ||
| * | ||
| * @var \Exception | ||
| */ | ||
| private $exception; |
There was a problem hiding this comment.
Like request classes, there is no need to define a property of the object: It's all stored within data. With that in mind, this one is not used, and can be removed.
| * | ||
| * @param \Exception $exception Exception to store. | ||
| */ | ||
| public function set_exception( $exception ) { |
There was a problem hiding this comment.
I'm not insisting on this, but I'd prefer to call those set_last_exception and get_last_exception, similarly to how there's a $wpdb->last_error.
Also, please add a proper type hint to the method:
Suggested change
| public function set_exception( $exception ) { | |
| public function set_exception( Exception $exception ) { |
Similar,y get_(last_)exception() should return : ?Exception
Comment on lines
+133
to
+138
| /** | ||
| * Payments are based on intents, and intents use customer objects for billing details. | ||
| * | ||
| * The customer is created/updated right before requesting the creation of | ||
| * a payment intent, and the two actions must be adjacent to each-other. | ||
| */ |
There was a problem hiding this comment.
This docblock is completely out of place now.
| return $next_state->complete_processing(); | ||
| } catch ( Amount_Too_Small_Exception $e ) { | ||
| $this->minimum_amount_service->store_amount_from_exception( $e ); | ||
| return $this->create_error_state( SystemErrorState::class, $e ); |
There was a problem hiding this comment.
The amount being too small is not a system exception, it happens whenever the amount to pay is less than the required minimum. This is not an indication that there's something wrong with the system, just a payment error.
|
|
||
| return $next_state->complete_processing(); | ||
| return $next_state->complete_processing(); |
There was a problem hiding this comment.
By positioning this line within the catch block of this state, we're essentially trying to handle all exceptions from the ProcessedState here as well. This goes against the idea of granularly handling exceptions.
Comment on lines
+74
to
+81
| /** | ||
| * Determines whether an error should be logged. | ||
| * | ||
| * @return bool True if the error should be logged, otherwise false. | ||
| */ | ||
| protected function should_log_error(): bool { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Logging of the whole context is already built, and specific errors (ex. API errors) are logged already. I'd remove the logging functionality from erroneous states altogether.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7425
Changes proposed in this Pull Request
This PR introduces error states and their management. It ensures that all exceptions, including minor ones, are effectively handled. The core principle is to avoid returning exceptions from states. Instead, states should consistently return their designated state. This approach eliminates the need for exception handling in services utilizing these states or in the gateway, where they were previously handled.
Error states:
AbstractPaymentErrorState: Abstract error class that holds common or reusable logic for all error statesPaymentErrorState: Error state that is used when an error occurs on the buyer's side, which cannot be fixed through code.PaymentRequestErrorState: HandlesPaymentRequestExceptionthat can occur when context is populated from the request.SystemErrorState: Equivalent of \Exception we have in PHP but used for our state machine. Look at this state as a default error state when an exception or any unwanted flow occurs, where we don't have a specific error state to transition.WooPaymentsApiServerErrorState: Error state that is used when server API throws an exception. I used WooPayments prefix to make it more clearer that this is the error state for API server errors on WooCommerce Payments server.Testing instructions
StateTransitionExceptionwhich can occur outside state processing or within the catch block, where we transition from state to new error state.npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge