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.

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
Changes from 1 commit
Commits
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
Next Next commit
Handled 402 errors error response
  • Loading branch information
zmaglica committed Jan 30, 2024
commit 0d4a8d3acb0e25fdcf79068eb8b816d204a32ff1
10 changes: 8 additions & 2 deletions includes/class-wc-payments-utils.php
Original file line number Diff line number Diff line change
@@ -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.

}

/**