Problem/Motivation

Problem #1: Workspaces can no longer be published from the CLI (or via cron), because #3037136: Make Workspaces and Content Moderation work together introduced the requirement that a workspace can be published only if the current user has access to do that. The rationale for that change was that we needed a mechanism that would allow us to prevent the publication of a workspace if certain conditions are not met.

Problem #2: There is no way to react to the publication of a workspace. This issue affects current HEAD as well: the workspace publisher has to hard-code a call to WorkspaceAssociation::postPublish(), which can be avoided if the workspace association would react to a post-publish event instead.

Proposed resolution

Instead of relying on access checking, introduce an event that allows any subscriber to prevent the publishing process.

Remaining tasks

Review.

User interface changes

Nope.

API changes

- new WorkspaceEvents::WORKSPACE_PRE_PUBLISH and WorkspaceEvents::WORKSPACE_POST_PUBLISH events added.
- \Drupal\workspaces\WorkspaceAssociationInterface::postPublish() is deprecated in favor of reacting to the post-publish event.

Data model changes

Nope.

Release notes snippet

N/A.

CommentFileSizeAuthor
#44 interdiff-44.txt1.33 KBamateescu
#44 3242564-44.patch32.34 KBamateescu
#38 interdiff-38.txt1.25 KBamateescu
#38 3242564-38.patch32.31 KBamateescu
#37 interdiff-37.txt6.48 KBamateescu
#37 3242564-37.patch32.31 KBamateescu
#35 interdiff-35.txt1.65 KBamateescu
#35 3242564-35.patch32.73 KBamateescu
#32 interdiff-32.txt4.92 KBamateescu
#32 3242564-32.patch32.42 KBamateescu
#30 interdiff-30.txt577 bytesamateescu
#30 3242564-30.patch32.29 KBamateescu
#29 interdiff-29.txt9.98 KBamateescu
#29 3242564-29.patch32.34 KBamateescu
#28 3242564-nr-bot.txt144 bytesneeds-review-queue-bot
#26 3242564-26.patch32.01 KBamateescu
#16 3242564-16.patch29.43 KBamateescu
#26 interdiff-26.txt5.24 KBamateescu
#5 interdiff-5.txt4.43 KBamateescu
#26 3242564-26-test-only.patch1.54 KBamateescu
#16 interdiff-16.txt4.54 KBamateescu
#5 3242564-5.patch29.21 KBamateescu
#2 3242564.patch26.74 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Status: Active » Needs review
FileSize
26.74 KB

This should do it.

amateescu’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: 3242564.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
29.21 KB
4.43 KB

Fixed those fails.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

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.

Status: Reviewed & tested by the community » Needs work

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

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

Probably a random fail.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Status: Reviewed & tested by the community » Needs work

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

Spokje’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

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

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

Another random fail.

Status: Reviewed & tested by the community » Needs work

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

Spokje’s picture

Status: Needs work » Reviewed & tested by the community

Random test failure

amateescu’s picture

This is now a Drupal 9.4.x issue.

Status: Reviewed & tested by the community » Needs work

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

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

Random fail.

quietone’s picture

Version: 9.4.x-dev » 10.0.x-dev
quietone’s picture

Running tests on D10

larowlan’s picture

This feels like a task rather than a bug

amateescu’s picture

@larowlan the bug is problem #1 from the IS, workspaces can not be published from the CLI or cron, which is a regression introduced by #3037136: Make Workspaces and Content Moderation work together.

catch’s picture

Title: Add pre/post workspace publishing events » Move workspace publishing access implementation to a prePublish event instead

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

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

catch’s picture

Title: Move workspace publishing access implementation to a prePublish event instead » Workspaces can't be published from the command line
amateescu’s picture

Thanks for reviewing this!

Why not add the logic that's being moved from access to the event subscriber, to a protected method on this class?

Because the workspace publisher shouldn't know why or how Content Moderation decides if a workspace can be published or not, that logic is solely CM's responsibility. Additionally, other modules will also need the ability to signal that a workspace can't be published, for example alternative workflow modules that are not based on CM.

If there's a separate need for events, then that could be its own issue.

There is definitely a need to for these events, at the moment there's no way for other modules to react on workspace publishing, for example to keep track of which entities were published as part of a workspace publication. That example is needed for the revert functionality, among others.

As for adding events in its own issue, I thought we generally introduce a new API with at least one usage, and this is a great use-case because we're fixing a bug as well.

I think the rich API provided by this event warrants their use here, but I'm happy to explore alternatives if really needed :)

Also not sure why this needs a completely custom implementation, and couldn't use the entity validation API maybe?

I don't think so, wouldn't that prevent editing a workspace (e.g. its title) if CM says that it can't be published?

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.

That's right, there was no explicit test coverage for the CLI publishing bug, so I added one.

The last submitted patch, 26: 3242564-26-test-only.patch, failed testing. View results

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
144 bytes

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.

amateescu’s picture

Version: 10.0.x-dev » 10.1.x-dev
Status: Needs work » Needs review
Parent issue: » #2732071: WI: Workspace module roadmap
FileSize
32.34 KB
9.98 KB

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

amateescu’s picture

The testbot is really strict these days :)

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

@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

amateescu’s picture

Status: Needs work » Needs review
FileSize
32.42 KB
4.92 KB

Thanks for reviewing! Addressed #31, except adding an additional CR because WorkspaceOperationFactory is marked as @internal.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Think this ready now.

longwave’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
32.73 KB
1.65 KB

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.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

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

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
32.31 KB
6.48 KB

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

amateescu’s picture

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

Status: Reviewed & tested by the community » Needs work

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

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

Looks like a random fail.

Status: Reviewed & tested by the community » Needs work

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

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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?

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
32.34 KB
1.33 KB

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 :)

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

Saving issue credits

  • larowlan committed 686537e1 on 10.1.x
    Issue #3242564 by amateescu, larowlan, catch, Fabianx, longwave:...
larowlan’s picture

Status: Needs review » Fixed

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

Status: Fixed » Closed (fixed)

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