Use REST API for Community Events

[iandunn]

#40702 introduced new Community Events to the News widget on the Dashboard screen, but it uses admin-AJAX.

Converting to the REST API is a good opportunity to lay some groundwork for migration the rest of wp-admin in the future.

The work for this was started in #40702, but it'll be easier to keep track of with a new ticket.

I'm working on an updated version of 40702.11.diff and will upload it soon.

[matt]

Just so everyone is on the same page, this is out of scope for 4.8 but hopefully can come in soon after.

[iandunn]

Iteration on 40702.11.diff

[iandunn]

40748.diff​ is an iteration on 40702.11.diff from #40702, props @flixos90, @adamsilverstein, @azaozz, @swissspidy. (It'd be nice to give @georgestephanis and @ocean90 props on this as well, since they should have gotten it for #40702 but accidentally didn't.)

Changes made:

• refresh after r40651
• call renderEventsTemplate in modelMe.always, so the logic is more straight-forward and less scattered throughout the various event callbacks.
• fixed bug when searching for location that isn't found (e.g., narnia). response.code doesn't exist, need to use response.responseJSON.code
• remove unneeded property checks in tmpl-community-events-event-list because they should always exist
• related to removing those unneeded checks, I fixed a bug where date/time data didn't show up when displaying cached events. An alternative approach would be to call WP_REST_Community_Events_Events_Controller() in wp_get_community_events_script_data(). I don't have a strong opinion about which is better.
• load the dashboard schema after app.init has been called, but before getEvents is called. This way, we don't have to wait on the schema to load when we have cached data, but we can start loading the schema immediately on init, instead of waiting until getEvents gets called. This is really only helpful for situations where there is cached data, but then the user decides to manually change locations. In those situations, though, it could potentially mean they only have to wait on 1 HTTP request, instead of 2.
• removed duplicate requestParams, initiatedBy, spinner statements inside dashboardLoadPromise.done
• removed rest_url and nonce_url from wp_get_community_events_script_data() because wp.api provides those
• removed delete response._embedded and ._links from meModel.success because response isn't being passed to renderEventsTemplate anymore
• removed console.log statements
• removed stray ;

Next Steps

• Feedback and iterations on 40748.diff​
• Discussion on namespace, see @jnylen0's comment in comment:41 in #40702

[iandunn]

At the risk of starting a bikeshed discussion, "dashboard" feels odd to me as a term for all internal routes. I've always considered the Dashboard to only be the individual screen at wp-admin/index.php. I think that's consistent with how the menu and ​Codex describe it.

It's not uncommon to hear the term used to describe all of wp-admin, though, and ​language is determined by the people who use it, so it wouldn't bother me if that's what we choose.

[azaozz]

Looking at the REST API patches on #40702, think there is a misunderstanding here. Why is the scope so much expanded and the functionality so different when trying to switch from admin-ajax to REST?

Generally the scope for this bit of code is to do one thing only: return a little bit of data once per page load for use in the "WordPress Events and News" dashboard widget. Everything else is pretty pointless at this stage.

• It is not meant to be a general use REST API endpoint.
• It is not meant to be used by plugins and themes for any other purpose.
• It is not meant to turn all WP installs into proxies to access geolocation data from wordpress.org's API.
• It is not meant to support any form of arbitrary usage.

It is meant to do only one thing: provide a small amount of data for the dashboard widget.

On top of that I think this conversion sends out the wrong message. No matter how I try to look at it, the example is negative. It replaces 24 lines of simple, straightforward, easy to read and understand code with exactly 498 lines of much more complex, harder to read and understand code contained in two new files! In addition the new code seems to introduce bugs, edge cases and unneeded functionality. Is that really a typical example on how to use the REST API instead of admin-ajax? How can we expect plugin authors to want to do something like that in their plugins?

[rmccue]

Replying to azaozz:

Looking at the REST API patches on #40702, think there is a misunderstanding here. Why is the scope so much expanded and the functionality so different when trying to switch from admin-ajax to REST?

I agree with @azaozz here, we don't need two controllers and their boilerplate for this functionality. My original thinking was to keep this quite minimal.

We _can_ have schema if we really want, but I don't see that there's a huge benefit to it. We don't really care about other people using the endpoint, and it's not part of our "public" REST API (which is why it has a different namespace).

Will work on an updated patch.

[rmccue]

Switch from admin-ajax to the REST API

[rmccue]

40748.2.diff​ switches from admin-ajax to the REST API, under the new dashboard/v1 namespace. (The specific naming of this namespace is ​under discussion, but not relevant to this discussion.) The diffstat for record is +71 -36.

As part of this, it switches from wp.ajax to $.ajax directly. Since there's no global variables that contain the REST API URL or nonce, we have to keep passing these in. We could enqueue wp-api, but we don't need anything else from the Backbone library, so just passing the variables is simpler.

Because there's no other dashboard-only endpoints added yet, this includes the minimal infrastructure work to get that happening. In particular, it adds the new wp-admin/includes/rest-endpoints.php file, which is loaded in wp-settings.php. We might want to move this to create_initial_rest_routes to avoid loading an admin file on every page load.

This isn't particularly RESTful or anything, and the response format isn't what I'd really like, but since it's just a proxied response from w.org it doesn't matter. (I previously recommended changing the format, but hadn't realised it was just a proxied response, so apologies for misleading there.)

This also doesn't use the controller pattern, or schema, or... intentionally. The API was designed so that all you really need is a single callback, because not everything needs the kitchen sink suite of REST tools. Sometimes you just want to send some data from the server to the client.

[rmccue]

Replying to rmccue:

Because there's no other dashboard-only endpoints added yet, this includes the minimal infrastructure work to get that happening. In particular, it adds the new wp-admin/includes/rest-endpoints.php file, which is loaded in wp-settings.php. We might want to move this to create_initial_rest_routes to avoid loading an admin file on every page load.

Also worth noting: adding this infrastructure makes it way easier to add more endpoints for new features in the future, since it establishes the pattern. Basically, the process changes from adding an item to the admin-ajax.php list and making a specially-named function, to adding the register call and a function to implement it.

(Once we've finalised the naming of this new namespace and bikeshedded it, I'll also enshrine this into the Core Handbook.)

[joehoyle]

I agree with @rmccue that we don't need fully-fledged endpoints for this, as it's really just an alternative to admin-ajax and not part of the /wp namespace. I think all we need to do is set a good precedent for using the REST API as an ajax router (in this case). If I were to add anything I'd also add an GET endpoint, and use rest_do_request to populate the PHP data server-side too. This is again a good example to lay down for future REST API admin work.

Given the simplicity of 40748.2.diff I think we could probably drop this in 4.8, but I understand @matt's reluctance to do that given how late it is.

[jnylen0]

I think we should keep the schema from 40748.diff​. Even if this is an internal-only endpoint, one of the strengths of the REST API is that it provides a standardized way to describe request and response formats.

In this case it's really helpful for developers that want to use this endpoint, whether that's core developers or someone writing a plugin to add more functionality later on. Adding a schema also sets a good example for future work.

I don't have a preference on namespace between dashboard/v1 and wp-dashboard/v1, just that the namespace contains exactly one slash. I also think we should document this more explicitly and deprecate registration of endpoints that do not follow this pattern.

Finally, agreed that this needs unit tests.

[flixos90]

I agree with @jnylen0. I don't see any reason why this endpoint should only be considered "internal". Allowing other developers to use it (if they wanna do custom things with community events) should be a best practice for any endpoint in my opinion.

Also, suddenly not following the concepts and structures for endpoints that were just introduced in 4.7, seems way too early to me to break a pattern. More code might possibly contain more bugs, but I hardly see this as an argument (we might as well just have used simply functions without schemas for the other endpoints then), especially as we're supposed to heavily test and write unit tests for everything to knock such things out in advance as best as possible. In the long run, following a proper structure and providing a schema defines clear boundaries for the data being returned and raises the quality and reusability of the code.

[azaozz]

Replying to rmccue:

...we don't need two controllers and their boilerplate for this functionality. My original thinking was to keep this quite minimal.

Exactly, 40748.2.diff looks much much better.

adding this infrastructure makes it way easier to add more endpoints for new features in the future, since it establishes the pattern. Basically, the process changes from adding an item to the admin-ajax.php list and making a specially-named function, to adding the register call and a function to implement it.

Yep, and that is a lot more valuable example than adding fully fledged endpoint(s) for a mostly private functionality.

Replying to jnylen0:

I think we should keep the schema from 40748.diff​. Even if this is an internal-only endpoint, one of the strengths of the REST API is that it provides a standardized way to describe request and response formats.

Right, also having the schema would make it a better example (following best practices).

In this case it's really helpful for developers that want to use this endpoint, whether that's core developers or someone writing a plugin to add more functionality later on. Adding a schema also sets a good example for future work.

This is a "mostly" private end point. We don't plan on maintaining 100% backwards compatibility and don't recommend for plugins to reuse. It may (and probably will) change in 4.8, 4.9, etc.

One of the original ideas was to also add a front-end (standard) widget people can put on their sites. This can happen in the future and would mean things are likely to change a bit.

I'm not sure what would be the best way to pass this message to plugin authors that may think to reuse the endpoint. Would that be in the schema somewhere? (Having this would also be quite helpful as a precedent for plugins.)

Replying to flixos90:

Allowing other developers to use it (if they wanna do custom things with community events) should be a best practice for any endpoint in my opinion.

Don't think so. There are "public" and "private" use endpoints. This is a private use one, and I'd think most endpoints added by plugins will be private (even if there is a schema). Having to carry the back-compat responsibility is heavy enough for core, most plugins wouldn't do that.

Like with all private functions in WordPress, plugin developers should be discouraged to use private (or mostly private) end points.

[jnylen0]

Replying to azaozz:

This is a "mostly" private end point. We don't plan on maintaining 100% backwards compatibility and don't recommend for plugins to reuse. It may (and probably will) change in 4.8, 4.9, etc.

One of the original ideas was to also add a front-end (standard) widget people can put on their sites. This can happen in the future and would mean things are likely to change a bit.

I'm not sure what would be the best way to pass this message to plugin authors that may think to reuse the endpoint. Would that be in the schema somewhere? (Having this would also be quite helpful as a precedent for plugins.)

AFAIK this doesn't exist yet. We could add a flag, or we could say that anything under the dashboard/v* namespace is not subject to the same backwards compatibility expectations as wp/v2, and document this carefully.

I question why we want to go ahead and say that we won't maintain backwards compatibility for this endpoint, though. Shouldn't we try anyway? Isn't that the whole point of the /v1 versioning mechanism (new endpoints would be /v2 or /v1.1, and the old ones would stay in place)?

[rmccue]

Replying to jnylen0:

AFAIK this doesn't exist yet. We could add a flag, or we could say that anything under the dashboard/v* namespace is not subject to the same backwards compatibility expectations as wp/v2, and document this carefully.

This is basically the plan. dashboard/ would be the prefix for endpoints just for the "dashboard app", which wouldn't meet the same design standards or back compat guarantees. That said, I don't think we'd necessarily have to just break backwards compat, we can continue iterating just like any API. (We could also continue to do the standard deprecation in release n, but then actually commit to removing it in release n+1.)

dashboard/ is basically the same as us creating internal PHP classes or functions called _wp_....

(See ​the proposal which would also add docs to the core contributor handbook; we can add further docs as well.)

---

I'll update the patch to re-add the schema back in, since it seems like we still want that.

[TimothyBlynJacobs]

Are we still introduced in doing this?