-
Notifications
You must be signed in to change notification settings - Fork 276
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
Conversation
@akirk @tellyworth @StevenDufresne @dd32 Can we open up CORS headers on |
There was a problem hiding this 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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_string
s 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 []; | ||
}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
...ib/steps/apply-wordpress-patches/wp-content/mu-plugins/includes/requests_transport_fetch.php
Outdated
Show resolved
Hide resolved
$options['filename'], | ||
substr($this->headers, $index + 4) | ||
); | ||
} |
There was a problem hiding this comment.
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 ) );
}
There was a problem hiding this comment.
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.
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
…d give non-proxied requests a chance to succeed if the CORS setup is right
aa2cae4
to
82893b1
Compare
…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
I'll go ahead and merge. CORS is not an issue with this one as:
|
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.
#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.
…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">
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
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 mechanismNetworking is enabled by default now, but it can be disabled with either:
?networking=no
{ "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:
wp_safe_remote_get()
to domain not on this list: ['api.wordpress.org', 'w.org', 's.w.org']cc @akirk @dmsnell @ellatrix