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

Static files run as PHP when path includes directory like <some-dir>.php/ #1365

Closed
brandonpayton opened this issue May 6, 2024 · 11 comments
Closed
Assignees
Labels
Milestone

Comments

@brandonpayton
Copy link
Member

If a static file is under a directory tree where an ancestor directory has a name ending in ".php" (e.g., /root/some.php/assets/photo.jpeg), Playground will say that the file seems like a PHP file:

function seemsLikeAPHPFile(path: string) {
return path.endsWith('.php') || path.includes('.php/');
}

And IIUC Playground will try to run such non-PHP files as PHP:

if (!seemsLikeAPHPRequestHandlerPath(fsPath)) {
return this.#serveStaticFile(
await this.processManager.getPrimaryPhp(),
fsPath
);
}
return this.#spawnPHPAndDispatchRequest(request, requestedUrl);

Discovered when investigating #1332.

@adamziel
Copy link
Collaborator

adamziel commented May 6, 2024

I wonder what's the path resolution algorithm Apache of Nginx uses and can we do the same here. One complication in the web version of Playground is that some static files aren't shipped in the VFS but are fetch()-ed on demand from https://playground.wordpress.net/.

@brandonpayton
Copy link
Member Author

@adamziel I am currently trying to work through this. Initially, I was going to create a PR to quickly communicate my thoughts but kept finding edge cases.

What is difficult is:

  • We need to support requests for non-existent permalink paths
  • Permalink paths may not end with an explicit / which indicates a directory
  • Some static files do not exist on disk (as you mentioned)

I think it is probably OK in most cases to treat files without an extension (i.e., without a "." in them) as directories. That is probably a reasonable first pass. In addition, maybe we can only do this for WP source directories. That way, plugins that include requests for extension-less files do not break when we incorrectly guess their static file path points to a directory.

If we find we need to handle these decisions with more certainty within requests for WP core assets, perhaps the stripped-down WordPress builds could include an explicit listing of the static files that exist remotely.

@brandonpayton
Copy link
Member Author

I think it is probably OK in most cases to treat files without an extension (i.e., without a "." in them) as directories. That is probably a reasonable first pass. In addition, maybe we can only do this for WP source directories. That way, plugins that include requests for extension-less files do not break when we incorrectly guess their static file path points to a directory.

I was mistaken. We can't easily treat Playground-provided asset paths in a special way because some of Playground's remote assets are under wp-content/(mu-plugins|plugins|themes) directories along with third-party code.

As a first pass, we can handle a path with PHP when any of the following are true:

  • the path ends with .php
  • the path is an existing directory
  • the path ends with /
  • the path does not exist in the FS and contains no . character in the base filename

The line

the path does not exist in the FS and contains no . character in the base filename

will probably work for most cases, but it will break if custom permalink structures contain . characters in the path. Third party code may also rely on such permalink delegation to implement other things like custom endpoints.

A couple options to solve this are:

  1. Make a request for a remote asset. If there is a 404 response, delegate the request to WordPress.
  2. Include a list of remote static asset paths with the stripped-down WP builds, so we can know exactly what paths are remote and which should be delegated to WordPress.

@brandonpayton brandonpayton self-assigned this May 8, 2024
@brandonpayton
Copy link
Member Author

I wonder what's the path resolution algorithm Apache of Nginx uses and can we do the same here.

@adamziel I got ahead of myself and forgot to answer directly. The nginx behavior is completely defined in the config, and we can use the WordPress.org nginx example for reference:

One complication in the web version of Playground is that some static files aren't shipped in the VFS but are fetch()-ed on demand from https://playground.wordpress.net/.

This is what makes the last rule

Other paths are tried first as a file, then as a directory where index.php may be present, then delegated to WP's index.php

hard to implement.

The first thing to try is whether a file exists at that path, but we cannot know that without additional info. So we either need to take a different approach as mentioned above or add metadata to the stripped WP builds so we know what files exist remotely.

After spelling out the above nginx config, I think we should just generate the metadata, use it, and be done with all the special cases. What do you think?

@adamziel
Copy link
Collaborator

adamziel commented May 8, 2024

Metadata sound great and could also help with building a PWA (unless we'll just use a non-minified build for that as it's easy to do and already enables fetch-less usage). Cc @bgrgicak

@bgrgicak
Copy link
Collaborator

bgrgicak commented May 9, 2024

Having a list of metadata would also help me in the case of this draft PR #1203. I wanted to add the list to it, but didn't have the time.

Similarly for offline support. We will need to download all files when the PWA is installed and this list is a requirement.

adamziel added a commit that referenced this issue May 9, 2024
… them in wp-content/mu-plugins) (#1366)

This PR moves the Playground mu-plugins from
`/wordpress/wp-content/mu-plugins`, where they may interfere with the
user-provided files and pollute the git repository, to
`/internal/mu-plugins`, where they're invisible to the Playground
consumer and don't show up in exports or OPFS mounts.

Closes #1318

## Changes made

Technically, this PR uses the `auto_prepend_file` PHP options to
pre-load the following script:

```php
<?php
foreach (glob('/internal/preload/*.php') as $file) {
	require_once $file;
}
```

From here, preloading files is as simple as creating them in
`/internal/preload`. This PR ships a few such preloaded files:

* `consts.php`, that enables predefining PHP constants without affecting
`wp-config.php`.
* `env.php`, that preloads mu-plugins from `/internal/mu-plugins` using
[the method described
by](#1318 (comment))
@brandonpayton.
* `phpinfo.php`, that just calls `phpinfo();` when the request URL is
`/phpinfo.php` – this prevents creating an actual `phpinfo.php` file in
the document root. This involves a slight change in the path resolution
logic which will have to change even more soon to solve #1365.

From there, the SQLite integration plugin and the Playground mu-plugins
are placed in `/internal/mu-plugins` and do not affect the WordPress
file structure.

This PR also reduces code duplication by moving some parts of the
WordPress setup to the `@wp-playground/wordpress` package where they can
be imported by both the web and the CLI versions of Playground. We'll
only see more of these moving forward.

## Follow-up work

* Define clear use-cases and behaviors related to the location of
Playground-provided files. E.g. Full site export, where we want to keep
the SQLite integration plugin, or a GitHub repo, where we only want to
keep track of our changes in wp-content.
* Use `@wp-playground/cli` to build and setup WordPress in
`packages/playground/wordpress-builds/build/Dockerfile`
* Playground web: do not ship the SQLite integration plugin in the
pre-built `wp-content` directory. Source it from the
`/internal/mu-plugins` directory same way as in the CLI build.
* Do not create the drop-in plugin `wp-content/db.php`
* Do not install the WordPress importer plugin in `wp-content/plugins`
* Figure out how to avoid touching `wp-config.php` – defining PHP
constants may require rewriting it and it might be desirable to keep
those changes on export

## Testing instructions

* Confirm the tests pass
* In the browser – export Playground as zip, import it, also refresh the
page and import again, confirm it all worked.
@brandonpayton
Copy link
Member Author

Having a list of metadata would also help me in the case of this draft PR #1203. I wanted to add the list to it, but didn't have the time.

@bgrgicak, cool. 👍

OK, I am planning to work on this next.

@bgrgicak
Copy link
Collaborator

Thanks! Ping me for a review when it's ready and then I can use it in #1203.

@brandonpayton
Copy link
Member Author

Thanks! Ping me for a review when it's ready and then I can use it in #1203.

Will do! I ended up spending most of today working on things related to the website migration but hope to work on this tomorrow.

It looks like this where we can save a list of remote static files which can be added to the stripped-down WP zip and read later:

# Separate WordPress static files
RUN cp -r wordpress wordpress-static && \
cd wordpress-static && \
find ./ -name '*.php' -delete && \
# Keep only the static files inside the directories like wp-admin or wp-content:
find . -maxdepth 1 -type f -delete && \
# Remove all empty directories
find . -type d -empty -delete
# Remove the sqlite database integration from the static files
RUN rm -rf wordpress-static/wp-content/mu-plugins

@brandonpayton
Copy link
Member Author

I made a draft PR for listing remote static asset paths so we can make better decisions in request handling: #1417

We also need to make a change to this line as a separate PR so we don't try to handle a static file under a some.php/ directory as PHP. Planning to work on that tomorrow.

brandonpayton added a commit that referenced this issue May 30, 2024
…1417)

## What is this PR doing?

This PR:
- Includes a list of remote asset paths with minified WP builds
- Updates request handling to only handle missing files as static when
they are explicitly listed as remote assets
(perhaps this should be split into multiple PRs)
- Updates request routing to behave more like a regular web server

Related to #1365 in that we need better information to judge whether to
handle a request for a missing static file as PHP.

## What problem is it solving?

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate the request to PHP.

## How is the problem addressed?

By including a list of remote asset paths with minified WP builds, we
can judge which missing static file paths represent remote assets and
which can be delegated to PHP.

## Testing Instructions

- CI tests
- Examine a minified WP zip and the contents of its
wordpress-remote-asset-paths file
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
brandonpayton added a commit that referenced this issue Jun 6, 2024
…1417)

This PR:
- Includes a list of remote asset paths with minified WP builds
- Updates request handling to only handle missing files as static when
they are explicitly listed as remote assets
(perhaps this should be split into multiple PRs)
- Updates request routing to behave more like a regular web server

Related to #1365 in that we need better information to judge whether to
handle a request for a missing static file as PHP.

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate the request to PHP.

By including a list of remote asset paths with minified WP builds, we
can judge which missing static file paths represent remote assets and
which can be delegated to PHP.

- CI tests
- Examine a minified WP zip and the contents of its
wordpress-remote-asset-paths file
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
brandonpayton added a commit that referenced this issue Jun 6, 2024
…1417)

This PR:
- Includes a list of remote asset paths with minified WP builds
- Updates request handling to only handle missing files as static when
they are explicitly listed as remote assets
(perhaps this should be split into multiple PRs)
- Updates request routing to behave more like a regular web server

Related to #1365 in that we need better information to judge whether to
handle a request for a missing static file as PHP.

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate the request to PHP.

By including a list of remote asset paths with minified WP builds, we
can judge which missing static file paths represent remote assets and
which can be delegated to PHP.

- CI tests
- Examine a minified WP zip and the contents of its
wordpress-remote-asset-paths file
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
brandonpayton added a commit that referenced this issue Jun 7, 2024
… Second attempt (#1490)

## Motivation for the change, related issues

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate a request for a missing
file to WordPress.

Related to #1365 where we need better information to judge whether to
handle a request for a missing static file as PHP.

## Implementation details

This PR:
- Includes a list of remote asset paths with minified WP builds
- Updates request handling to only handle missing files as static when
they are explicitly listed as remote assets
(perhaps this should be split into multiple PRs)
- Updates request routing to behave more like a regular web server

By including a list of remote asset paths with minified WP builds, we
can judge which missing static file paths represent remote assets and
which are truly missing and need to be delegated to WordPress.

## Testing Instructions (or ideally a Blueprint)

- CI tests
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
brandonpayton added a commit that referenced this issue Jun 24, 2024
… Second attempt (#1490)

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate a request for a missing
file to WordPress.

Related to #1365 where we need better information to judge whether to
handle a request for a missing static file as PHP.

This PR:
- Includes a list of remote asset paths with minified WP builds
- Updates request handling to only handle missing files as static when
they are explicitly listed as remote assets
(perhaps this should be split into multiple PRs)
- Updates request routing to behave more like a regular web server

By including a list of remote asset paths with minified WP builds, we
can judge which missing static file paths represent remote assets and
which are truly missing and need to be delegated to WordPress.

- CI tests
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
brandonpayton added a commit that referenced this issue Jun 27, 2024
… Second attempt (#1490)

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate a request for a missing
file to WordPress.

Related to #1365 where we need better information to judge whether to
handle a request for a missing static file as PHP.

This PR:
- Includes a list of remote asset paths with minified WP builds
- Updates request handling to only handle missing files as static when
they are explicitly listed as remote assets
(perhaps this should be split into multiple PRs)
- Updates request routing to behave more like a regular web server

By including a list of remote asset paths with minified WP builds, we
can judge which missing static file paths represent remote assets and
which are truly missing and need to be delegated to WordPress.

- CI tests
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
brandonpayton added a commit that referenced this issue Jul 13, 2024
… Second attempt (#1490)

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate a request for a missing
file to WordPress.

Related to #1365 where we need better information to judge whether to
handle a request for a missing static file as PHP.

This PR:
- Includes a list of remote asset paths with minified WP builds
- Updates request handling to only handle missing files as static when
they are explicitly listed as remote assets
(perhaps this should be split into multiple PRs)
- Updates request routing to behave more like a regular web server

By including a list of remote asset paths with minified WP builds, we
can judge which missing static file paths represent remote assets and
which are truly missing and need to be delegated to WordPress.

- CI tests
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
brandonpayton added a commit that referenced this issue Jul 18, 2024
This PR:
- Updates PHPRequestHandler to support custom file-not-found responses
- Updates worker-thread to configure PHPRequestHandler to handle
   file-not-found conditions properly for WordPress
- Updates worker-thread to flag remote WP assets for retrieval by the
    Service Worker

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate a request for a missing
file to WordPress.

Related to #1365 where we need better information to judge whether to
handle a request for a missing static file as PHP.

- CI tests
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
brandonpayton added a commit that referenced this issue Jul 18, 2024
This PR:
- Updates PHPRequestHandler to support custom file-not-found responses
- Updates worker-thread to configure PHPRequestHandler to handle
   file-not-found conditions properly for WordPress
- Updates worker-thread to flag remote WP assets for retrieval by the
    Service Worker

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate a request for a missing
file to WordPress.

Related to #1365 where we need better information to judge whether to
handle a request for a missing static file as PHP.

- CI tests
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
brandonpayton added a commit that referenced this issue Jul 19, 2024
## Motivation for the change, related issues

This PR updates PHPRequestHandler to route HTTP requests more like a
regular web server.

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate a request for a missing
file to WordPress.

Related to #1365 and #1187

## Implementation details

This PR:
- Updates PHPRequestHandler to support custom file-not-found responses
- Updates worker-thread to configure PHPRequestHandler to handle
file-not-found conditions properly for WordPress
- Updates worker-thread to flag remote WP assets for retrieval by the
Service Worker

## Testing Instructions (or ideally a Blueprint)

- CI tests
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
@brandonpayton
Copy link
Member Author

This was fixed by #1539.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

3 participants