Workspaces can't be published from the command line

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.

Rerolled and updated the patch with the new way of doing events, using class names instead of special constants.

The testbot is really strict these days :)

@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

+++ 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.

WorkspacePublisher is marked as @internal but I guess it can't hurt to add the BC layer. Fixed #34 and updated the CR to mention it.

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

Thanks for reviewing @larowlan, fixed all the points from #36 :)

Fixed #37.. still getting used to this stuff.

The last submitted patch, 38: 3242564-38.patch, failed testing. View results →

Looks like a random fail.

The last submitted patch, 38: 3242564-38.patch, failed testing. View results →

+++ 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?

1. I think it makes sense for the publish exception to extent access exception, because the reason for not being able to publish a workspace could very well be access-related.

2. Sure thing, done :)

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

Automatically closed - issue fixed for 2 weeks with no activity.

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 🐛 WorkspaceSubscriber service parameter $workspaceAssociation must be optional Needs work .

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.

@stefan.korn, I replied in #3411308-10: WorkspaceSubscriber service parameter $workspaceAssociation must be optional → .