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

Add a shim to override wp.apiRequest, allowing HTTP/1.0 emulation #5741

Merged
merged 7 commits into from
Mar 27, 2018
Merged

Add a shim to override wp.apiRequest, allowing HTTP/1.0 emulation #5741

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c3c7f97
Add a shim to override wp.apiRequest, allowing HTTP/1.0 emulation
pento Mar 22, 2018
543bb37
options.headers is an associative array.
pento Mar 22, 2018
3154754
Add a link to the core ticket.
pento Mar 22, 2018
7c7ada0
Remove the now unnecessary Backbone.emulateHTTP setting
pento Mar 22, 2018
7ba2dda
\s++
pento Mar 23, 2018
a7abd50
PATCH !== GET
pento Mar 23, 2018
d22b177
Headers shoud be an object, not an array.
pento Mar 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions lib/client-assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,12 +614,6 @@ function gutenberg_extend_wp_api_backbone_client() {
wp_json_encode( $schema_response->get_data() )
), 'before' );
}

/*
* For API requests to happen over HTTP/1.0 methods,
* as HTTP/1.1 methods are blocked in a variety of situations.
*/
wp_add_inline_script( 'wp-api', 'Backbone.emulateHTTP = true;', 'before' );
}

/**
Expand Down
31 changes: 31 additions & 0 deletions lib/compat.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,37 @@ function gutenberg_shim_fix_api_request_plain_permalinks( $scripts ) {
}
add_action( 'wp_default_scripts', 'gutenberg_shim_fix_api_request_plain_permalinks' );

/**
* Shims support for emulating HTTP/1.0 requests in wp.apiRequest
*
* @see https://core.trac.wordpress.org/ticket/43605
*
* @param WP_Scripts $scripts WP_Scripts instance (passed by reference).
*/
function gutenberg_shim_api_request_emulate_http( $scripts ) {
$api_request_fix = <<<JS
( function( wp ) {
var oldApiRequest = wp.apiRequest;
wp.apiRequest = function ( options ) {
if ( options.method ) {
if ( [ 'PATCH', 'PUT', 'DELETE' ].indexOf( options.method.toUpperCase() ) >= 0 ) {
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could inverse the check to better reflect that we support only GET and POST:

if ( [ 'GET', 'POST' ].indexOf( options.method.toUpperCase() ) === -1 ) {

Aside: Curiosity got me wondering: https://jsperf.com/indexof-array-vs-regexp (tl;dr: This is good)

Copy link
Member Author

Choose a reason for hiding this comment

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

OPTIONS shouldn't be converted, either. (Neither should HEAD, CONNECT, or TRACE, even though we don't specifically use them anywhere.) I'm inclined to go with the same conversion as Backbone, as that's the one that is most likely to be supported.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I'm not clear what exactly we're doing in this pull request then. Are we targeting HTTP/1.0 ? There are only 3 defined verbs (previously overlooked HEAD):

https://www.w3.org/Protocols/HTTP/1.0/spec.html#Methods

It makes me a bit nervous that we target Backbone, as it's not obvious why we target Backbone (aside from anecdotal evidence that it appears to be well-supported).

if ( ! options.headers ) {
options.headers = [];
Copy link
Member

Choose a reason for hiding this comment

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

This should be assigned as an object, not array.

http://api.jquery.com/jquery.ajax/ (settings.headers)

Copy link
Member

@gziolo gziolo Mar 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Missed that, but array is also an object in JS... so might work, but obviously should be updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍🏻

}
options.headers['X-HTTP-Method-Override'] = options.method;
options.method = 'POST';
}
}

return oldApiRequest( options );
}
} )( window.wp );
JS;

$scripts->add_inline_script( 'wp-api-request', $api_request_fix, 'after' );
}
add_action( 'wp_default_scripts', 'gutenberg_shim_api_request_emulate_http' );
Copy link
Member

Choose a reason for hiding this comment

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

The previous shim was called inside gutenberg_editor_scripts_and_styles. This code uses wp_default_scripts action. Not sure what is the difference as I can't find what this action does.

Copy link
Member Author

Choose a reason for hiding this comment

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

No practical difference, I just copy/pasta-ed from the gutenberg_shim_fix_api_request_plain_permalinks shim above it.


/**
* Disables wpautop behavior in classic editor when post contains blocks, to
* prevent removep from invalidating paragraph blocks.
Expand Down