Remove @internal from workflowHasData/workflowStateHasData and use those methods for access control and configuration validation.

@Sam152 I really liked your suggestions in #2830740: Allow workflow types to lock certain changes to workflows once things are in use and think this would be good, it should not block stable, and should be fine to go in 8.5.x

+++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -46,38 +46,6 @@ public function label();
-  public function workflowHasData(WorkflowInterface $workflow);
...
-  public function workflowStateHasData(WorkflowInterface $workflow, StateInterface $state);

This is an API change, which means this would need to happen before the beta. If it happens post-beta, you'll at most be able to mark this @deprecated.

Does that mean this is WI Critical?

I think if this doesn't get in before 8.4.0 we update the patch to @deprecate these methods, until then we can assume it will be in 8.4.0 and we can delete these methods.

If that is the direction we want to go, IIRC it was discussed in #2830740: Allow workflow types to lock certain changes to workflows once things are in use that we keep those methods even if we move to an access based solution.

1. +++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
   @@ -119,7 +119,7 @@ public function form(array $form, FormStateInterface $form_state) {
   +      if (!in_array($state->id(), $workflow_type->getRequiredStates()) && count($states) !== 1) {
   

   This logic is now a duplicate of what is in \Drupal\workflows\WorkflowAccessControlHandler::checkAccess.

2. +++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
   @@ -264,6 +264,17 @@ public function save(array $form, FormStateInterface $form_state) {
   +    unset($actions['delete']['#access']);
   

   If we had specific a specific "delete workflows" permission I'd question this, but we only have "administer workflows", so I guess it's ok.

3. +++ b/core/modules/workflows/src/WorkflowTypeInterface.php
   @@ -46,38 +46,6 @@ public function label();
   -  public function workflowHasData(WorkflowInterface $workflow);
   ...
   -  public function workflowStateHasData(WorkflowInterface $workflow, StateInterface $state);
   

   So these are now ContentModeration specific methods?

#15: d'oh, I missed that — but that's great news! :)

#16: great points, thanks for the review! On point 2: note that contrib/custom code could alter the access checking for workflows, which means it could introduce additional permissions, which means your point/concern actually still stands.

#18: You'd be right if this was only potentially removing two @internal methods. But this patch is also removing WorkflowDeleteAccessCheck, which is a service that is not marked @internal. That change would not be allowed in 8.5, unless it'd be marked @internal. I think you could create a separate issue to mark that class @internal, and add a @todo Remove this service and class in https://www.drupal.org/node/2897148, i.e. refer back to this issue. Then this issue can safely remove it, or if it ends up changing direction, it could remove the @internal.

👍

RTBC'd.

Blocker has been resolved

@Sam152, I'm glad you changed your mind about the general usefulness of these methods :) Here's another place where we need them: #2904101: Do not allow modules that provide a workflow type plugin with existing data to be uninstalled

For #25.

I'm not sure I fully understand #24. Is it saying that rather than using the entity access system, it should use a \Drupal\Core\Config\ConfigEvents::IMPORT_VALIDATE event subscriber?

Perhaps it's because I've been out of Drupal for a week, but I'm really having a hard time grokking #24 — I'd appreciate it if you could elaborate. Or perhaps update the IS with your new thinking (while keeping the old thinking mentioned and explaining your reasons for changing — this also helps core committers, see #2824851-107: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior for example).

I think it makes sense to move the config import event subscriber to Workflows, I'm not sure why it was put in Content Moderation in the first place.

#28: thank you, that helped a lot!

1. +++ b/core/modules/workflows/src/WorkflowTypeInterface.php
   @@ -31,10 +31,6 @@ public function label();
   -   * @internal
   -   *   Marked as internal until it's validated this should form part of the
   -   *   public API in https://www.drupal.org/node/2897148.
   
   @@ -46,10 +42,6 @@ public function workflowHasData(WorkflowInterface $workflow);
   -   * @internal
   -   *   Marked as internal until it's validated this should form part of the
   -   *   public API in https://www.drupal.org/node/2897148.
   

   So we're officially making these non-internal. Makes sense. But can we then add @see \Drupal\workflows\EventSubscriber\ConfigImportSubscriber, to make it clear what this being used for?

2. +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Plugin/WorkflowType/HasDataTestType.php
   @@ -0,0 +1,35 @@
   +    $workflows_with_data = \Drupal::state()->get('workflows.workflows_with_data');
   ...
   +    $states_with_data = \Drupal::state()->get('workflows.workflow_states_with_data');
   

   Let's make these strings constants. Will make the test more robust.

So the patch in #31 is now just moving the config import validation from content_moderation to workflows and adding test coverage.

BUT…

+++ b/core/modules/workflows/src/Form/WorkflowDeleteForm.php
@@ -15,9 +15,10 @@ class WorkflowDeleteForm extends EntityConfirmFormBase {
-    if ($this->entity->getTypePlugin()->workflowHasData($this->entity)) {
+    $access = $this->entity->access('delete', NULL, TRUE);
+    if (!$access->isAllowed()) {
...
-      $form['description'] = ['#markup' => $this->t('This workflow is in use. You cannot remove this workflow until you have removed all content using it.')];

That this error message is still UI/form/render-specific.

Therefore when this action is attempted to be performed via REST in the future, it won't get a similar error message.

This is why it's wrong/problematic from an API-First point of view to have error checking + error messages hardcoded in the UI rather than in the underlying logic.

IOW: in HEAD, business logic is coupled to UI logic, this change is decoupling them, allowing the same logic to also be applied for API-First purposes.

Therefore I think we should also still do what the patches prior to the new approach were doing.

Sorry!

I'm glad these are @internal and we can take our time to sort it out.

Exactly! :)

Re #35:

So we're officially making these non-internal. Makes sense. But can we then add @see \Drupal\workflows\EventSubscriber\ConfigImportSubscriber, to make it clear what this being used for?

These methods are also used in </code> and #2904101: Do not allow modules that provide a workflow type plugin with existing data to be uninstalled adds another usage. Do we really need to add <code>@see to every usage of a public method? And if not, how do we determine which usages are more important than others?

Let's make these strings constants. Will make the test more robust.

I don't agree with this.. We use this pattern of state set/get in dozens of test and I've never seen any of them using constants..

As for the patch itself, am I the only one who thinks it's weird that both of these methods receive a workflow entity as their first argument? I would have expected to have that passed-in automatically somehow..

#48

+++ b/core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php
@@ -212,6 +233,33 @@ public function checkAccessProvider() {
+          ->addCacheContexts([]), // @todo, should these be uncacheable?
...
+          ->addCacheContexts([]), // @todo, should these be uncacheable?
...
+          ->addCacheContexts(['user.permissions']), // @todo, should these be uncacheable?

This is the last thing I'm unsure about. These decisions are based on calls to hasData methods. We don't know what cacheability exists behind the checks in these methods, so unless we were able to return metadata with these methods themselves, the whole access check is uncacheable.

\Drupal\content_moderation\Plugin\WorkflowType\ContentModeration::workflowHasData() and \Drupal\content_moderation\Plugin\WorkflowType\ContentModeration::workflowStateHasData() both do entity queries, and then return a boolean. What's missing here, are entity list cache tags — in this case: content_moderation_state_list.

This cache tag is technically associated with the boolean that is returned: the boolean might change whenever the list cache tag is invalidated. Which means that technically, the return value should not be bool but CacheableBool. With:

class CacheableBool implements RefinableCacheableDependencyInterface {

  use RefinableCacheableDependencyTrait;

  …
}

but doing that would change the signature of \Drupal\workflows\WorkflowTypeInterface::workflowHas(State)Data, and hence cause a BC break.

So you have two options:

1. The strategy used by \Drupal\Core\Access\AccessibleInterface::access(): add an optional $return_as_object = FALSE parameter, which by default returns just the boolean, and if the caller opts in, returns the boolean plus cacheability metadata.
2. Hardcode the list cache tag in the access control handler.

There's a few coding standards issues as mentioned in https://www.drupal.org/pift-ci-job/790541, but once done I think we're ready for RTBC.

Assigning to Sam152 to complete the updates.

Thanks @sam152!

1. +++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
   @@ -119,7 +119,10 @@ public function form(array $form, FormStateInterface $form_state) {
   +      if (!in_array($state->id(), $workflow_type->getRequiredStates()) && count($states) !== 1) {
   

   We should use the third arg for in_array here because these are strings

2. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
   @@ -46,14 +47,14 @@ public function label() {
   +  public function workflowHasData(WorkflowInterface $workflow, RefinableCacheableDependencyInterface $cacheability_metadata) {
   ...
   +  public function workflowStateHasData(WorkflowInterface $workflow, StateInterface $state, RefinableCacheableDependencyInterface $cacheability_metadata) {
   

   This is an API break on a non-internal class in a stable module, so we need to support passing NULL unfortunately. And we need tests for that too.

3. +++ b/core/modules/workflows/src/WorkflowAccessControlHandler.php
   @@ -54,17 +55,25 @@ public function __construct(EntityTypeInterface $entity_type, PluginManagerInter
   +    elseif ($operation === 'delete' && $workflow_type->workflowHasData($entity, $workflow_has_data_cacheability)) {
   ...
   +    else {
   

   I prefer returning early over if/elseif/else.

   I think that will make this tidier and easier to read

2. Ah sorry, I missed that the method was internal, I only checked the class - all good.

Patch does not apply to 9.3:

KristenBackupMBP:drupal-9.3.x-dev admin$ patch -p1 < 2897148-66.patch 
patching file core/modules/content_moderation/content_moderation.services.yml
Hunk #1 FAILED at 15.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/content_moderation/content_moderation.services.yml.rej
patching file core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
Hunk #2 succeeded at 111 (offset 2 lines).
Hunk #3 succeeded at 126 (offset 2 lines).
patching file core/modules/content_moderation/tests/src/Kernel/ContentModerationWorkflowConfigTest.php
Hunk #1 FAILED at 120.
Hunk #2 succeeded at 137 with fuzz 1 (offset 2 lines).
1 out of 2 hunks FAILED -- saving rejects to file core/modules/content_moderation/tests/src/Kernel/ContentModerationWorkflowConfigTest.php.rej
patching file core/modules/content_moderation/src/EventSubscriber/ConfigImportSubscriber.php
patching file core/modules/workflows/src/Form/WorkflowDeleteForm.php
patching file core/modules/workflows/src/Form/WorkflowEditForm.php
Hunk #1 succeeded at 118 with fuzz 2 (offset -1 lines).
Hunk #2 succeeded at 283 (offset 4 lines).
patching file core/modules/workflows/src/Form/WorkflowStateDeleteForm.php
patching file core/modules/workflows/src/Plugin/WorkflowTypeBase.php
patching file core/modules/workflows/src/WorkflowAccessControlHandler.php
The next patch would delete the file core/modules/workflows/src/WorkflowDeleteAccessCheck.php,

Re-rolled the patch in #66 and attached an diff file.

Thanks!

Tried to fix custom failures in the patch #77. Please review.

Re-rolled patch #77, fixed cs error.

Fixed fail tests, Please have a look.