From 81ed785a58d3243e9c0ee63c32e8fbfd130b9cbe Mon Sep 17 00:00:00 2001 From: Kelly Dwan Date: Tue, 16 Jun 2020 15:18:37 -0400 Subject: [PATCH 1/5] Block Directory: Use plugin API for installing & deleting block-plugins --- packages/block-directory/src/store/actions.js | 22 ++++--- .../block-directory/src/store/test/actions.js | 65 ++++++++++++++++--- 2 files changed, 67 insertions(+), 20 deletions(-) diff --git a/packages/block-directory/src/store/actions.js b/packages/block-directory/src/store/actions.js index f14fb51b177454..bc09c9d1baa84d 100644 --- a/packages/block-directory/src/store/actions.js +++ b/packages/block-directory/src/store/actions.js @@ -66,14 +66,15 @@ export function* installBlockType( block ) { throw new Error( __( 'Block has no assets.' ) ); } yield setIsInstalling( block.id, true ); - yield apiFetch( { - path: '__experimental/block-directory/install', + const response = yield apiFetch( { + path: '__experimental/plugins', data: { slug: block.id, + status: 'active', }, method: 'POST', } ); - yield addInstalledBlockType( block ); + yield addInstalledBlockType( { ...block, plugin: response.plugin } ); yield loadAssets( assets ); const registeredBlocks = yield select( 'core/blocks', 'getBlockTypes' ); @@ -94,18 +95,19 @@ export function* installBlockType( block ) { * @param {Object} block The blockType object. */ export function* uninstallBlockType( block ) { - const { id } = block; + const plugin = block.plugin.replace( '.php', '' ); try { - const response = yield apiFetch( { - path: '__experimental/block-directory/uninstall', + yield apiFetch( { + path: `__experimental/plugins/${ plugin }`, data: { - slug: id, + status: 'inactive', }, + method: 'PUT', + } ); + yield apiFetch( { + path: `__experimental/plugins/${ plugin }`, method: 'DELETE', } ); - if ( response !== true ) { - throw new Error( __( 'Unable to uninstall this block.' ) ); - } yield removeInstalledBlockType( block ); } catch ( error ) { yield dispatch( diff --git a/packages/block-directory/src/store/test/actions.js b/packages/block-directory/src/store/test/actions.js index 4ecb769260b448..c40aa876f07cb0 100644 --- a/packages/block-directory/src/store/test/actions.js +++ b/packages/block-directory/src/store/test/actions.js @@ -10,6 +10,13 @@ describe( 'actions', () => { assets: [ 'script.js' ], }; + const plugin = { + plugin: 'block/block.php', + status: 'active', + name: 'Test Block', + version: '1.0.0', + }; + describe( 'installBlockType', () => { it( 'should install a block successfully', () => { const generator = installBlockType( item ); @@ -28,11 +35,16 @@ describe( 'actions', () => { expect( generator.next().value ).toMatchObject( { type: 'API_FETCH', + request: { + path: '__experimental/plugins', + method: 'POST', + }, } ); - expect( generator.next( { success: true } ).value ).toEqual( { + const itemWithPlugin = { ...item, plugin: plugin.plugin }; + expect( generator.next( plugin ).value ).toEqual( { type: 'ADD_INSTALLED_BLOCK_TYPE', - item, + item: itemWithPlugin, } ); expect( generator.next().value ).toEqual( { @@ -102,6 +114,10 @@ describe( 'actions', () => { expect( generator.next().value ).toMatchObject( { type: 'API_FETCH', + request: { + path: '__experimental/plugins', + method: 'POST', + }, } ); const apiError = { @@ -128,16 +144,32 @@ describe( 'actions', () => { } ); describe( 'uninstallBlockType', () => { + const itemWithPlugin = { ...item, plugin: plugin.plugin }; + it( 'should uninstall a block successfully', () => { - const generator = uninstallBlockType( item ); + const generator = uninstallBlockType( itemWithPlugin ); + + // First the deactivation step + expect( generator.next().value ).toMatchObject( { + type: 'API_FETCH', + request: { + path: '__experimental/plugins/block/block', + method: 'PUT', + }, + } ); + // Then the deletion step expect( generator.next().value ).toMatchObject( { type: 'API_FETCH', + request: { + path: '__experimental/plugins/block/block', + method: 'DELETE', + }, } ); - expect( generator.next( true ).value ).toEqual( { + expect( generator.next().value ).toEqual( { type: 'REMOVE_INSTALLED_BLOCK_TYPE', - item, + item: itemWithPlugin, } ); expect( generator.next() ).toEqual( { @@ -146,19 +178,32 @@ describe( 'actions', () => { } ); } ); - it( "should set a global notice if the plugin can't uninstall", () => { - const generator = uninstallBlockType( item ); + it( "should set a global notice if the plugin can't be deleted", () => { + const generator = uninstallBlockType( itemWithPlugin ); expect( generator.next().value ).toMatchObject( { type: 'API_FETCH', + request: { + path: '__experimental/plugins/block/block', + method: 'PUT', + }, + } ); + + expect( generator.next().value ).toMatchObject( { + type: 'API_FETCH', + request: { + path: '__experimental/plugins/block/block', + method: 'DELETE', + }, } ); const apiError = { - code: 'could_not_remove_plugin', - message: 'Could not fully remove the plugin .', + code: 'rest_cannot_delete_active_plugin', + message: + 'Cannot delete an active plugin. Please deactivate it first.', data: null, }; - expect( generator.next( apiError ).value ).toMatchObject( { + expect( generator.throw( apiError ).value ).toMatchObject( { type: 'DISPATCH', actionName: 'createErrorNotice', storeKey: 'core/notices', From 7d38eb1e422ccbdb9df4629960cfab556faad387 Mon Sep 17 00:00:00 2001 From: Kelly Dwan Date: Tue, 16 Jun 2020 15:19:08 -0400 Subject: [PATCH 2/5] Remove the install & uninstall endpoints from the block-directory --- ...ass-wp-rest-block-directory-controller.php | 143 +----------------- ...p-rest-block-directory-controller-test.php | 40 +---- 2 files changed, 3 insertions(+), 180 deletions(-) diff --git a/lib/class-wp-rest-block-directory-controller.php b/lib/class-wp-rest-block-directory-controller.php index 7ffa52a1fd767e..1a04e380fd8588 100644 --- a/lib/class-wp-rest-block-directory-controller.php +++ b/lib/class-wp-rest-block-directory-controller.php @@ -41,36 +41,6 @@ public function register_routes() { 'schema' => array( $this, 'get_public_item_schema' ), ) ); - - register_rest_route( - $this->namespace, - '/' . $this->rest_base . '/install', - array( - 'methods' => WP_REST_Server::CREATABLE, - 'callback' => array( $this, 'create_item' ), - 'permission_callback' => array( $this, 'create_item_permissions_check' ), - 'args' => array( - 'slug' => array( - 'required' => true, - ), - ), - ) - ); - - register_rest_route( - $this->namespace, - '/' . $this->rest_base . '/uninstall', - array( - 'methods' => WP_REST_Server::DELETABLE, - 'callback' => array( $this, 'delete_item' ), - 'permission_callback' => array( $this, 'delete_item_permissions_check' ), - 'args' => array( - 'slug' => array( - 'required' => true, - ), - ), - ) - ); } /** @@ -136,117 +106,6 @@ public function get_items( $request ) { return rest_ensure_response( $result ); } - /** - * Checks whether a given request has permission to install and activate plugins. - * - * @since 5.5.0 - * - * @param WP_REST_Request $request Full details about the request. - * - * @return WP_Error|bool True if the request has permission, WP_Error object otherwise. - */ - public function create_item_permissions_check( $request ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable - if ( ! current_user_can( 'install_plugins' ) || ! current_user_can( 'activate_plugins' ) ) { - return new WP_Error( - 'rest_block_directory_cannot_create', - __( 'Sorry, you are not allowed to install blocks.', 'gutenberg' ), - array( 'status' => rest_authorization_required_code() ) - ); - } - - return true; - } - - /** - * Installs and activates a plugin - * - * @since 5.5.0 - * - * @param WP_REST_Request $request Full details about the request. - * - * @return WP_Error|WP_REST_Response Response object on success, or WP_Error object on failure. - */ - public function create_item( $request ) { - require_once ABSPATH . 'wp-admin/includes/plugin.php'; - - $existing = $this->find_plugin_for_slug( $request['slug'] ); - - if ( $existing ) { - $activate = new WP_REST_Request( 'PUT', '/__experimental/plugins/' . substr( $existing, 0, - 4 ) ); - $activate->set_body_params( array( 'status' => 'active' ) ); - - return rest_do_request( $activate ); - } - - $inner_request = new WP_REST_Request( 'POST', '/__experimental/plugins' ); - $inner_request->set_body_params( - array( - 'slug' => $request['slug'], - 'status' => 'active', - ) - ); - - return rest_do_request( $inner_request ); - } - - /** - * Checks whether a given request has permission to remove/deactivate plugins. - * - * @since 5.5.0 - * - * @param WP_REST_Request $request Full details about the request. - * - * @return WP_Error|bool True if the request has permission, WP_Error object otherwise. - */ - public function delete_item_permissions_check( $request ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable - if ( ! current_user_can( 'delete_plugins' ) || ! current_user_can( 'deactivate_plugins' ) ) { - return new WP_Error( - 'rest_block_directory_cannot_delete', - __( 'Sorry, you are not allowed to uninstall blocks.', 'gutenberg' ), - array( 'status' => rest_authorization_required_code() ) - ); - } - - return true; - } - - /** - * Deactivates and deletes a plugin - * - * @since 5.5.0 - * - * @param WP_REST_Request $request Full details about the request. - * - * @return WP_Error|WP_REST_Response Response object on success, or WP_Error object on failure. - */ - public function delete_item( $request ) { - require_once ABSPATH . 'wp-admin/includes/plugin.php'; - - $slug = trim( $request->get_param( 'slug' ) ); - - if ( ! $slug ) { - return new WP_Error( 'slug_not_provided', 'Valid slug not provided.', array( 'status' => 400 ) ); - } - - $plugin_file = $this->find_plugin_for_slug( $slug ); - - if ( ! $plugin_file ) { - return new WP_Error( 'block_not_found', 'Valid slug not provided.', array( 'status' => 400 ) ); - } - - $route = '/__experimental/plugins/' . substr( $plugin_file, 0, - 4 ); - $deactivate = new WP_REST_Request( 'PUT', $route ); - $deactivate->set_body_params( array( 'status' => 'inactive' ) ); - - $deactivated = rest_do_request( $deactivate ); - - if ( $deactivated->is_error() ) { - return $deactivated->as_error(); - } - - return rest_do_request( new WP_REST_Request( 'DELETE', $route ) ); - } - /** * Parse block metadata for a block, and prepare it for an API repsonse. * @@ -277,7 +136,7 @@ public function prepare_item_for_response( $plugin, $request ) { 'assets' => array(), 'last_updated' => $plugin['last_updated'], 'humanized_updated' => sprintf( - /* translators: %s: Human-readable time difference. */ + /* translators: %s: Human-readable time difference. */ __( '%s ago', 'gutenberg' ), human_time_diff( strtotime( $plugin['last_updated'] ) ) ), diff --git a/phpunit/class-wp-rest-block-directory-controller-test.php b/phpunit/class-wp-rest-block-directory-controller-test.php index 1675fac5fa35c6..d3e285dffa4420 100644 --- a/phpunit/class-wp-rest-block-directory-controller-test.php +++ b/phpunit/class-wp-rest-block-directory-controller-test.php @@ -19,10 +19,6 @@ public static function wpSetUpBeforeClass( $factory ) { if ( is_multisite() ) { grant_super_admin( self::$admin_id ); } - - if ( ! defined( 'FS_METHOD' ) ) { - define( 'FS_METHOD', 'direct' ); - } } public static function wpTearDownAfterClass() { @@ -95,20 +91,7 @@ public function test_get_item() { } public function test_create_item() { - if ( isset( get_plugins()['hello-dolly/hello.php'] ) ) { - delete_plugins( array( 'hello-dolly/hello.php' ) ); - } - - wp_set_current_user( self::$admin_id ); - - $request = new WP_REST_Request( 'POST', '/__experimental/block-directory/install' ); - $request->set_body_params( array( 'slug' => 'hello-dolly' ) ); - - $response = rest_do_request( $request ); - $this->skip_on_filesystem_error( $response ); - $this->assertNotWPError( $response->as_error() ); - $this->assertEquals( 201, $response->get_status() ); - $this->assertEquals( 'Hello Dolly', $response->get_data()['name'] ); + $this->markTestSkipped( 'Controller does not have create_item route.' ); } public function test_update_item() { @@ -116,7 +99,7 @@ public function test_update_item() { } public function test_delete_item() { - $this->markTestSkipped( 'Covered by Plugins controller tests.' ); + $this->markTestSkipped( 'Controller does not have delete_item route.' ); } public function test_prepare_item() { @@ -172,25 +155,6 @@ public function test_get_item_schema() { $this->assertArrayHasKey( 'assets', $properties ); } - /** - * Skips the test if the response is an error due to the filesystem being unavailable. - * - * @since 5.5.0 - * - * @param WP_REST_Response $response The response object to inspect. - */ - protected function skip_on_filesystem_error( WP_REST_Response $response ) { - if ( ! $response->is_error() ) { - return; - } - - $code = $response->as_error()->get_error_code(); - - if ( 'fs_unavailable' === $code || false !== strpos( $code, 'mkdir_failed' ) ) { - $this->markTestSkipped( 'Filesystem is unavailable.' ); - } - } - /** * Simulate a network failure on outbound http requests to a given hostname. * From fa992a71104c6676d347fa79b2fd6a6b8dad7ac1 Mon Sep 17 00:00:00 2001 From: Kelly Dwan Date: Wed, 17 Jun 2020 19:54:22 -0400 Subject: [PATCH 3/5] Set the endpoint for the block by using the self link property --- packages/block-directory/src/store/actions.js | 8 ++--- .../block-directory/src/store/test/actions.js | 29 ++++++++++++------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/packages/block-directory/src/store/actions.js b/packages/block-directory/src/store/actions.js index bc09c9d1baa84d..a2da6b8d9794e7 100644 --- a/packages/block-directory/src/store/actions.js +++ b/packages/block-directory/src/store/actions.js @@ -74,7 +74,8 @@ export function* installBlockType( block ) { }, method: 'POST', } ); - yield addInstalledBlockType( { ...block, plugin: response.plugin } ); + const endpoint = response?._links?.self[ 0 ]?.href; + yield addInstalledBlockType( { ...block, endpoint } ); yield loadAssets( assets ); const registeredBlocks = yield select( 'core/blocks', 'getBlockTypes' ); @@ -95,17 +96,16 @@ export function* installBlockType( block ) { * @param {Object} block The blockType object. */ export function* uninstallBlockType( block ) { - const plugin = block.plugin.replace( '.php', '' ); try { yield apiFetch( { - path: `__experimental/plugins/${ plugin }`, + url: block.endpoint, data: { status: 'inactive', }, method: 'PUT', } ); yield apiFetch( { - path: `__experimental/plugins/${ plugin }`, + url: block.endpoint, method: 'DELETE', } ); yield removeInstalledBlockType( block ); diff --git a/packages/block-directory/src/store/test/actions.js b/packages/block-directory/src/store/test/actions.js index c40aa876f07cb0..a301a5ab283088 100644 --- a/packages/block-directory/src/store/test/actions.js +++ b/packages/block-directory/src/store/test/actions.js @@ -4,17 +4,24 @@ import { installBlockType, uninstallBlockType } from '../actions'; describe( 'actions', () => { + const endpoint = '/wp-json/__experimental/plugins/block/block'; const item = { id: 'block/block', name: 'Test Block', assets: [ 'script.js' ], }; - const plugin = { plugin: 'block/block.php', status: 'active', name: 'Test Block', version: '1.0.0', + _links: { + self: [ + { + href: endpoint, + }, + ], + }, }; describe( 'installBlockType', () => { @@ -41,10 +48,10 @@ describe( 'actions', () => { }, } ); - const itemWithPlugin = { ...item, plugin: plugin.plugin }; + const itemWithEndpoint = { ...item, endpoint }; expect( generator.next( plugin ).value ).toEqual( { type: 'ADD_INSTALLED_BLOCK_TYPE', - item: itemWithPlugin, + item: itemWithEndpoint, } ); expect( generator.next().value ).toEqual( { @@ -144,16 +151,16 @@ describe( 'actions', () => { } ); describe( 'uninstallBlockType', () => { - const itemWithPlugin = { ...item, plugin: plugin.plugin }; + const itemWithEndpoint = { ...item, endpoint }; it( 'should uninstall a block successfully', () => { - const generator = uninstallBlockType( itemWithPlugin ); + const generator = uninstallBlockType( itemWithEndpoint ); // First the deactivation step expect( generator.next().value ).toMatchObject( { type: 'API_FETCH', request: { - path: '__experimental/plugins/block/block', + url: endpoint, method: 'PUT', }, } ); @@ -162,14 +169,14 @@ describe( 'actions', () => { expect( generator.next().value ).toMatchObject( { type: 'API_FETCH', request: { - path: '__experimental/plugins/block/block', + url: endpoint, method: 'DELETE', }, } ); expect( generator.next().value ).toEqual( { type: 'REMOVE_INSTALLED_BLOCK_TYPE', - item: itemWithPlugin, + item: itemWithEndpoint, } ); expect( generator.next() ).toEqual( { @@ -179,12 +186,12 @@ describe( 'actions', () => { } ); it( "should set a global notice if the plugin can't be deleted", () => { - const generator = uninstallBlockType( itemWithPlugin ); + const generator = uninstallBlockType( itemWithEndpoint ); expect( generator.next().value ).toMatchObject( { type: 'API_FETCH', request: { - path: '__experimental/plugins/block/block', + url: endpoint, method: 'PUT', }, } ); @@ -192,7 +199,7 @@ describe( 'actions', () => { expect( generator.next().value ).toMatchObject( { type: 'API_FETCH', request: { - path: '__experimental/plugins/block/block', + url: endpoint, method: 'DELETE', }, } ); From f7f2b19c3be969f33d8884eb3ab823b0f6466967 Mon Sep 17 00:00:00 2001 From: tellyworth Date: Tue, 23 Jun 2020 22:42:26 +1000 Subject: [PATCH 4/5] Remove erroneous route tests The install and uninstall routes no longer exist on this controller, as they've moved to the plugin controller. --- phpunit/class-wp-rest-block-directory-controller-test.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/phpunit/class-wp-rest-block-directory-controller-test.php b/phpunit/class-wp-rest-block-directory-controller-test.php index d3e285dffa4420..3abf7845553b71 100644 --- a/phpunit/class-wp-rest-block-directory-controller-test.php +++ b/phpunit/class-wp-rest-block-directory-controller-test.php @@ -29,8 +29,6 @@ public function test_register_routes() { $routes = rest_get_server()->get_routes(); $this->assertArrayHasKey( '/__experimental/block-directory/search', $routes ); - $this->assertArrayHasKey( '/__experimental/block-directory/install', $routes ); - $this->assertArrayHasKey( '/__experimental/block-directory/uninstall', $routes ); } public function test_context_param() { From fd8d9225f3f88ca58a5606978ca85b516c314a0e Mon Sep 17 00:00:00 2001 From: dufresnesteven Date: Wed, 24 Jun 2020 09:11:13 +0900 Subject: [PATCH 5/5] Update e2e install mock url to fix tests. --- .../e2e-tests/specs/experiments/block-directory-add.test.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/e2e-tests/specs/experiments/block-directory-add.test.js b/packages/e2e-tests/specs/experiments/block-directory-add.test.js index c3ad73ea7ea655..7c55976f65da5f 100644 --- a/packages/e2e-tests/specs/experiments/block-directory-add.test.js +++ b/packages/e2e-tests/specs/experiments/block-directory-add.test.js @@ -22,10 +22,8 @@ const SEARCH_URLS = [ ]; const INSTALL_URLS = [ - '/__experimental/block-directory/install', - `rest_route=${ encodeURIComponent( - '/__experimental/block-directory/install' - ) }`, + '/__experimental/plugins', + `rest_route=${ encodeURIComponent( '/__experimental/plugins' ) }`, ]; // Example Blocks