Update: Simplify base-php sapi_handle_request call #387
Conversation
This file contains 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
Merged
danielbachhuber
approved these changes
May 18, 2023
kozer
added a commit
that referenced
this pull request
May 18, 2023
<!-- Thanks for contributing to WordPress Playground! --> ## What? This pr adds a 404 error if the requested path doesn't exist <!-- In a few words, what is the PR actually doing? Include screenshots or screencasts if applicable --> We decided to split #331 into two parts, to make changes clear. Part 2 of #331 Part 1 is #387 ## Why? When the user uses a path that doesn't have any files, the `wp-now` was throwing an error, and was no longer usable. This pr fixes that. Also, take a look at #331 <!-- Why is this PR necessary? What problem is it solving? Reference any existing previous issue(s) or PR(s), but please add a short summary here, too --> ## How? When we call `php.run` method, we build the `scriptPath` using `resolvePHPFilePath`. If the file is not there we assume that an `/index.php` file exists in the folder. This is not correct, and we need to return a 404 `PhpRequest`. Also, take a look at #331 <!-- How is your PR addressing the issue at hand? What are the implementation details? --> ## Testing Instructions - Create an empty folder - Run wp-now and make sure you add this folder as a path: npx nx preview wp-now start --path=<your-empty-folder> - Ensure that you don't see any errors in the console - Ensure that you see 404 File not found in the browser - Change the URL to something else. eg: /file-not-exist.php. - Ensure that you still see 404 File not found
|
@danielbachhuber the commit message for this one wasn't quite there. I re-applied it with a small writeup to explain the entire context behind the change: I know I'm pushing hard on this one. This is the most important code path in the entire codebase and I want to make sure the change history is extremely clear for the next person debugging it. |
|
@adamziel Cool, sounds good. |
Pookie717
added a commit
to Pookie717/wordpress-playground
that referenced
this pull request
Oct 1, 2023
<!-- Thanks for contributing to WordPress Playground! --> ## What? This pr adds a 404 error if the requested path doesn't exist <!-- In a few words, what is the PR actually doing? Include screenshots or screencasts if applicable --> We decided to split #331 into two parts, to make changes clear. Part 2 of WordPress/wordpress-playground#331 Part 1 is WordPress/wordpress-playground#387 ## Why? When the user uses a path that doesn't have any files, the `wp-now` was throwing an error, and was no longer usable. This pr fixes that. Also, take a look at #331 <!-- Why is this PR necessary? What problem is it solving? Reference any existing previous issue(s) or PR(s), but please add a short summary here, too --> ## How? When we call `php.run` method, we build the `scriptPath` using `resolvePHPFilePath`. If the file is not there we assume that an `/index.php` file exists in the folder. This is not correct, and we need to return a 404 `PhpRequest`. Also, take a look at #331 <!-- How is your PR addressing the issue at hand? What are the implementation details? --> ## Testing Instructions - Create an empty folder - Run wp-now and make sure you add this folder as a path: npx nx preview wp-now start --path=<your-empty-folder> - Ensure that you don't see any errors in the console - Ensure that you see 404 File not found in the browser - Change the URL to something else. eg: /file-not-exist.php. - Ensure that you still see 404 File not found
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.
What?
We decided to split #331 into two parts, to make changes clear.
Part 1 of #331
Part 2 is #388
This pr simplifies the logic of
#handleRequestmethod by removing unnecessary async wrappingWhy?
It was originally introduced in #331, where a 404 page for nonexisting files was introduced, in order to simplify things.
For more info, look at #331
How?
asyncfunction.asyncfunctions return a promise by themselves and alsoresolvemethod is able to resolve nested promises if they returned correctly. So the originalasyncfunction wasn't really necessaryTesting Instructions
N/A