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

Add network support via fetch() #724

Merged
merged 15 commits into from Nov 8, 2023
Merged

Add network support via fetch() #724

merged 15 commits into from Nov 8, 2023

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Oct 25, 2023

Description

This pull request introduces network support to the WordPress Playground via the JavaScript fetch() API. By adding this support, the playground will have the ability to execute HTTP requests, paving the way for enhanced interactions with various APIs and services.

The implementation involves creating a custom transport that delegates HTTP requests to the fetch() API. This custom transport handles network requests, thus enabling communication outside the local environment.

Local testing will be a bit challenging until I deploy the updated plugin-proxy.php to playground.wordpress.net

CleanShot 2023-10-26 at 00 29 07@2x

How does it work?

A new Requests transport for WordPress asks JavaScript to run fetch() via post_message_to_js(). JavaScript runs the request and returns the response ľ see #732 for more details of that mechanism

Networking is enabled by default now, but it can be disabled with either:

  • Query API: ?networking=no
  • Blueprints API: { "features": { "networking": false } }

Testing Instructions:

Testing this feature locally might be challenging until the updated plugin-proxy.php is deployed to the playground environment. Here are some basic steps to test the changes:

  1. Open local Playground
  2. Set up an mu-plugin that calls wp_safe_remote_get() to domain not on this list: ['api.wordpress.org', 'w.org', 's.w.org']
  3. Ensure that the requests are handled correctly and the expected responses are received.
  4. Test various types of requests (GET, POST, etc.) and verify the correct response.
  5. Check the functionality with different APIs (e.g., REST API, external APIs).

cc @akirk @dmsnell @ellatrix

@adamziel
Copy link
Collaborator Author

adamziel commented Oct 25, 2023

@akirk @tellyworth @StevenDufresne @dd32 Can we open up CORS headers on api.wordpress.org and s.w.org to enable direct fetch() calls from Playground? That way there would be no PHP proxy required to use this networking feature. We may need to track it separately but I still wanted to start the discussion.

Copy link
Collaborator

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

preliminary review if it's helpful.

amazing you have this working

* Passes a message to the JavaScript module and writes the response
* data, if any, to the response_buffer pointer.
*
* @param message The message to pass into JavaScript.
Copy link
Collaborator

Choose a reason for hiding this comment

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

 * @param data

message data

also, what happened to/with data_len? I don't see us passing a length here or any way to communicate what's inside of data. am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

is it emscripten magic that handles the length of data here?

Copy link
Collaborator Author

@adamziel adamziel Nov 6, 2023

Choose a reason for hiding this comment

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

@dmsnell this assumes data is a null-terminated C string and no length is needed. The data is supposed to be json_encode()-d so there should never be a null byte in it. I'm happy to start supporting arbitrary zend_strings once a use-case arises, but there doesn't seem to be any now.

#endif
} else {
char *response;
size_t response_len = js_module_onMessage(data, &response);
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's another trick here I've seen used before that's kind of nice, which is to return a struct that includes all aspects of the response. that can get around some of the funniness of checking response and response_len and passing response as an uninitialized var, of passing an in and out parameter

typedef struct js_on_message_response_t {
	char did_succeed;
	char *data;
	size_t data_len;
} js_on_message_response;

extern js_on_message_response js_module_onMessage(…);

implementing code can do this:

js_on_message_response js_module_onMessage( … ) {
	js_on_message_response response = {0};

	…

	if ( error ) {
		return response;
	}

	if ( data ) {
		response.data = data;
		response.data_length = …;
		response.did_succeed = true;
		return response;
	}

	// default zero-initialized value is `did_succeed = false`
	return response;
}

anyway not a big deal, but we can return a heap-allocated object and not worry as much about allocations and then we can effectively return multiple values while maintaining clear boundaries between allocating and filling data structures.

Copy link
Collaborator Author

@adamziel adamziel Nov 6, 2023

Choose a reason for hiding this comment

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

Neat! I'd have to figure out how to allocate structs on the JavaScript side, but AFAIR that's just a few pointers next to each other? So maybe this would work:

const structSize = 1 /* did_succeed */ + 4 /* *data */ + 4 /* data_len */;
const structPtr = _malloc(responseSize);
HEAPU8[structPtr] = didSucceed ? 0 : 1;
HEAPU8[structPtr + 1] = dataPtr;
HEAPU8[structPtr + 2] = dataPtr >> 8;
HEAPU8[structPtr + 3] = dataPtr >> 16;
HEAPU8[structPtr + 4] = dataPtr >> 24;
HEAPU8[structPtr + 5] = dataSize;
HEAPU8[structPtr + 6] = dataSize >> 8;
HEAPU8[structPtr + 7] = dataSize >> 16;
HEAPU8[structPtr + 8] = dataSize >> 24;

Debugging these things and rebuilding PHP each time is a bit tedious so I might get this one in without a struct, but I would definitely want to make working with structs a part of Playground. Maybe we could have a helper such as allocateStruct(...data[]) or so, or just expose a allocate_js_message_response_struct C function 🤔

*/
add_filter('wp_signature_hosts', function ($hosts) {
return [];
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL

Also I confirmed by looking at these in curl and tried various ways of calling to find any X-Content-Signature headers and found none. wordpress.org/latest.zip at least returns a Content-MD5 header, but no signature. I tried OPTIONS requests too hoping they might provide one, but no.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @akirk @tellyworth @dd32 any ideas?

$options['filename'],
substr($this->headers, $index + 4)
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks safe, but maybe we should make it more obvious what we're doing?

// Store a file if the request specifies it.
// Are we sure that `$this->headers` includes the body of the response?
$before_response_body = strpos( $this->headers, "\r\n\r\n" );
if ( isset( $options['filename'] ) && false !== $before_response_body ) {
	$response_body = substr( $this->headers, $before_response_body + 4 );
	if ( strlen( $response_body ) !== (int) REQUEST_HEADER['content-length'] ) {
		// Big trouble in little Fetcher
		scream();
	}

	file_put_contents( only_safe_filenames( $options['filename'], $response_body ) );
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is only_safe_filenames a WordPress function? Or did you mean we could sanitize that in some way? I copied that part straight from either the Curl or FSockopen transport. I don't think there's an easy way to read the response headers here, unfortunately, as parsing only happens later on :-( But we can have that comment and clearer variable names as in the snippet you proposed.

adamziel added a commit that referenced this pull request Nov 6, 2023
post_message_to_js passes a message to the JavaScript module
where it can be handled inside onMessage(). This commit adds
support for returning values and promises inside that handler
to feed them back to PHP. For example:

```ts
	await playground.onMessage(async (message: string) => {
		const envelope: RequestMessage = JSON.parse(message);
		const { type, data } = envelope;
		if (type !== 'request') {
			return '';
		}

		return handleRequest(data);
	});
```

A handler like above could be used to add support for network requests,
which is in fact what
#724 does
adamziel added a commit that referenced this pull request Nov 6, 2023
…essage_to_js() (#732)

## Description

A part of #724

`post_message_to_js` passes a message to the JavaScript module where it
can be handled inside `onMessage()`. This commit adds support for
returning values and promises inside that handler to feed them back to
PHP. For example:

```php
$response = post_message_to_js(json_encode($request_data));
```

```ts
	await playground.onMessage(async (message: string) => {
		const requestData = JSON.parse(message);
		const response = await fetch(requestData);
		return JSON.stringify(toSimpleObject(response));
	});
```

A handler like above could be used to add support for network requests,
which is in fact what
#724 does
@adamziel
Copy link
Collaborator Author

adamziel commented Nov 8, 2023

I'll go ahead and merge. CORS is not an issue with this one as:

  • fetch() runs in a credentialless mode
  • A custom Blueprint may already install a WordPress plugin that will run fetch() on playground.wordpress.net on behalf of the user. This merely provides another way to do it.

@adamziel adamziel merged commit a5fff02 into trunk Nov 8, 2023
4 checks passed
@adamziel adamziel deleted the add/network-support branch November 8, 2023 11:49
adamziel added a commit that referenced this pull request Nov 15, 2023
     file. However, the setPhpIniEntry() was called after the initial
     php.run() which made it noop. This broke the networking support
     merged in #724.

     This commit solves that by moving the setPhpIniEntry right after
     the PHP module is initialized. I am not super happy about exporting
     the virtualized file path from the blueprints module – let's
     revisit that in the future.
adamziel added a commit that referenced this pull request Nov 17, 2023
#738 introduced a
change that made all WordPress constants be defined by including a
"virtualized" wp-consts.php file. However, the required
`setPhpIniEntry()` call was sometimes happening after the initial
`php.run()` which made it noop. This broke the networking support merged
in #724.

This commit solves that by exposing a new `php.defineConstant()` API
that handles the file virtualization. I'm not super happy about
virtualizing files so let's revisit that in the future, see #750.
adamziel added a commit that referenced this pull request Nov 27, 2023
…to enable it (#812)

Disables the network support by default and adds a UI control to
enable it.

Why?

#724 introduced
support for wp_safe_remote_get() request and enabled it by default on
playground.wordpress.net. The problem is, all the requests block
rendering of WordPress pages and noticeably slow down the site. Let's
disable it by default for a lightweight user experience, and then add an
easy way to enable it, for example in the configuration modal.

Closes #755

## Testing Instructions

* Go to http://localhost:5400/website-server/?url=/wp-admin/plugin-install.php
* Confirm the plugins aren't loaded and you see a communicative error message
* Go to the settings modal
* Enable the network support
* Confirm the plugins are loaded now

<img width="1121" alt="CleanShot 2023-11-27 at 17 10 45@2x"
src="https://github.com/WordPress/wordpress-playground/assets/205419/ea95b783-d7a1-45c6-ab95-90b5c8ec6ce4">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants