Workspaces can't be published from the command line

RTBC + 1, I first had trouble understanding this patch and why it's needed, but after looking at it again at some time it made click:

- The code within content_moderation is mainly moved from the access hook to the new publish event
- Not using access means it's not bound to permissions (the IS makes that already clear)
- The publish event has a rich interface to stop the publish
- A once stopped publish cannot be restarted anymore (Is there a use-case for that? Probably not ...)

In addition to that you can now react to postPublish.

Back to RTBC per #6 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.

Random test failure

Running tests on D10

This feels like a task rather than a bug

Re-titling to make it a bit clearer what this issue is about.

+++ b/core/modules/workspaces/src/WorkspacePublisher.php
@@ -77,10 +88,15 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Con
    */
   public function publish() {
-    $publish_access = $this->sourceWorkspace->access('publish', NULL, TRUE);
-    if (!$publish_access->isAllowed()) {
-      $message = $publish_access instanceof AccessResultReasonInterface ? $publish_access->getReason() : '';
-      throw new WorkspaceAccessException($message);
+    if ($this->sourceWorkspace->hasParent()) {
+      throw new WorkspacePublishException('Only top-level workspaces can be published.');
+    }
+
+    $event = new WorkspacePublishEvent($this->sourceWorkspace);
+    $this->eventDispatcher->dispatch($event, WorkspaceEvents::WORKSPACE_PRE_PUBLISH);
+
+    if ($event->isPublishingStopped()) {
+      throw new WorkspacePublishException((string) $event->getPublishingStoppedReason());
     }
 
     if ($this->checkConflictsOnTarget()) {

It's not clear to me why we have to add an entirely new API via events to fix this bug. Why not add the logic that's being moved from access to the event subscriber, to a protected method on this class?

If there's a separate need for events, then that could be its own issue. However in general I've been pushing back against the addition of new events, due to issues like #2825358: Event class constants for event names can cause fatal errors when a module is installed and #2237831: Allow module services to specify hooks.

Also not sure why this needs a completely custom implementation, and couldn't use the entity validation API maybe? https://www.drupal.org/docs/drupal-apis/entity-api/entity-validation-api... although I realise publishing a workspace isn't the same as just saving it, but the entity validation API is decoupled from saving, so... maybe?

Also missing a test-only fail patch here - can't really see from reading the test coverage whether the original bug report is covered by the changes to coverage.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

@trigger_error('The event dispatcher service should be passed to WorkspaceOperationFactory::__construct() since 9.4.0. This will be required in Drupal 10.0.0.', E_USER_DEPRECATED);

+    arguments: ['@entity_type.manager', '@database', '@workspaces.manager', '@workspaces.association', '@cache_tags.invalidator', '@event_dispatcher']

1, Will have to be updated. A 2nd change record may be needed to announce this service has a new parameter. And added to the message.

getSubscribedEvents
2. Should be typehinted

3. Any new functions that return should be typehinted

Think this ready now.

+++ b/core/modules/workspaces/src/WorkspacePublisher.php
@@ -62,14 +71,17 @@ class WorkspacePublisher implements WorkspacePublisherInterface {
+    $this->eventDispatcher = $event_dispatcher;

We need to add backward compatibility here; the $event_dispatcher argument could be the source workspace if callers have not been updated, in which case we need to issue a deprecation.

Functionally this looks ready to go to me, however our standards for new code have changed since this was first created. I was tempted to fix these on commit but then it got to the point where a peer review felt appropriate.

Feel free to self RTBC after all these changes

1. +++ b/core/modules/content_moderation/src/EventSubscriber/WorkspaceSubscriber.php
   @@ -0,0 +1,109 @@
   +  /**
   +   * The entity type manager service.
   +   *
   +   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   +   */
   +  protected $entityTypeManager;
   +
   +  /**
   +   * The workspace association service.
   +   *
   +   * @var \Drupal\workspaces\WorkspaceAssociationInterface
   +   */
   +  protected $workspaceAssociation;
   +
   +  /**
   +   * Constructs a new WorkspaceSubscriber instance.
   +   *
   +   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   +   *   The entity type manager service.
   +   * @param \Drupal\workspaces\WorkspaceAssociationInterface $workspace_association
   +   *   The workspace association service.
   +   */
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, WorkspaceAssociationInterface $workspace_association) {
   +    $this->entityTypeManager = $entity_type_manager;
   +    $this->workspaceAssociation = $workspace_association;
   +  }
   

   I realise this patch predates the requirement, but as this is new code, we need to use constructor property promotion here now

2. +++ b/core/modules/content_moderation/src/EventSubscriber/WorkspaceSubscriber.php
   @@ -0,0 +1,109 @@
   +  public function onWorkspacePrePublish(WorkspacePublishEvent $event) {
   

   And need a return type hint here

3. +++ b/core/modules/content_moderation/src/EventSubscriber/WorkspaceSubscriber.php
   @@ -0,0 +1,109 @@
   +      // Find all workflows which are moderating entity types of the same type to
   

   nit; >80 here

4. +++ b/core/modules/workspaces/src/Event/WorkspacePublishEvent.php
   @@ -0,0 +1,115 @@
   +  protected $publishingStopped = FALSE;
   ...
   +  protected $publishingStoppedReason = '';
   

   Can we get property type hints here too

5. +++ b/core/modules/workspaces/src/Event/WorkspacePublishEvent.php
   @@ -0,0 +1,115 @@
   +   * @param \Drupal\workspaces\WorkspaceInterface $workspace
   ...
   +  public function __construct(WorkspaceInterface $workspace, array $published_revision_ids) {
   

   and here too for constructor property promotion

6. +++ b/core/modules/workspaces/src/Event/WorkspacePublishEvent.php
   @@ -0,0 +1,115 @@
   +  public function stopPublishing() {
   

   And a void return type here

7. +++ b/core/modules/workspaces/src/Event/WorkspacePublishEvent.php
   @@ -0,0 +1,115 @@
   +  public function setPublishingStoppedReason($reason) {
   

   Should we make this fluent? if not can we add a void return type hint

8. +++ b/core/modules/workspaces/src/WorkspaceAssociation.php
   @@ -263,4 +267,23 @@ public function initializeWorkspace(WorkspaceInterface $workspace) {
   +  public function onPostPublish(WorkspacePublishEvent $event) {
   

   Can we add void here too

+++ b/core/modules/workspaces/src/WorkspacePublisher.php
@@ -77,23 +93,28 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Con
-      throw new WorkspaceAccessException($message);
+    if ($this->sourceWorkspace->hasParent()) {
+      throw new WorkspacePublishException('Only top-level workspaces can be published.');

Should we have publish exception extend access exception? Someone may have code that is catching the access exception that will no longer catch from here? The fact we had to update our code here seems to indicate this is a potential BC break. If we don't want to have them extend that, we should at least add a second change notice with that detail.

+++ b/core/modules/workspaces/src/Event/WorkspacePublishEvent.php
@@ -0,0 +1,112 @@
+  public function setPublishingStoppedReason($reason): static {

Can we typehint $reason at all? string|\Stringable perhaps?

Saving issue credits

Whoops, didn't mean to update the status.

Committed to 10.1.x.

Not backporting because of the new API/deprecations.

Published the change notice

What is the deal about having the second argument optional here:

  content_moderation.workspace_subscriber:
    class: Drupal\content_moderation\EventSubscriber\WorkspaceSubscriber
    arguments: ['@entity_type.manager', '@?workspaces.association']

but required there:

class WorkspaceSubscriber implements EventSubscriberInterface {
...
public function __construct(
    protected readonly EntityTypeManagerInterface $entityTypeManager,
    protected readonly WorkspaceAssociationInterface $workspaceAssociation
  ) {}

as pointed out in #3411308: WorkspaceSubscriber service parameter $workspaceAssociation must be optional.

I came across this when devel quits the event info with an exception.

Actually

Drupal::service('event_dispatcher')->getListeners();

does not seem to work because of the difference between service definition and class implementation.