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.

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 REST endpoint providing post types, taxonomies, blocks, and widgets registered by the theme #5751

Closed
wants to merge 8 commits into from

Conversation

johnwatkins0
Copy link
Contributor

@johnwatkins0 johnwatkins0 commented Dec 30, 2020

Summary

Related to #4795 but does not resolve this issue because it doesn't provide any of the UI for the settings screen or onboarding wizard.

This sets up a REST endpoint, amp/v1/theme-entities. When in the admin with a logged-in user with dev tools turned on, a request to amp/v1/theme-entities will return an object that looks something like this:

{
    "blocks": [
        "my-theme/my-block"
    ],
    "post_types": [
        "theme-post-type"
    ],
    "taxonomies": [],
    "widgets": []
} 

Each of these fields will contain a list of names/slugs for any blocks, post types, taxonomies, widgets, registered by the theme.

How it works

The REST endpoint callback first gets all entities active on the site. Then it makes a nested request to the same endpoint with a context flag that disables the theme. Within that nested request all entities are again retrieved while the theme is disabled. Then array_diff is used to find the items that only exist when the theme is active. The result is cached using a cache key from the theme name and version.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@codecov
Copy link

codecov bot commented Dec 30, 2020

Codecov Report

Merging #5751 (ac3a475) into develop (7850cf2) will increase coverage by 0.26%.
The diff coverage is 98.41%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #5751      +/-   ##
=============================================
+ Coverage      74.14%   74.40%   +0.26%     
- Complexity      5498     5517      +19     
=============================================
  Files            201      202       +1     
  Lines          16653    16716      +63     
=============================================
+ Hits           12347    12438      +91     
+ Misses          4306     4278      -28     
Flag Coverage Δ Complexity Δ
javascript 73.34% <ø> (ø) 0.00 <ø> (ø)
php 74.44% <98.41%> (+0.27%) 0.00 <19.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
src/AmpWpPlugin.php 100.00% <ø> (ø) 8.00 <0.00> (ø)
src/Validation/ThemeEntitiesRESTController.php 98.41% <98.41%> (ø) 19.00 <19.00> (?)
includes/amp-helper-functions.php 83.24% <0.00%> (+0.50%) 0.00% <0.00%> (ø%)
src/Infrastructure/ServiceBasedPlugin.php 82.85% <0.00%> (+0.71%) 50.00% <0.00%> (ø%)
includes/class-amp-post-type-support.php 82.05% <0.00%> (+12.82%) 15.00% <0.00%> (ø%)
...options/class-amp-reader-theme-rest-controller.php 72.00% <0.00%> (+16.00%) 6.00% <0.00%> (ø%)
src/OptionsRESTController.php 77.77% <0.00%> (+19.75%) 17.00% <0.00%> (ø%)

@johnwatkins0 johnwatkins0 marked this pull request as ready for review December 31, 2020 17:18
@johnwatkins0 johnwatkins0 changed the title [WIP] Add REST endpoint providing post types, taxonomies, blocks, and widgets registered by the theme Add REST endpoint providing post types, taxonomies, blocks, and widgets registered by the theme Dec 31, 2020
@github-actions
Copy link
Contributor

Plugin builds for ac3a475 are ready 🛎️!

@swissspidy
Copy link
Collaborator

The REST endpoint callback first gets all entities active on the site. Then it makes a nested request to the same endpoint with a context flag that disables the theme. Within that nested request all entities are again retrieved while the theme is disabled.

Interesting approach!

Curious: what happens if a theme enables a post type configured by a plugin using add_theme_support()? Jetpack does that. I assume when I do add_theme_support( 'nova_menu_item' );, this endpoint will say it's coming from the theme? Kinda expected at this point I guess, but just wanted to point this out.

@westonruter westonruter added this to the v2.2 milestone Feb 12, 2021
@westonruter
Copy link
Member

westonruter commented Feb 12, 2021

Curious: what happens if a theme enables a post type configured by a plugin using add_theme_support()? Jetpack does that. I assume when I do add_theme_support( 'nova_menu_item' );, this endpoint will say it's coming from the theme? Kinda expected at this point I guess, but just wanted to point this out.

Interesting. In this case, I think the end result is largely the same, at least for the Reader themes that we support out of the box (the core themes), since none of the core themes would add theme support for any particular post types. In that case, the registered entities request with the theme disabled would return with any such post types omitted, meaning they would not be available in a Reader theme.

Then it makes a nested request to the same endpoint with a context flag that disables the theme. Within that nested request all entities are again retrieved while the theme is disabled.

Come to think of it, we can avoid the nested request to simplify things. We can just have the client make two requests: one with the theme enabled, and one with the theme disabled, and it can then compute the difference itself. Then we wouldn't need to worry about authenticating the loopback request. Also we in general have had problems with loopback requests failing for sites (see #4979), so the more we can eliminate them the better. (Then we won't need #3789.)

@westonruter
Copy link
Member

In that case, the registered entities request with the theme disabled would return with any such post types omitted, meaning they would not be available in a Reader theme.

As an alternative to the REST API request disabling the theme, we could also provide a parameter to switch the theme, as the Customizer does. In that case, we could see if a particular Reader theme would be problematic alongside an active primary theme. So instead of checking theme compatibility once when showing the list of Reader themes, we could eventually check the theme compatibility when selecting an individual Reader theme. This would be useful as more themes become AMP-compatible and are listed in Reader mode. This could be done as a future improvement.

@westonruter
Copy link
Member

I've given this some thought and I've realized that I think we can take a different approach which will allow us to detect the entities which were registered not only be the theme but by the plugins as well. And this can be done in a single request without having to turn off a theme/plugin. See #6516. So I'm closing this PR for now, although it may make sense to re-open if it turns out that the logic could serve as a starting point.

@westonruter westonruter closed this Aug 5, 2021
@westonruter westonruter removed this from the v2.2 milestone Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants