WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 7 weeks ago

Last modified 7 weeks ago

#40919 closed enhancement (fixed)

REST API: Allow overriding `jQuery.ajax` calls from within wp-admin

Reported by: jnylen0 Owned by: jnylen0
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.8
Component: REST API Keywords: has-patch has-unit-tests commit
Focuses: javascript, administration, rest-api Cc:

Description

In WP 4.8, there are now 3 places where we call out to the REST API from within wp-admin (see attached patch for details):

  • When viewing the settings for the video widget, to load a video preview.
  • When editing the settings for a video widget in the media modal, to load a video preview.
  • When embedding from a URL in the post editor, to load an oEmbed preview.

These all call jQuery.ajax directly. We need to avoid this going forward because it is impossible to override. I am aware of one large, customized WordPress installation cough where we will need to put this change in place as a core hack. (jQuery has functionality to override AJAX request options but this is not sufficient due to the way cross-domain API requests work on WP.com.)

There are other benefits as well, such as testability (this wrapper will provide a cleaner way to mock these API responses) and extensibility (this will be a natural place to add a filter when #21170 lands).

Attachments (5)

40919.diff (5.1 KB) - added by jnylen0 4 months ago.
40919.2.diff (6.1 KB) - added by timmydcrawford 3 months ago.
40919.3.diff (13.8 KB) - added by jnylen0 2 months ago.
Pass nonce as header; use main wpApiSettings variable; allow requests via path/namespace/endpoint; add unit tests
40919.4.diff (15.5 KB) - added by jnylen0 2 months ago.
Update @since version; only add nonce if not already present; more tests
40919.5.diff (15.5 KB) - added by jnylen0 7 weeks ago.
Refresh against latest trunk

Download all attachments as: .zip

Change History (24)

@jnylen0
4 months ago

#1 @jnylen0
4 months ago

  • Keywords needs-refresh dev-feedback added
  • Owner set to jnylen0
  • Status changed from new to assigned

40919.diff is incomplete:

  • It changes the built media-views.js file but not the source file wp-includes/js/media/views/embed/link.js.
  • It also needs to be properly remade against a dev installation of WP.
  • Anything overriding wp.RESTRequest should depend on the wp-rest-request script; however, we probably want to avoid clobbering an existing wp.RESTRequest function.

This ticket was mentioned in Slack in #core-customize by timmyc. View the logs.


4 months ago

#3 @westonruter
4 months ago

I think it should automatically handle adding the nonce, should it not?

#4 @rmccue
4 months ago

Right now, this patch is pretty useless, and is basically just abstraction for abstraction's sake (I get the need behind it, but there's already ways to do that).

That said, if it incorporated the nonce header for you automatically, I'd be happy to use it.

Not a big fan of the naming here though; RESTRequest sounds like a class name to me. wp.api.request() I think would be a better name, and fits with the existing wp.api for the Backbone library. That said, it's also slightly confusing in that you'd have two scripts with an overlapping namespace. wp.api_request or wp.rest_request might be better there?

(We could also consider switching to use fetch() with the polyfill for this...)

This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.


4 months ago

#6 @timmydcrawford
3 months ago

  • Keywords needs-refresh removed

40919.2.diff changes the name of the method to wp.restRequest and adds in _wpnonce to the options.data that is passed along to the $.ajax call.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


2 months ago

@jnylen0
2 months ago

Pass nonce as header; use main wpApiSettings variable; allow requests via path/namespace/endpoint; add unit tests

#8 @jnylen0
2 months ago

In 40919.3.diff:

  • Rename to wp.apiRequest to match the existing wp.api.
  • Use the main window.wpApiSettings variable to get the nonce and the root URL.
  • Pass the nonce as a header instead of a query string argument.
  • Allow sending requests via path parameter (or namespace and endpoint parameters) instead of requiring full URLs. This eliminates the need for plugins and themes to call rest_url themselves.
  • Add QUnit tests.

I would still like to get this into 4.8.1 if possible. @westonruter @adamsilverstein are either of you available to take a look?

Future follow-up steps:

  • Use this helper for AJAX requests in the wp-api.js client so that there is only one code path for providing nonces, sending requests, etc. This ticket sets up the appropriate script dependencies.
  • Implement nonce refreshing here. See #37569 and #40422.

#9 @westonruter
2 months ago

@jnylen0 Instead of adding wp.apiRequest via a newly-enqueued script, what about adding this as wp.api.request and make it part of the existing wp-api.js? This will be required for this to be part of 4.8.1 since minor releases can't introduce new files (does not apply to tests) due to how automatic updates work.

#10 @jnylen0
2 months ago

The idea is that we can load wp.apiRequest without loading all of wp.api, for example in the existing wp-admin uses of the API, and probably in Gutenberg as well since we're not using Backbone otherwise.

I forgot about the new file rule. In that case I'd say this can wait until 4.9.

#11 @jnylen0
2 months ago

  • Keywords has-unit-tests needs-refresh added; dev-feedback removed
  • Milestone changed from 4.8.1 to 4.9

Punting to 4.9 to enable adding a new file. Patch needs refresh for at the very least changing 4.8.1 to 4.9.0. If there are no blockers, I plan to make that change and land this within the next week or two.

Edit: this could also use a test to ensure that pre-existing values for the X-WP-Nonce header are preserved.

Last edited 2 months ago by jnylen0 (previous) (diff)

@jnylen0
2 months ago

Update @since version; only add nonce if not already present; more tests

#12 @jnylen0
2 months ago

  • Keywords needs-refresh removed

Refreshed in 40919.4.diff.

This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.


2 months ago

@jnylen0
7 weeks ago

Refresh against latest trunk

#14 @jnylen0
7 weeks ago

  • Keywords commit added

I plan to land this tomorrow. A further change for 4.9 will be to make wp-api.js use this function. I'll create a separate ticket for that.

This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.


7 weeks ago

#16 @jnylen0
7 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 41206:

REST API: Allow overriding jQuery.ajax calls from within wp-admin

There are now 3 places where we call out to the REST API from within wp-admin. This commit introduces a small library to allow overriding these calls, centralize nonce-passing logic, and eliminate the need to pass a full REST URL down to client code (this last feature is not yet used and will be explored in a separate ticket).

Fixes #40919.

#17 @jnylen0
7 weeks ago

In 41207:

Remove unnecessary semicolon

See #40919.

#18 @jnylen0
7 weeks ago

In 41222:

REST API: Add QUnit tests for api-request.js

These should have been included in [41206].

See #40919.

#19 @afercia
7 weeks ago

In 41224:

REST API: Make jshint happy again after [41222].

See #40919.

Note: See TracTickets for help on using tickets.