Problem/Motivation

During cron processing, the scheduler 'PUBLISH' event and Rules Integration 'SchedulerHasPublishedThisNodeEvent' event are dispatched before the node is actually saved. These should be moved so that they happen after saving. The status has already been changed, so there is no difference there. But logically these events should be done after saving.

Same goes for the Unpublish events.

CommentFileSizeAuthor
#4 3196989-4.interdiff-2-4.txt1.01 KBjonathan1055
#4 3196989-4.dispatch-after-save.patch4.13 KBjonathan1055
#2 3196989-2.dispatch-after-save.patch3.12 KBjonathan1055

Issue fork scheduler-3196989

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review
StatusFileSize
new 3196989-2.dispatch-after-save.patch3.12 KB

First patch makes the change. This will fail, due to two event watchers accidentally not having a 'save' when they need it.

Status: Needs review » Needs work

The last submitted patch, 2: 3196989-2.dispatch-after-save.patch, failed testing. View results

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 3196989-4.dispatch-after-save.patch4.13 KB
new 3196989-4.interdiff-2-4.txt1.01 KB

As expected the tests fail. Here's the full patch and interdiff.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

This change has a subtle side effect. Before this change anything listening to \Drupal\scheduler\SchedulerEvents::UNPUBLISH or \Drupal\scheduler\SchedulerEvents::PUBLISH could make a change to a node and then rely on these actions saving the node for them.

Additionally, looking at the following code:

        if ($failed) {
          // At least one hook function returned a failure or exception, so stop
          // processing this node and move on to the next one.
          $this->logger->warning('Unpublishing failed for %title. Calls to @hook returned a failure code.', $logger_variables);
          continue;
        }
        elseif ($processed) {
          // The node has 'unpublishing' processed by a module implementing the
          // hook, so no need to do anything more, apart from log this result.
          $this->logger->notice('@type: scheduled processing of %title completed by calls to @hook.', $logger_variables);
        }
        else {
          // None of the above hook calls processed the node and there were no
          // errors detected so set the node to unpublished.
          $this->logger->notice('@type: scheduled unpublishing of %title.', $logger_variables);
          $node->setUnpublished();
        }

        // Use the standard actions system to unpublish and save the node.
        $node = $event->getNode();
        $action_id = 'node_unpublish_action';
        if ($this->moduleHandler->moduleExists('workbench_moderation_actions')) {
          // workbench_moderation_actions module uses a custom action instead.
          $action_id = 'state_change__node__archived';
        }
        $this->entityTypeManager->getStorage('action')->load($action_id)->getPlugin()->execute($node);

I think the call to $node->setUnpublished() is redundant.

Furthermore calling both hook_scheduler_unpublish_action() and the node_unpublish_action action seems really confusing. I guess with the move to events we should consider deprecating hook_scheduler_unpublish_action().

I do agree that triggering the event and rules integration event with a saved node in the correct state does feel more correct. But this is a tricky change. It'd be nice if you could determine if the node was updated by these events and not saved - but this is really tricky.

jonathan1055’s picture

jonathan1055 commented

Hi @alexpott,
Thanks for the response, yes you make some good points.

  1. Before this change anything listening to \Drupal\scheduler\SchedulerEvents::UNPUBLISH or \Drupal\scheduler\SchedulerEvents::PUBLISH could make a change to a node and then rely on these actions saving the node for them.

    Yes I had also realised this. Hence the need for the extra ->save() which I added to the two tests, see interdiff 2-4. The event listeners were "getting away with" not saving, when they should really be doing a save, and I agree that just changing this silently could break some implementations. To detect if the node was changed, is there a saved property that can be inspected? Or could scheduler compare the $node to the unchanged version?

  2. I think the call to $node->setUnpublished() is redundant.

    This is where the node status gets set to FALSE, so if it was not called, we'd have to manually set the status? Or maybe you are saying that this is done within the action $action_id = 'node_unpublish_action'. If so, then you are right, but if that is the case then we should not be using that action when $processed is true because that means another module (such as Scheduler Content Moderation Integration for which the 'scheduler_unpublish_action' hook function was created) has done the work and nothing else needs to be done.

  3. Furthermore calling both hook_scheduler_unpublish_action() and the node_unpublish_action action seems really confusing.

    Yes it is unfortunate that they have similar names, becuase they are not really doing the same thing. See the SCMI comment above

jonathan1055’s picture

jonathan1055 commented
Version: 8.x-1.x-dev » 2.x-dev

Moving to 2.x branch. Will need a re-roll now that #2096585: Support for non-node entities, e.g. Media, Commerce Products, Custom 3rd-party entities has been committed.

jonathan1055 opened merge request !8

jonathan1055’s picture

jonathan1055 commented
Title: Rules and Scheduler events should be dispatched after the node is saved, not before » 3196989-events-after-save

@alexpott I did some more checking, and found that $node->setPublished() and $node->setUnpublished() are redundant when using the actions system (thanks for that hint). This also led on my question about not using the actions system if the entity has been fully processed by a 3rd-party hook. This is indeed the case, which means that the code section is much tidier, and easier to follow.

In 2.x the old hook hook_scheduler_publish_action() can still be used, but this is just for nodes. The new entity-plugin work required new generic hooks and entity-specific hooks, and I took the opportunity to remove 'action' from the name. So we now have hook_scheduler_publish_process() to process any/all entity types, and hook_scheduler_{type}_publish_process() for the entity-specific implementations.

Removed patches now that this is a MR branch. The full patch is 8.diff

jonathan1055’s picture

jonathan1055 commented
Title: 3196989-events-after-save » Rules and Scheduler events should be dispatched after the node is saved, not before

It is slightly annoying that when creating an issue fork branch, and naming it with an appropriately short name, then the issue title gets automatically reset to match the branch. So I'm setting it back to the proper name.

Regarding @alexpott's concern that event subscribers could rely on the actions system to save the node, but now would require a ->save() making this change at 2.x before the first 2.0 release is the right time. The release notes can include this minor chnage. Also, the better place to make entity field changes is via the PRE_PUBLISH and PRE_UNPUBLISH events. There are no changes to these events and no ->save() is required when using them.

  • jonathan1055 committed 32e6378 on 2.x 
    Issue #3196989 by jonathan1055: Rules and Scheduler events should be...
jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Fixed

Following on from my comment in #10 we can document in the 2.0 release that any PUBLISH or UNPUBLISHevent subscribers that modified the node and did not have a ->save() should be re-checked and the necessary save() added. We can also documet that the proper place to be making changes to entities during the Scheduler publish or unpublish processing is by using the PRE_PUBLISH or PRE_UNPUBLISHevent subscribers.

Merged and fixed, so I can release 2.0-alpha1 and set the default issue branch to 2.x-dev.

Status: Fixed » Closed (fixed)

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