diff --git a/core/modules/content_moderation/content_moderation.services.yml b/core/modules/content_moderation/content_moderation.services.yml index 5035b67..921a43f 100644 --- a/core/modules/content_moderation/content_moderation.services.yml +++ b/core/modules/content_moderation/content_moderation.services.yml @@ -15,8 +15,3 @@ services: arguments: ['@content_moderation.moderation_information'] tags: - { name: access_check, applies_to: _content_moderation_latest_version } - content_moderation.config_import_subscriber: - class: Drupal\content_moderation\EventSubscriber\ConfigImportSubscriber - arguments: ['@config.manager', '@entity_type.manager'] - tags: - - { name: event_subscriber } diff --git a/core/modules/content_moderation/tests/src/Kernel/ContentModerationWorkflowConfigTest.php b/core/modules/content_moderation/tests/src/Kernel/ContentModerationWorkflowConfigTest.php index 931056e..85984b6 100644 --- a/core/modules/content_moderation/tests/src/Kernel/ContentModerationWorkflowConfigTest.php +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationWorkflowConfigTest.php @@ -120,7 +120,7 @@ public function testDeletingStateViaConfiguration() { catch (ConfigImporterException $e) { $this->assertEqual($e->getMessage(), 'There were errors validating the config synchronization.'); $error_log = $this->configImporter->getErrors(); - $expected = ['The moderation state Test two is being used, but is not in the source storage.']; + $expected = ['The state Test two is being used, but is not in the source storage.']; $this->assertEqual($expected, $error_log); } @@ -135,7 +135,7 @@ public function testDeletingStateViaConfiguration() { $this->assertEqual($e->getMessage(), 'There were errors validating the config synchronization.'); $error_log = $this->configImporter->getErrors(); $expected = [ - 'The moderation state Test two is being used, but is not in the source storage.', + 'The state Test two is being used, but is not in the source storage.', 'The workflow Editorial is being used, and cannot be deleted.', ]; $this->assertEqual($expected, $error_log); diff --git a/core/modules/content_moderation/src/EventSubscriber/ConfigImportSubscriber.php b/core/modules/workflows/src/EventSubscriber/ConfigImportSubscriber.php similarity index 93% rename from core/modules/content_moderation/src/EventSubscriber/ConfigImportSubscriber.php rename to core/modules/workflows/src/EventSubscriber/ConfigImportSubscriber.php index 930167f..ee2ade5 100644 --- a/core/modules/content_moderation/src/EventSubscriber/ConfigImportSubscriber.php +++ b/core/modules/workflows/src/EventSubscriber/ConfigImportSubscriber.php @@ -1,6 +1,6 @@ getTypePlugin()->getState($state_id); if ($workflow->getTypePlugin()->workflowStateHasData($workflow, $state)) { - $event->getConfigImporter()->logError($this->t('The moderation state @state_label is being used, but is not in the source storage.', ['@state_label' => $state->label()])); + $event->getConfigImporter()->logError($this->t('The state @state_label is being used, but is not in the source storage.', ['@state_label' => $state->label()])); } } } diff --git a/core/modules/workflows/src/WorkflowAccessControlHandler.php b/core/modules/workflows/src/WorkflowAccessControlHandler.php index 1e0456c..e3eb242 100644 --- a/core/modules/workflows/src/WorkflowAccessControlHandler.php +++ b/core/modules/workflows/src/WorkflowAccessControlHandler.php @@ -57,13 +57,17 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter if (strpos($operation, 'delete-state') === 0) { list(, $state_id) = explode(':', $operation, 2); // Deleting a state is editing a workflow, but also we should forbid - // access if there is only one state. - return AccessResult::allowedIf(count($entity->getTypePlugin()->getStates()) > 1) + // access if there is only one state, if the state is a required state or + // that state has data associated with it. + return AccessResult::allowedIf(count($workflow_type->getStates()) > 1) ->andIf(parent::checkAccess($entity, 'edit', $account)) ->andIf(AccessResult::allowedIf(!in_array($state_id, $workflow_type->getRequiredStates(), TRUE))) + ->andIf(AccessResult::allowedIf(!$workflow_type->workflowStateHasData($entity, $workflow_type->getState($state_id)))) ->addCacheableDependency($entity); } - + if ($workflow_type->workflowHasData($entity)) { + return AccessResult::neutral(); + } return parent::checkAccess($entity, $operation, $account); } diff --git a/core/modules/workflows/src/WorkflowTypeInterface.php b/core/modules/workflows/src/WorkflowTypeInterface.php index 5ef843b..ece2d79 100644 --- a/core/modules/workflows/src/WorkflowTypeInterface.php +++ b/core/modules/workflows/src/WorkflowTypeInterface.php @@ -27,25 +27,19 @@ public function label(); /** * Determines if the workflow is being has data associated with it. * - * @internal - * Marked as internal until it's validated this should form part of the - * public API in https://www.drupal.org/node/2897148. - * * @param \Drupal\workflows\WorkflowInterface $workflow * The workflow to check. * * @return bool * TRUE if the workflow is being used, FALSE if not. + * + * @see \Drupal\workflows\EventSubscriber\ConfigImportSubscriber */ public function workflowHasData(WorkflowInterface $workflow); /** * Determines if the workflow state has data associated with it. * - * @internal - * Marked as internal until it's validated this should form part of the - * public API in https://www.drupal.org/node/2897148. - * * @param \Drupal\workflows\WorkflowInterface $workflow * The workflow to check. * @param \Drupal\workflows\StateInterface $state @@ -53,6 +47,8 @@ public function workflowHasData(WorkflowInterface $workflow); * * @return bool * TRUE if the workflow state is being used, FALSE if not. + * + * @see \Drupal\workflows\EventSubscriber\ConfigImportSubscriber */ public function workflowStateHasData(WorkflowInterface $workflow, StateInterface $state); diff --git a/core/modules/workflows/tests/modules/workflow_type_test/config/schema/workflow_type_test.schema.yml b/core/modules/workflows/tests/modules/workflow_type_test/config/schema/workflow_type_test.schema.yml index 1d9e742..c67eac5 100644 --- a/core/modules/workflows/tests/modules/workflow_type_test/config/schema/workflow_type_test.schema.yml +++ b/core/modules/workflows/tests/modules/workflow_type_test/config/schema/workflow_type_test.schema.yml @@ -70,3 +70,20 @@ workflow.type_settings.predefined_states_workflow_test_type: sequence: label: 'Transitions' type: workflows.transition + +workflow.type_settings.has_data_workflow_type_test: + type: mapping + label: 'Has data workflow type test' + mapping: + states: + type: sequence + label: 'States' + sequence: + type: workflows.state + label: 'States' + transitions: + type: sequence + label: 'Transitions' + sequence: + label: 'Transitions' + type: workflows.transition diff --git a/core/modules/workflows/tests/modules/workflow_type_test/src/Plugin/WorkflowType/HasDataTestType.php b/core/modules/workflows/tests/modules/workflow_type_test/src/Plugin/WorkflowType/HasDataTestType.php new file mode 100644 index 0000000..f9774b4 --- /dev/null +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Plugin/WorkflowType/HasDataTestType.php @@ -0,0 +1,35 @@ +get('workflows.workflows_with_data'); + return !empty($workflows_with_data[$workflow->id()]); + } + + /** + * {@inheritdoc} + */ + public function workflowStateHasData(WorkflowInterface $workflow, StateInterface $state) { + $states_with_data = \Drupal::state()->get('workflows.workflow_states_with_data'); + return !empty($states_with_data[$workflow->id() . ':' . $state->id()]); + } + +} diff --git a/core/modules/workflows/tests/src/Kernel/ConfigImportSubscriberTest.php b/core/modules/workflows/tests/src/Kernel/ConfigImportSubscriberTest.php new file mode 100644 index 0000000..dccaebf --- /dev/null +++ b/core/modules/workflows/tests/src/Kernel/ConfigImportSubscriberTest.php @@ -0,0 +1,143 @@ +createTestWorkflow('with_data', 'With Data', [ + 'data' => 'Data', + 'no_data' => 'No data', + ]); + $this->createTestWorkflow('without_data', 'Without Data', [ + 'no_data' => 'No data', + ]); + // The "with_data" workflow and the "data" state will return TRUE for the + // associated hasData methods. The rest will not. + $this->container->get('state')->set('workflows.workflows_with_data', [ + 'with_data' => TRUE, + ]); + $this->container->get('state')->set('workflows.workflow_states_with_data', [ + 'with_data:data' => TRUE, + ]); + + $this->configStorageSync = $this->container->get('config.storage.sync'); + $this->copyConfig($this->container->get('config.storage'), $this->container->get('config.storage.sync')); + } + + /** + * @covers ::onConfigImporterValidate + */ + public function testDeletingStates() { + // Removing states without data will succeed, even if the workflow has data. + $this->alterConfigData('workflows.workflow.with_data', function(&$data) { + unset($data['type_settings']['states']['no_data']); + }); + $this->alterConfigData('workflows.workflow.without_data', function(&$data) { + unset($data['type_settings']['states']['no_data']); + }); + $this->configImporter()->reset()->import(); + + // Removing a state with data will cause an import exception. + $this->alterConfigData('workflows.workflow.with_data', function(&$data) { + unset($data['type_settings']['states']['data']); + }); + try { + $this->configImporter()->reset()->import(); + $this->fail(); + } + catch (ConfigImporterException $e) { + $this->assertEquals(['The state Data is being used, but is not in the source storage.'], $this->configImporter->getErrors()); + } + } + + /** + * @covers ::onConfigImporterValidate + */ + public function testDeletingWorkflows() { + // Deleting a workflow without data will succeed. + $this->configStorageSync->delete('workflows.workflow.without_data'); + $this->configImporter()->reset()->import(); + + // Deleting a workflow with data will cause an import exception. + $this->configStorageSync->delete('workflows.workflow.with_data'); + try { + $this->configImporter()->reset()->import(); + } + catch (ConfigImporterException $e) { + $this->assertEquals([ + 'The workflow With Data is being used, and cannot be deleted.', + ], $this->configImporter->getErrors()); + } + } + + /** + * Alter the values of an item of configuration and write to the storage sync. + * + * @param string $id + * The ID of the configuration object. + * @param callable $alter + * The alterations which should be made. + */ + protected function alterConfigData($id, $alter) { + $config_data = $this->config($id)->get(); + $alter($config_data); + $this->configStorageSync->write($id, $config_data); + } + + /** + * Create and save a test workflow of type "has_data_workflow_type_test". + * + * @param string $id + * The ID. + * @param string $label + * The label. + * @param array $states + * An array of states. + * + * @return \Drupal\workflows\WorkflowInterface + */ + protected function createTestWorkflow($id, $label, array $states) { + $workflow = Workflow::create([ + 'label' => $label, + 'id' => $id, + 'type' => 'has_data_workflow_type_test', + ]); + foreach ($states as $id => $label) { + $workflow->getTypePlugin()->addState($id, $label); + } + $workflow->save(); + return $workflow; + } + +} diff --git a/core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php b/core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php index cd4e7ce..5108d94 100644 --- a/core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php +++ b/core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php @@ -65,6 +65,15 @@ protected function setUp() { $this->createUser([]); $this->user = $this->createUser([]); $this->adminUser = $this->createUser(['administer workflows']); + + // The "with_data" workflow and the "with_data" state will return TRUE for + // the associated hasData methods. + $this->container->get('state')->set('workflows.workflows_with_data', [ + 'with_data' => TRUE, + ]); + $this->container->get('state')->set('workflows.workflow_states_with_data', [ + 'with_data:with_data' => TRUE, + ]); } /** @@ -102,10 +111,10 @@ public function testCheckCreateAccess() { * @covers ::checkAccess * @dataProvider checkAccessProvider */ - public function testCheckAccess($user, $operation, $result, $states_to_create = []) { + public function testCheckAccess($workflow_id, $user, $operation, $result, $states_to_create = []) { $workflow = Workflow::create([ - 'type' => 'workflow_type_test', - 'id' => 'test_workflow', + 'type' => 'has_data_workflow_type_test', + 'id' => $workflow_id, ]); $workflow->save(); $workflow_type = $workflow->getTypePlugin(); @@ -131,27 +140,32 @@ public function checkAccessProvider() { return [ 'Admin view' => [ + 'test_workflow', 'adminUser', 'view', AccessResult::allowed()->addCacheContexts(['user.permissions']), ], 'Admin update' => [ + 'test_workflow', 'adminUser', 'update', AccessResult::allowed()->addCacheContexts(['user.permissions']), ], 'Admin delete' => [ + 'test_workflow', 'adminUser', 'delete', AccessResult::allowed()->addCacheContexts(['user.permissions']), ], 'Admin delete only state' => [ + 'test_workflow', 'adminUser', 'delete-state:foo', AccessResult::neutral()->addCacheTags(['config:workflows.workflow.test_workflow']), ['foo' => FALSE], ], 'Admin delete one of two states' => [ + 'test_workflow', 'adminUser', 'delete-state:foo', AccessResult::allowed() @@ -160,6 +174,7 @@ public function checkAccessProvider() { ['foo' => FALSE, 'bar' => FALSE], ], 'Admin delete required state when there are >1 states' => [ + 'test_workflow', 'adminUser', 'delete-state:foo', AccessResult::allowed() @@ -168,6 +183,7 @@ public function checkAccessProvider() { ['foo' => TRUE, 'bar' => FALSE], ], 'User view' => [ + 'test_workflow', 'user', 'view', AccessResult::neutral() @@ -175,6 +191,7 @@ public function checkAccessProvider() { ->setReason("The 'administer workflows' permission is required."), ], 'User update' => [ + 'test_workflow', 'user', 'update', AccessResult::neutral() @@ -182,6 +199,7 @@ public function checkAccessProvider() { ->setReason("The 'administer workflows' permission is required."), ], 'User delete' => [ + 'test_workflow', 'user', 'delete', AccessResult::neutral() @@ -189,12 +207,14 @@ public function checkAccessProvider() { ->setReason("The 'administer workflows' permission is required."), ], 'User delete only state' => [ + 'test_workflow', 'user', 'delete-state:foo', AccessResult::neutral()->addCacheTags(['config:workflows.workflow.test_workflow']), ['foo' => FALSE], ], 'User delete one of two states' => [ + 'test_workflow', 'user', 'delete-state:foo', AccessResult::neutral() @@ -204,6 +224,7 @@ public function checkAccessProvider() { ['foo' => FALSE, 'bar' => FALSE], ], 'User delete required state when there are >1 states' => [ + 'test_workflow', 'user', 'delete-state:foo', AccessResult::neutral() @@ -212,6 +233,24 @@ public function checkAccessProvider() { ->setReason("The 'administer workflows' permission is required."), ['foo' => TRUE, 'bar' => FALSE], ], + 'Admin cannot delete state with data' => [ + 'with_data', + 'adminUser', + 'delete-state:with_data', + AccessResult::neutral() + ->addCacheTags(['config:workflows.workflow.with_data']) + ->addCacheContexts([]), // @todo, should these be uncacheable? + ['with_data' => FALSE], + ], + 'Admin cannot delete workflow with data' => [ + 'with_data', + 'adminUser', + 'delete', + AccessResult::neutral() + ->addCacheTags([]) + ->addCacheContexts([]), // @todo, should these be uncacheable? + ['foo' => FALSE], + ], ]; } diff --git a/core/modules/workflows/workflows.services.yml b/core/modules/workflows/workflows.services.yml index 7d32420..858660a 100644 --- a/core/modules/workflows/workflows.services.yml +++ b/core/modules/workflows/workflows.services.yml @@ -8,3 +8,8 @@ services: class: \Drupal\workflows\WorkflowDeleteAccessCheck tags: - { name: access_check, applies_to: _workflow_state_delete_access } + workflows.config_import_subscriber: + class: Drupal\workflows\EventSubscriber\ConfigImportSubscriber + arguments: ['@config.manager', '@entity_type.manager'] + tags: + - { name: event_subscriber }