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

Jump to bottom

Fix/404 error if requested path doesn't exist #331

Merged
merged 14 commits into from
May 18, 2023
Merged

Fix/404 error if requested path doesn't exist #331

merged 14 commits into from
May 18, 2023

Conversation

kozer
Copy link
Contributor

@kozer kozer commented May 12, 2023 •
edited
Loading

Resolves: #304

This pr adds a 404 error if the requested path doesn't exist

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

@kozer kozer self-assigned this May 12, 2023
@kozer kozer changed the title Fix/add 404 error Fix/404 error if requested path doesn't exist May 12, 2023
@kozer kozer requested review from adamziel and sejas May 15, 2023 06:41
@kozer kozer requested review from danielbachhuber and wojtekn May 15, 2023 14:06
danielbachhuber
danielbachhuber reviewed May 15, 2023
View reviewed changes
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

I can confirm the 404 appears as expected when no index.php is present:

image

My index.php is rendered as expected when it exists.

Let's wait until #339 lands, then merge trunk so we can add some integration tests for this scenario (covering https://github.com/WordPress/wordpress-playground/pull/331/files#r1192930303 too)

adamziel
adamziel requested changes May 16, 2023
View reviewed changes
Copy link
Collaborator

@adamziel adamziel left a comment

Choose a reason for hiding this comment

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

Let's figure out the async function discussion before merging.

@danielbachhuber danielbachhuber mentioned this pull request May 16, 2023
Open
danielbachhuber and others added 2 commits May 17, 2023 13:56
@danielbachhuber
Merge branch 'trunk' into fix/add_404_error
7c4e283
@kozer
Update: Fix and add new tests
056fa42 
- Fix testing for current tests for Asyncify error (always passing
  before)
- Add a test to handle no promise cases
@kozer kozer requested review from danielbachhuber and adamziel May 18, 2023 08:48
@adamziel
Copy link
Collaborator

Let's split this into two PRs: one that deals with promises in base-php.ts and one that updates php-request-handler.ts. The framework-level change is important enough to warrant a separate, correctly labeled entry in git history.

@adamziel
Copy link
Collaborator

adamziel commented May 18, 2023 •
edited
Loading

We're almost there, btw. The implementation is sound and the two suggestions I left will take the tests where they need to be. Also note the typecheck CI job failed with:

packages/php-wasm/node/src/test/php.spec.ts(627,12): error TS18046: 'error' is of type 'unknown'.

This SO question should help: https://stackoverflow.com/questions/64452484/how-can-i-safely-access-caught-error-properties-in-typescript

@kozer kozer requested a review from adamziel May 18, 2023 12:20
@kozer kozer merged commit 61ba309 into WordPress:trunk May 18, 2023
kozer added a commit that referenced this pull request May 18, 2023
@kozer
Revert "Fix/404 error if requested path doesn't exist (#331)"
6d89530 
This reverts commit 61ba309.
@kozer kozer mentioned this pull request May 18, 2023
Merged
kozer added a commit that referenced this pull request May 18, 2023
@kozer
Revert "Fix/404 error if requested path doesn't exist" (#386)
436b629 
Reverts #331
This was referenced May 18, 2023
Merged
Merged
danielbachhuber pushed a commit that referenced this pull request May 18, 2023
@kozer
Update: Simplify base-php sapi_handle_request call (#387)
72221a5 
<!-- Thanks for contributing to WordPress Playground! -->

## What?

<!-- 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 1 of #331

Part 2 is #388

This pr simplifies the logic of `#handleRequest` method by removing
unnecessary async wrapping

## Why?

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 
<!-- 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?

- By removing the `async` function. `async` functions return a promise
by themselves and also `resolve` method is able to resolve nested
promises if they returned correctly. So the original `async` function
wasn't really necessary

<!-- How is your PR addressing the issue at hand? What are the
implementation details? -->

## Testing Instructions

N/A
kozer added a commit that referenced this pull request May 18, 2023
@kozer
Add: Support 404 page (#388)
df871b8 
<!-- 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
Pookie717 added a commit to Pookie717/wordpress-playground that referenced this pull request Oct 1, 2023
@Pookie717
Revert "Fix/404 error if requested path doesn't exist" (#386)
515f48c 
Reverts WordPress/wordpress-playground#331
Pookie717 added a commit to Pookie717/wordpress-playground that referenced this pull request Oct 1, 2023
@Pookie717
Update: Simplify base-php sapi_handle_request call (#387)
5ff7be3 
<!-- Thanks for contributing to WordPress Playground! -->

## What?

<!-- 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 1 of WordPress/wordpress-playground#331

Part 2 is WordPress/wordpress-playground#388

This pr simplifies the logic of `#handleRequest` method by removing
unnecessary async wrapping

## Why?

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 
<!-- 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?

- By removing the `async` function. `async` functions return a promise
by themselves and also `resolve` method is able to resolve nested
promises if they returned correctly. So the original `async` function
wasn't really necessary

<!-- How is your PR addressing the issue at hand? What are the
implementation details? -->

## Testing Instructions

N/A
Pookie717 added a commit to Pookie717/wordpress-playground that referenced this pull request Oct 1, 2023
@Pookie717
Add: Support 404 page (#388)
13cea4a 
<!-- 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
Reviewers

@adamziel adamziel adamziel approved these changes

@sejas sejas Awaiting requested review from sejas

@wojtekn wojtekn Awaiting requested review from wojtekn

@danielbachhuber danielbachhuber Awaiting requested review from danielbachhuber

Assignees

@kozer kozer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Local Environment: add a 404 error page when the file doesn't exist. For example index.php.
3 participants
@kozer @adamziel @danielbachhuber