Block Directory REST API cleanup

[dd32]

Description

Fixes #17664, fixes #17535.

This PR contains a lot of changes, which is my first pass at the Block Directory rest api endpoints.

In short, the major components of this include:

• A better permission_callback for each endpoint that calls the more fine-grained capabilities
• Doesn't re-create WP_Error objects when returning from called functions (which also fixes some fatal errors in the error-handling branches)
• Handles the installation process better using the $plugin_slug as the unique identifier internally, removing the need for multiple WordPress.org API calls
• Uses plugins_api() where possible
• Updates it to also allow block installation where FTP is in use, and the credentials are hard-coded (Still doesn't support the user entering FTP credentials, which I believe we don't really want to support).

There's a TODO remaining in the content where a change needs to be made on api.wordpress.org before the code can be altered.

How has this been tested?

1. Inserting Blocks that aren't installed into a post, and verifying that the REST API endpoints behave as expected
2. That the Uninstall hook is triggered and cleans up the plugins as expected.

Types of changes

This change is only code cleanup and bug fixes.

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.

[dd32]

This covers some parts of #17440

[Soean]

Fixes #17664 and #17535

[TimothyBJacobs]

So is the only difference between querying for blocks vs plugins from .org the use of the of block query arg instead of search?

[talldan]

Not all that familiar with the REST API, and I haven't tested anything, but I left a few minor comments.

[talldan]

The description of this function is the same as get_items_permissions_check. Would be great to detail why/how they're different.

[dd32]

The difference is very minimal, and I'm not sure how best to explain, other than the code..

It's possible to a user to be able to "get" plugins and potentially be able to install/activate them, but it's only once they actually attempt to install a singular item that the true capability checks can be processed.

[talldan]

Looking at the code, it seems like this permissions check is now only used for retrieving a list of plugins, so I wondered whether the current_user_can( 'activate_plugins' ) check still relevant in this function?

[TimothyBJacobs]

parse_block_metadata could be renamed to prepare_item_for_response and return a WP_REST_Response object instantiated with a data array ( not stdClass ) which would better match existing WordPress Core behavior.

Chatting with @kadamwhite, ideally we'd separate plugin installation/deletion to a separate bare-bones plugins endpoint instead of including it with the block directory endpoints. I'll try and PR that separately.

[StevenDufresne]

parse_block_metadata could be renamed to prepare_item_for_response and return a WP_REST_Response object instantiated with a data array ( not stdClass ) which would better match existing WordPress Core behavior.

Would it be okay to add that to the Block Directory backlog and return to it in another PR?

There are some updates in this PR that will be immediately useful and since this feature is still experimental we are keen on iterating quickly. The risk is that this PR grows in scope and gets stale.

Also, if we are ok with that, is there anything else that needs addressing before we can merge?

[chrisvanpatten]

“Immediately useful” after sitting stale for three months? Feels like something which should be addressed now as so often these types of things get backlogged and never addressed, then we end up shipping code which doesn’t mirror other core behavior.

[StevenDufresne]

“Immediately useful” after sitting stale for three months? Feels like something which should be addressed now as so often these types of things get backlogged and never addressed, then we end up shipping code which doesn’t mirror other core behavior.

To be clear, this PR updates already shipped code. For example, we have already shipped a bunch of invalid php in the form of fatal WP_Error statements missing the keyword new; addressed in this PR. We can continue to pick at this experimental Rest controller until its 100% inline with core in this PR if we think that's necessary, but i'll always champion working over perfection. Also, the breadth of this PR does brings a lot into question, which I imagine is why its three months stale.

So with that said, @chrisvanpatten, what do we need to do to get this over the line?

[TimothyBJacobs]

For the exact reasons you mention, REST API code that is merged should be in a shape that at least generally follows WordPress core patterns. There is a larger refactor that I mentioned in my comment, but that doesn't need to be completed right now.

IMO, the controllers should be update to use prepare_item_for_response before merging.

[StevenDufresne]

For the exact reasons you mention, REST API code that is merged should be in a shape that at least generally follows WordPress core patterns. There is a larger refactor that I mentioned in my comment, but that doesn't need to be completed right now.

IMO, the controllers should be update to use prepare_item_for_response before merging.

Okay sounds good. We'll get that change in.

[TimothyBJacobs]

Closing this out since it has been superseded by #22454 which contains these changed.