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 plugins endpoints #22454

Merged
merged 25 commits into from
Jun 10, 2020

Conversation

TimothyBJacobs
Copy link
Member

Introduces a set of plugin endpoints, intended to compliment the block directory searching endpoints.

Brings tests and endpoint code from tellyworth/wordpress-develop/add/rest-api/block-directory.

For some reason it seems like the filesystem isn't available when running tests.

Description

How has this been tested?

This has been manually tested and has some test coverage. Needs more test coverage and coverage on multisite.

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.

@TimothyBJacobs TimothyBJacobs added REST API Interaction Related to REST API Core REST API Task Task for Core REST API efforts labels May 19, 2020
@TimothyBJacobs TimothyBJacobs self-assigned this May 19, 2020
@TimothyBJacobs TimothyBJacobs added the [Feature] Block Directory Related to the Block Directory, a repository of block plugins label May 19, 2020
@TimothyBJacobs

This comment has been minimized.

@noahtallen

This comment has been minimized.

@noahtallen

This comment has been minimized.

@TimothyBJacobs

This comment has been minimized.

@noahtallen

This comment has been minimized.

@noahtallen

This comment has been minimized.

@TimothyBJacobs

This comment has been minimized.

@noahtallen

This comment has been minimized.

@noahtallen

This comment has been minimized.

@TimothyBJacobs

This comment has been minimized.

@TimothyBJacobs

This comment has been minimized.

@noahtallen

This comment has been minimized.

@TimothyBJacobs

This comment has been minimized.

@noahtallen

This comment has been minimized.

@TimothyBJacobs
Copy link
Member Author

I pushed a commit that refactors the block directory install and uninstall endpoints to use the plugins controller. Ideally we'll drop those methods and the block directory would POST to the plugins endpoint.

One thing of note is the uninstall endpoint. It is performing a deactivation and uninstall in one request. As far as I know this can't happen in the WordPress UI typically. It can also lead to all sorts of weird errors since the request started with the plugin active and running its code, but ends the request without the plugin files. This would mean if that plugin would include any files on a hook that occurred after it got uninstalled, everything could blow up. I've left this for now, but I think we should drop it and do the separate status update to inactive and then delete.

@TimothyBJacobs
Copy link
Member Author

Also, I dropped the item specific permission checks for activating and deactivating since that will be handled by the plugins controller. The purpose of those permission checks now is just to generate an Allow header which wouldn't have the plugin slug available.

I've also fixed the single activate_plugin cap check. It was operating on the plugin slug, but is meant to operate on the whole plugin file. This means we can't preemptively check it before doing the install if we are going to activate it in the same request. But we check it once the install is complete.

@TimothyBJacobs
Copy link
Member Author

I've adjusted the tests to mark as skipped instead of erroring if they run into a permission error while we wait on #22515. The tests should all work if you run them locally.

I also started to add some multisite tests, but I couldn't get multisite tests to work using test-unit-php-multisite. AFAICT, it is still running the tests on a single-site setup and it seems like the ms-required and ms-excluded groups aren't respected. I've also marked those as skipped for now.

@TimothyBJacobs TimothyBJacobs marked this pull request as ready for review May 23, 2020 23:35
@spacedmonkey
Copy link
Member

Review complete.

@TimothyBJacobs
Copy link
Member Author

Thanks for the review @spacedmonkey! I've pushed commits or left comments for all of your feedback.

Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

I think it is good first version. Once merged we can do some testing and tweak as required.

@TimothyBJacobs
Copy link
Member Author

Once merged we can do some testing and tweak as required.

Definitely.

I've tested with the inserter and searching blocks and installing blocks all still works as normal in my testing. Going to go ahead and merge this.

@TimothyBJacobs TimothyBJacobs merged commit 26363cf into WordPress:master Jun 10, 2020
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 10, 2020
Copy link
Contributor

@StevenDufresne StevenDufresne left a comment

Choose a reason for hiding this comment

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

Once merged we can do some testing and tweak as required.

Definitely.

I've tested with the inserter and searching blocks and installing blocks all still works as normal in my testing. Going to go ahead and merge this.

I am able to reproduce the issue that i mentioned in my review in master now. I'm curious, how did you test?

error

error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core REST API Task Task for Core REST API efforts [Feature] Block Directory Related to the Block Directory, a repository of block plugins REST API Interaction Related to REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants