WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 20 months ago

#42807 closed defect (bug) (maybelater)

Add filter to modfiy wp.api versionString

Reported by: mkaz Owned by: adamsilverstein
Milestone: Priority: normal
Severity: normal Version:
Component: REST API Keywords:
Focuses: javascript Cc:
PR Number:

Description

We have a need to modify the full REST URL which includes the domain as well as appending something after the versionString. The WordPress.com API is served at a different URL than the blog is posted, and includes the blogId for the request.

So for self-hosted request to /wp/v2/post we would need to be able to modify it to https://public-api.wordpress.com/wp/v2/sites/123456/post

There already exists the rest_url filter to modify the domain, but the versionString wp/v2/ is hard-coded in most spots and gets tangled up with schema and other parts.

Here is a suggested patch that would add a filter version_string which would allow modifying the hard-coded part. If no filter defined, it would default to the current value.

Index: wp-includes/rest-api.php
===================================================================
+++ wp-includes/rest-api.php    (working copy)
@@ -367,6 +367,27 @@
 }
  
 /**
+ * Retrieves the version string for the REST endpoint
+ *
+ * @since 5.0.0
+ * @return string version string, default wp/v2/
+ */
+function get_rest_version_string() {
+   $version_string = 'wp/v2/';
+
+   /**
+    * Filters the REST version string.
+    *
+    * Use this filter to adjust the url returned by the get_rest_version_string() function.
+    *
+    * @since 5.0.0
+    *
+    * @param string $version_string Version String
+    */
+   return apply_filters( 'version_string', $version_string );
+}
+
+/**
  * Retrieves the URL to a REST endpoint.
  *
  * Note: The returned URL is NOT escaped.

Index: wp-includes/script-loader.php
===================================================================
+++ wp-includes/script-loader.php   (working copy)
@@ -138,7 +138,7 @@
    did_action( 'init' ) && $scripts->localize( 'wp-api-request', 'wpApiSettings', array(
        'root'          => esc_url_raw( get_rest_url() ),
        'nonce'         => ( wp_installing() && ! is_multisite() ) ? '' : wp_create_nonce( 'wp_rest' ),
-       'versionString' => 'wp/v2/',
+       'versionString' => get_rest_version_string(),
    ) );
  

Change History (13)

#1 follow-up: @dd32
2 years ago

At first, this doesn't seem like a great direction for WordPress.com to go in, as it appears to me that it would perhaps diverge the API and cause incompatibilities with clients written for the .org API vs .com API (even when using the same API effectively).
Just thinking out loud here, I have a feeling a bunch of API clients are going to expect that they can take the root path element to the API and then suffix routes on from there. It also makes me question it'd possibly limit future uses of the API in plugins/etc (ie. Does it become /my-plugin/v1/sites/123456/my-endpoint when eventually supported? Etc)

I'm curious why the reverse wasn't taken instead here public-api.wordpress.com/sites/1234/wp/v2/post - Maybe explaining that reasoning would make it easier to understand the need for this filter?

#2 in reply to: ↑ 1 @rmccue
2 years ago

Replying to dd32:

I'm curious why the reverse wasn't taken instead here public-api.wordpress.com/sites/1234/wp/v2/post - Maybe explaining that reasoning would make it easier to understand the need for this filter?

This is somewhat a case of "the horse has already bolted", but I definitely agree here. Namespaces are linked directly to routes living directly underneath them, and some forms of feature detection rely on this being the case. Modifying the namespace definitely isn't something that I'd like to encourage, since it's intended to be a name.

Would it be feasible to change .com's API at this point though?

#3 @adamsilverstein
2 years ago

@mkaz Thanks for your bug report.

I can see how a filter might be useful here to change the default for initial load. In the meantime you should be able to overwrite this value already, or connect the wp-api client to another root/namespace after the initial connection.

To override the default, add this line right after you enqueue wp-api:

	wp_localize_script( 'wp-api', 'wpApiSettings', array(
			'root'          => esc_url_raw( get_rest_url() ),
			'nonce'         => ( wp_installing() && ! is_multisite() ) ? '' : wp_create_nonce( 'wp_rest' ),
			'versionString' => 'wp/v1/',
		) );

if you look in the source code this will come after core's localization and overwrite the values passed there.

Alternately, if the client is connecting fine at wp/v2 and you want to add an additional connection you can call init again:

wp.api.init( { 'versionString': 'wp/v1/' } ) - this will connect you do whatever versionString (or apiRoot if you provide that).

init returns a promise that resolves to the endpoint connection, which will contain the models and collections from that endpoint

in addition, multiple endpoints that have been connected thru init are stored in a collection: wp.api.endpoints

Does that help for your use case?

#4 @adamsilverstein
2 years ago

  • Focuses javascript added

#5 @adamsilverstein
2 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#6 follow-ups: @mkaz
2 years ago

Thanks @adamsilverstein for the suggestions - I've tried modifying the versionString in numerous other spots and it only seems to work within Gutenberg when modified at the source so it is picked up on initial load.

@rmccue @dd32 Agreed, if sites/blodId were a prefix instead of in the middle, it would be easier. I'm not sure about changing the WPCOM API.

#7 in reply to: ↑ 6 @adamsilverstein
2 years ago

Replying to mkaz:

Thanks @adamsilverstein for the suggestions - I've tried modifying the versionString in numerous other spots and it only seems to work within Gutenberg when modified at the source so it is picked up on initial load.

@mkaz
Ah, didn’t catch you were trying this for Gutenberg. Starting to see how this filter could be really useful, as long as we can ensure all uses of the namespace in core get filtered.

Did you try calling the init with your namespace on the api before gutenberg loads? this should add you data types to the wp.api models/collections and maybe let them work in Gutemberg?

One other idea if that doesn't work and you *really* need to replace the namespace used by Gutenberg:

If you can load after wp-api and before Gutenberg, you should be able to overwrite the wp.api.init method with your own implementation that calls the default implementation but sets the namespace to your own.

var originalInit = wp.api.init;
wp.api.init = function() { originalInit( { 'versionString': 'wp/v1/' } ) };

However looking at how Gutenberg is enqueueing wp-api, none of these changes are possible - the namespace is hardcoded in a server side schema call:

https://github.com/WordPress/gutenberg/blob/master/lib/client-assets.php#L489-L499

the schema is then localized and passed to the client. You will need to change this behavior to alter the namespace.

Maybe a good start would be a pull request for Gutenberg to add a filter there?

#8 @mkaz
2 years ago

@adamsilverstein I tried that, but unfortunately changing the schema wasn't successful.

The call to our schema at wp/v2/sites/{blogId} requires to be an authenticated request, typically handled by Javascript/Cookies but in Gutenberg it is being made server-side thus it is unauthenticated.

Another potential change we would need to do on WPCOM API.

#9 @adamsilverstein
2 years ago

@mkaz are you saying you tested changing the setup in gutenberg and it didn't work? you will probably also need to change it in the client by overwriting init so that its changed internally in the client for subsequent operations.

I'm surprised retrieving the schema requires authentication, can that be made public? Can you use a filter to turn off the authentication requirement? You can also remove the schema localization and the client will retrieve the schema when it loads although this has potential issues.

#10 in reply to: ↑ 6 @dd32
2 years ago

Replying to mkaz:

@rmccue @dd32 Agreed, if sites/blodId were a prefix instead of in the middle, it would be easier. I'm not sure about changing the WPCOM API.

FWIW Part of what I'm saying is that it doesn't seem like this is a filter that should be in core, and therefor supported as "something you should do". I think I'd rather see an alternative approach found instead, either core or on the WordPress.com side unless it's something that's needed by a lot of people.

One option instead would be to add a super-low-level filter which could be used as an override here - for example one in WP_Scripts::localize() (ie. wp_scripts_localise_l10n => $l10n, $handle, $object_name) and maybe something in WP_Scripts::add_inline_script() too. It'd have slightly more uses outside of this specific ticket too.

#11 @jnylen0
22 months ago

I think adding a low-level filter is a much more heavyweight approach than needed here.

A cleaner way to resolve this issue is outlined by #41549 and #41554. The idea is to introduce a single client-side code path for all API requests. Requests would be sent like wp.apiRequest( { namespace, path } ), and the function that builds the full API URL can be overridden if needed.

Similar functionality on the server side is already available through filters that modify the behavior of rest_do_request.

It would be pretty difficult to change the WP.com API structure at this point, and I think there's general value in adding flexibility in the way WordPress makes API calls.

#12 @mkaz
20 months ago

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

I'm solving this issue in a different way which doesn't require this change, I'll close out this ticket.

I still think a filter to be able to modify the version string would be useful, but understand it could be a bit dangerous.

#13 @netweb
20 months ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.