Skip to content
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.

Sign up for GitHub

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

Handle 402 errors so it won't be hijacked by the hosting provider error page #8102

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Handle 402 errors so it won't be hijacked by the hosting provider error page #8102

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d4a8d3
Handled 402 errors error response
zmaglica Jan 30, 2024
2ebe9d1
Added changelog
zmaglica Jan 30, 2024
ffb36ec
Code review fixes
zmaglica Feb 8, 2024
5bb9431
Merge branch 'develop' into fix/8080-handle-402-errors
zmaglica Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/fix-8080-handle-402-errors
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fix

Fixed parsing of 402 status codes.
10 changes: 8 additions & 2 deletions includes/class-wc-payments-utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,10 +591,16 @@ public static function get_filtered_error_message( Exception $e ) {
* @return int
*/
public static function get_filtered_error_status_code( Exception $e ) : int {
$status_code = 400;
if ( $e instanceof API_Exception ) {
return $e->get_http_code() ?? 400;
$status_code = $e->get_http_code() ?? 400;
// Sometimes hosting companies hijack this status code and return their own predefined error page.
// In this case, we want to return a 400 instead, since it will break the flow of the checkout page.
if ( 402 === $status_code ) {
$status_code = 400;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@zmaglica do you plan to add a test for this change? As I checked the test file does exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zmaglica do you plan to add a test for this change? As I checked the test file does exist.

I am not planning to write tests in this PR for this file, since the tests for this particular file don't exist, as you mentioned too. I will keep this PR as clean as possible and open a new issue to address the issue with the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zmaglica I meant the test file, which is this, does exist :))

IMO, it is always a good idea to write test in the same PR.

}
return 400;
return $status_code;
Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts on the following alternative, to decrease the indentation due to the cyclomatic complexity?

		$status_code = null;
		if ( $e instanceof API_Exception ) {
			$status_code = $e->get_http_code();
		}

		// Sometimes hosting companies hijack the 402 status code to return their own predefined error page.
		// In this case, we want to return a 400 instead, since it will break the flow of the checkout page.
		if ( 402 === $status_code ) {
			$status_code = 400;
		}

		return $status_code ?? 400;

Copy link
Contributor

Choose a reason for hiding this comment

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

@frosso's suggestion is easier to read 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed here: ffb36ec . Thank you for the feedback.

}

/**
Expand Down
Loading