Introduce event subscriber for the Scheduler hooks

Thanks for working on this. I will do some manual testing too.

Not yet, as I have spent most of time on #2651338: Create a service for the Scheduler API. I don't know anything about event dispatch and event subscribing, so I am having to learn it all before I can understand what the changes mean :-)

I will requeue the patch in #4 as that was when we only had 32 classes. Now that #2655666: API Testing module - conversion to 8.x has been expanded and completed, the result might be different.

The reasons for the failed tests in #4 would not be very clear to someone who had not written the tests. Also, the unpublishing assertions gave mis-leading passes when they should have failed, which was caused by earlier failed operations. Hence I have made the tests stronger, and given clearer assertion messages, in the commit in #9. I have re-queued the patch to show the improved failure messages.

The reason for patch failure is that the testing module also needs to be updated. It currently implements the existing hook_scheduler_api. But if this function is being replaced by events then our use of it in scheduler_api_test module also needs to updated.

/**
 * Implements hook_scheduler_api().
 */
function scheduler_api_test_scheduler_api($node, $action) {
  // This hook allows modules to react to the Scheduler process being performed.
  // It is invoked during cron processing and also from scheduler_node_presave().
  // Use the standard 'sticky' and 'promote' fields as a simple way to test the
  // processing. Use conditional checks on $node->isPublished() to make the
  // tests stronger so that they will fail if the calls are in the wrong place.
  switch ($action) {
    case 'pre_publish' :
      // Before publishing a node make it sticky.
      if (!$node->isPublished()) $node->set('sticky', TRUE)->save();
      break;
    case 'publish' :
      // After publishing a node promote to front page.
      if ($node->isPublished()) $node->set('promote', TRUE)->save();
      break;
    case 'publish_immediately' :
      // When publishing immediately set the node to sticky and promoted, and
      // also change the title. Do not use ->save() as this breaks the system!
      $node->set('sticky', TRUE)->set('promote', TRUE);
      $node->title->value = 'Published immediately';
      break;
    case 'pre_unpublish' :
      // Before unpublishing a node make it unsticky.
      if ($node->isPublished()) $node->set('sticky', FALSE)->save();
      break;
    case 'unpublish' :
      // After unpublishing a node remove it from front page.
      if (!$node->isPublished()) $node->set('promote', FALSE)->save();
      break;
    default:
  }
}

That's the beauty of automated testing!

@legovaer - what are the instructions for converting an implementation of hook_scheduler_api to use the new event?

Thanks for working on this.

Renamed scheduler_api_test to scheduler_events_test

Please can I suggest that we do not rename the test module until after the bulk of the actual work is complete. I tried to read your patch and interdiff, but because the module name was changed it was just a complete list of deleted files and new files, making it impossible to read what you have actually done. I understand that we should change the test module name in due course, but for other people to understand what coding you have done and what changes have actually been made, please can you revert the name change, for now. Trying to review a patch of 880 lines and an interdiff of 930, and understand what changes have actually been done is a big task, and it would be much easier if the module name was kept the same in the first instance.

By the way, patch 14 is smaller than 12, did you mean that?

I'm looking forward to seeing what is actually needed to convert the test module, as we will have to document this to allow our users to upgrade their own scheduler hooks to 8.x. Making the changes in stages will assist in this task.

Thanks again for your work on this. Hope you are not too annoyed with my request.

Great work, thanks. I've taken your patch and made a few changes:

1. In scheduler.manager, function publish() I removed an unnecessary $node->save(). I would say it is better to do the save only at the end of the function, where it is already handled by the actions process
2. In tests/modules/src/EventSubscriber.php, renamed the functions so they reflect the event not the node changes being done. This file can be used as the basis for our events documentation and examples, so using names which match the event will be easier for developers to follow
3. Added the conditional logic within these test event functions, which was in the original api test module. It makes the tests stronger, as mentioned in the comments
4. Removed definition of scheduler_api_test_scheduler_api() from tests/modules/scheduler_api_test.module
5. Removed example of hook_scheduler_api() from scheduler.api.php
6. All the other changes are only to the comments, either expanding the explanations, improving the grammar, or copying details from the old functions that got dropped

Patch has the full changes, interdiff shows just my changes between 25 and 26.

Hi legovaer,
Have you had a chance to review the latest changes in #26? As explained, there is nothing major that I have changed from your patch, it is mainly the naming of functions and a bit of tidying up. It all works, and passes the full set of tests, but I know that that's only half the story. These events replace all the calls to hook_scheduler_api(), but I think that hook_scheduler_nid_list() and hook_scheduler_allow() will remain unchanged.

You asked a question about the possible clash of events that I had started in #2651348: Port Rules integration for Scheduler to Drupal 8. Does this need to be looked at now?

This is the last blocker issue that we've identified before we can make an alpha1 release, and it would be really good if I could do that soon. I will also ask pfrenssen to review this, as I definitely want his agreement that we are doing the right thing before going ahead with the alpha release.

Jonathan

Thanks for the review. Good that you are OK with my changes.

I did contact Pieter and he said just over a week ago that he would like to review this final alpha1 blocker. I would appreciate his input, and I am not sure of the limitations we would be creating if we make alpha1 now, then Pieter wants to change the structure. I guess anyone using the dev release or upcoming alpha1 would just have to re-install Scheduler if we did make major changes, and possibly re-write any implementations of the new event subscriber.

I am unsure of the implications of #2651348: Port Rules integration for Scheduler to Drupal 8 and how that may clash with this issue. That's also what I would like Pieter's view on.

... or maybe we go ahead, and try to fix things if we find we need to do major changes later? You are right, it would be good to get a proper non-dev release on the project page.

I think that making a separate version would add more confusion. And doesn't a 'beta' release follow a few 'alpha' releases, when the structure and API are stable. We certainly must be confident that nothing major will change before we name anything 'beta'.

Thanks for the idea, but I am now considering waiting until after this weekend, and if Pieter has not reviewed, I will commit this change and release alpha1 anyway. I'm sure we can fix things afterwards, and for a first alpha release other users should accept that an update might require a re-install of the module, and that some restructuring might be needed if they have started coding to react to the new event subscribers.

Re-rolled the patch to cater for my change to the API test module in #2655666-36: API Testing module - conversion to 8.x.

Great work, Levi. Thank you for all this work. I decided to go ahead and commit - Pieter can review later and I am happy to make changes as required. Now for alpha1!

This is fixed so setting to Unassigned