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

PHP: Handle request errors in PHPRequestHandler, return response code 500 #1249

Merged
merged 10 commits into from
Apr 17, 2024

Conversation

kozer
Copy link
Contributor

@kozer kozer commented Apr 16, 2024

What is this PR doing?

Moves the handling of PHP fatal errors from a service worker message listener, where only the in-browser Playground can use it, to PHPRequestHandler, that allows all the Playground apps like Studio and wp-now to benefit.

In addition, this PR ensures that HTTP 500 Internal Server Error is returned if PHP exits with a non-zero code. This is required because PHP itself seems to happily return HTTP 200 in those cases.

What problem is it solving?

Studio app is showing error logs and is hung under certain scenarios. This pr fixes that.

How is the problem addressed?

By catching the error, and returning the error response.

Testing Instructions

The easiest way I found to test it was via Studio, where the problem appeared originally.

  • Clone the Studio app
  • Change package.json and replace @php-wasm references with the following:
		"@php-wasm/util": "file:../wordpress-playground/dist/packages/php-wasm/util",
		"@php-wasm/node-polyfills": "file:../wordpress-playground/dist/packages/php-wasm/node-polyfills",
		"@php-wasm/universal": "file:../wordpress-playground/dist/packages/php-wasm/universal",
  • Change webpack.main.config.ts and replace the externals object with the following:
externals: {
		'@php-wasm/node': '@php-wasm/node',
		'@php-wasm/universal': '@php-wasm/universal',
		'@php-wasm/util': '@php-wasm/util',
		'@php-wasm/node-polyfills': '@php-wasm/node-polyfills',
	},
  • Clone this pr.
  • Run npm i
  • Run npx nx reset && npm run build
  • Back to the Studio repo, run npm i
  • Create a new site.
  • Install WooCommerce
  • Reload the wp-admin
  • Ensure that the dashboard is showing as expected instead of hanging.

Run tests manually

1 From the root of the project run: npx --expose-gc nx run php-wasm-node:test --testFile ./packages/php-wasm/node/src/test/php-request-handler.spec.ts --skipNxCache

@kozer kozer self-assigned this Apr 16, 2024
@kozer
Copy link
Contributor Author

kozer commented Apr 16, 2024

NOTE:
The above use case solves the issue, however, I am not sure if this is the way to go. Let me explain why.
Although woo, produces a response with httpsStatusCode that is 500, and exitCode 255, it's not reflected in the network tab. The network tab shows the status 200 in all its responses.
Note that if we produce another critical error ( eg die in wp-config.php ), we immediately display an error screen, and I get status 500.
Another thing to note is that while I was testing this, I saw some PHPResponse objects that had httpStatusCode of 200, but exitCode of 255, which seems strange.

@kozer kozer marked this pull request as draft April 16, 2024 09:43
@adamziel
Copy link
Collaborator

This will work as expected once #1252 lands and base-php.ts is updated as follows (PHP doesn’t seem to set the status code to 500 on failure):

		return new PHPResponse(
			exitCode === 0 ? httpStatusCode : 500,

@kozer
Copy link
Contributor Author

kozer commented Apr 16, 2024

Exported a new interface and added the change @adamziel suggested.
Waiting for #1251

adamziel added a commit that referenced this pull request Apr 16, 2024
Ensures PHP returns the correct exit code after each request. Without
this PR, hitting an exit code 255 once means PHP will continue returning
it indefinitely.

Related to #1249

## Testing instructions

Confirm the unit tests pass (they won't until all PHP versions are
rebuilt and shipped with this PR)

cc @kozer

---------

Co-authored-by: Brandon Payton <brandon@happycode.net>
@kozer kozer marked this pull request as ready for review April 17, 2024 08:59
@kozer
Copy link
Contributor Author

kozer commented Apr 17, 2024

Updated the PR , and added some tests

@kozer kozer requested review from adamziel, sejas and wojtekn April 17, 2024 08:59
@adamziel adamziel changed the title Fix: handle request error gracefully PHP: Handle request errors in PHPRequestHandler, return response code 500 Apr 17, 2024
@adamziel adamziel merged commit db505d2 into WordPress:trunk Apr 17, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants