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

Pattern Directory: Create endpoints to proxy api.w.org/patterns. #26578

Merged

Conversation

iandunn
Copy link
Member

@iandunn iandunn commented Oct 29, 2020

Fixes #26577

Description

This adds API endpoints to browse and search for patterns. Similar to the Block Directory, the endpoints in Gutenberg proxy the api.wordpress.org endpoints, to prevent w.org from receiving the user's IP address.

  • http://{{ hostname }}/wp-json/wp/v2/pattern-directory/
  • http://{{ hostname }}/wp-json/wp/v2/pattern-directory/?category=2
  • http://{{ hostname }}/wp-json/wp/v2/pattern-directory/search?term=image

How has this been tested?

  • Manual testing with Insomnia
  • PHPUnit tests

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@iandunn iandunn force-pushed the add/pattern-directory-endpoint branch 3 times, most recently from 051ac57 to 196765f Compare November 19, 2020 22:57
@iandunn
Copy link
Member Author

iandunn commented Nov 19, 2020

@TimothyBJacobs , heads up about this, in case you have any early feedback or thoughts.

@iandunn
Copy link
Member Author

iandunn commented Nov 19, 2020

The phpunit-watcher stuff is just cherry-picked from #27058, for convenience. I'll remove it before publishing the PR.

@TimothyBJacobs
Copy link
Member

What's the difference between get_patterns and query_patterns? Will the response format be different based on that? Ideally, it wouldn't. The search endpoint should have a uniform schema.

@iandunn
Copy link
Member Author

iandunn commented Nov 20, 2020

The "get" endpoint is for browsing, like https://api.wordpress.org/patterns/1.0/?action=get_patterns or https://api.wordpress.org/patterns/1.0/?action=get_patterns&category=2.

The response format is different; the get_patterns endpoint on api.w.org are proxying a CPT's default endpoint, while query_patterns is proxying Core's search endpoint. There's also some workarounds due to https://core.trac.wordpress.org/ticket/49985 and https://core.trac.wordpress.org/ticket/49538.

The api.w.org code is at https://meta.trac.wordpress.org/browser/sites/trunk/api.wordpress.org/public_html/patterns/1.0/index.php?rev=10414

I imagined that prepare_item_for_response() would normalize the responses, though, so that searching and browsing would have the same schema.

@TimothyBJacobs
Copy link
Member

Ah, gotcha.

I imagined that prepare_item_for_response() would normalize the responses, though, so that searching and browsing would have the same schema.

That'd be great!

bazza pushed a commit to WordPress/wordpress.org that referenced this pull request Nov 20, 2020
The Core endpoint expects it, so that search and browse responses have identical schemas.

See WordPress/gutenberg#26578

git-svn-id: https://meta.svn.wordpress.org/sites/trunk@10459 74240141-8908-4e6f-9713-ba540dce6ec7
@iandunn iandunn force-pushed the add/pattern-directory-endpoint branch 4 times, most recently from 766b544 to 45c7dec Compare November 23, 2020 23:10
@iandunn iandunn marked this pull request as ready for review November 23, 2020 23:42
@iandunn
Copy link
Member Author

iandunn commented Nov 23, 2020

The e2e test failure looks unrelated.

@iandunn
Copy link
Member Author

iandunn commented Nov 30, 2020

Should this go behind a feature flag or experimental namespace until the other parts of the pattern directory are implemented?

@TimothyBJacobs
Copy link
Member

Should this go behind a feature flag or experimental namespace until the other parts of the pattern directory are implemented?

From a REST API perspective, I think it should have an experimental namespace. Whether it also needs to be behind a feature flag, I'm not sure. Maybe @youknowriad has thoughts about which features warrant that behavior.

Copy link
Member

@TimothyBJacobs TimothyBJacobs left a comment

Choose a reason for hiding this comment

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

Some minor bits of feedback.

lib/class-wp-rest-pattern-directory-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-pattern-directory-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-pattern-directory-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-pattern-directory-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-pattern-directory-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-pattern-directory-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-pattern-directory-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-pattern-directory-controller.php Outdated Show resolved Hide resolved
@TimothyBJacobs
Copy link
Member

Is there a bit more context of how the patterns structure is setup on .org? Looking just at https://meta.trac.wordpress.org/browser/sites/trunk/api.wordpress.org/public_html/patterns/1.0/index.php, I'm not sure why those need to be two different endpoints if the search endpoint is only looking in the wporg-pattern subtype. The purpose of the search controller is to allow for querying multiple/all subtypes at the same time.

@iandunn
Copy link
Member Author

iandunn commented Dec 16, 2020

Thanks for the review!

I think it should have an experimental namespace

👍🏻

That, and all the line-specific comments should be addressed in a45327f9f540356ce7b5f6ce402b9694a9704fd2.

I'm not sure why those need to be two different endpoints if the search endpoint is only looking in the wporg-pattern subtype. The purpose of the search controller is to allow for querying multiple/all subtypes at the same time.

Ah, that makes sense, I'll work on simplifying that. Thanks!

bazza pushed a commit to WordPress/wordpress.org that referenced this pull request Jan 12, 2021
Only one post type is being searched, so this is much simpler.

See WordPress/gutenberg#26578 (comment)

git-svn-id: https://meta.svn.wordpress.org/sites/trunk@10572 74240141-8908-4e6f-9713-ba540dce6ec7
@iandunn
Copy link
Member Author

iandunn commented Jan 13, 2021

I updated api.w.org to no longer use wp/v2/search in https://meta.trac.wordpress.org/changeset/10572/, and
2de7231c96f8cfecfdf25150129cec03ae752d5a updates this PR in light of that.

@TimothyBJacobs
Copy link
Member

Do we still need two separate endpoints in Gutenberg at this point?

@annezazu annezazu added the [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced label Jan 13, 2021
@TimothyBJacobs
Copy link
Member

f68389b consolodates the /search endpoint into /

face663 and 9078eb7 are related. I think this is more appropriate than using term, but am curious what you think @TimothyBJacobs

This seems good to me.

@iandunn
Copy link
Member Author

iandunn commented Jan 14, 2021

those locally-registered patterns should also be returned by the API.

Ah, 👍🏻

The schemas will need to match.

Local patterns have Title, content, description, viewportWidth, categories, keywords, so we should probably add keywords to the w.org API. None of the Core patterns use it, but I'm assuming it's still desired.

Locally, categories is an array of slugs, rather than IDs, so we should probably return that instead. Although, local pattern would call register_block_pattern_category(), which asks for a label in addition to the slug. Maybe we don't need to handle that here, though, since that'd be handled by whatever code eventually "installs" the w.org pattern locally? That might depend on whether or not we have that "install" flow; IIRC that's not finalized (cc @kellychoffman, @DaisyOlsen)?

@ryelle
Copy link
Contributor

ryelle commented Jan 14, 2021

There really isn't a local install flow, the block code is just inserted when you click the pattern. (there has been some discussion about saving/favoriting patterns on wp.org, but we don't have that wp.org user connection on local WP sites, so the idea's been pushed off to later iterations)

You make a good point about categories though, we'll need some way of getting the categories list off wp.org + the local site to populate the category dropdown here:

Screen Shot 2021-01-14 at 1 26 38 PM

On the wp.org side I've been assuming that's a static list of categories, but I don't think we want to make that assumption in the editor - that way we can add categories to wp.org without having to change Gutenberg.

@iandunn
Copy link
Member Author

iandunn commented Jan 14, 2021

we'll need some way of getting the categories list off wp.org + the local site to populate the category dropdown

👍🏻 , I think that'd be best scoped to a separate PR. I opened #28204 to track it.

@TimothyBJacobs
Copy link
Member

In light of that, I think we might want to go back to pattern-directory/search and have a pattern-directory/categories endpoint that would return the list of categories.

@iandunn
Copy link
Member Author

iandunn commented Jan 14, 2021

we might want to go back to pattern-directory/search

Do you mean renaming the current / to /search, or splitting them back into / for browsing and /search for searching?

@TimothyBJacobs
Copy link
Member

Renaming the current / to /search, or another name, but so we have "room" for a /categories endpoint.

@iandunn
Copy link
Member Author

iandunn commented Jan 15, 2021

👍🏻 , I'll do that, and normalize the local/remote schemas.

I don't think we should combine the local/remote patterns in this PR, though, since it opens a can of worms. I added #28248 for that. I think we should merge this once any final requests are done, and do the rest in separate PRs.

@iandunn
Copy link
Member Author

iandunn commented Jan 19, 2021

That latest batch of commits depends on some updates to w.org/patterns and api.w.org that I haven't finished & deployed yet. the test fixtures are updates to reflect the upcoming changes, though.

i'll leave a note when the w.org endpoints are updated.

@iandunn
Copy link
Member Author

iandunn commented Jan 19, 2021

@TimothyBJacobs , is 5b92da59f52e91b847c2ea1312061472ea27855d the right way to do validation for the response? I dug through the docs, examples, etc, but just confused myself, and couldn't get sanitize_callback working. Is that only for the request?

@TimothyBJacobs
Copy link
Member

Yeah, all the built in validation/sanitization handling is for request data. We don't apply that level of sanitization in the block directory, but 🤷.

@iandunn
Copy link
Member Author

iandunn commented Jan 26, 2021

🤔 , it seems like a "better safe than sorry" situation to me. I wouldn't want Core to trust input any other remote APIs.

@iandunn
Copy link
Member Author

iandunn commented Jan 26, 2021

@TimothyBJacobs, Do you feel like this is ready to merge from an API perspective?

@youknowriad, would you like this behind a feature flag?

Copy link
Member

@TimothyBJacobs TimothyBJacobs left a comment

Choose a reason for hiding this comment

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

Left some small bits of feedback.

lib/class-wp-rest-pattern-directory-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-pattern-directory-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-pattern-directory-controller.php Outdated Show resolved Hide resolved
@youknowriad
Copy link
Contributor

@youknowriad, would you like this behind a feature flag?

Since this is a PHP change that is only backported explicitly into Core. I don't think it should necessarily be behind a feature flag. We can include in Core when we think it's ready or when it's being used by the frontend.

@iandunn
Copy link
Member Author

iandunn commented Jan 26, 2021

Thanks, that sounds good 👍🏻

Copy link
Member

@TimothyBJacobs TimothyBJacobs left a comment

Choose a reason for hiding this comment

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

Looks great to me!

Similar to the Block Directory, the endpoints in Gutenberg proxy the api.wordpress.org endpoints, to prevent w.org from receiving the user's IP address. This will also be necessary in the future to combine locally-registered patterns with those hosted in the wordpress.org directory.

Fixes WordPress#26577
@iandunn iandunn force-pushed the add/pattern-directory-endpoint branch from cc5ad75 to 5303e4e Compare January 28, 2021 15:50
@iandunn
Copy link
Member Author

iandunn commented Jan 28, 2021

👍🏻 thanks for the review!

I squashed all the WIP commits down, and rebased against master. Is there anything else needed to merge?

@TimothyBJacobs TimothyBJacobs merged commit 3ff214f into WordPress:master Jan 28, 2021
@github-actions github-actions bot added this to the Gutenberg 9.9 milestone Jan 28, 2021
@TimothyBJacobs
Copy link
Member

Looks great, merged.

@iandunn
Copy link
Member Author

iandunn commented Jan 28, 2021

Thanks!

@mtias
Copy link
Member

mtias commented Feb 5, 2021

Awesome to see this progress :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pattern Directory: Add REST API endpoints
6 participants