Create a service for the Scheduler API

In attachment a patch which introduces this service. I've checked the test scripts and there's no need to update them as the functionality remains the same.

Thank legovaer for starting this.
I have just committed #2655666: API Testing module - conversion to 8.x so will re-queue the above patch. It will now actually test the hook_scheduler_allow() function.

That's good. The two API classes that were failing (before I converted the API test module code) are now passing.

Agreed that we should turn hook_scheduler_api() into events. I created #2669164: Introduce event subscriber for the Scheduler hooks, otherwise the change in this issue might get too big.

Updated method name to "isAllowed()" as suggested in #8

I've done a pull on the latest code we have in the 8.x-1.x branch and re-applied the patch of #3 manually. Also the changes of #10 (without the garbage) have been added. I've also removed the function _scheduler_api() as we won't need it anymore now that we have this service.

The interdiff may look a bit odd, but that's because several changes already went in the code.

While going through the code again, I noticed that the entire scheduler.cron.inc should have become part of a service as well. So now I introduced one service: SchedulerManager.

What do you think about this one?

What do you think about this one?

... um ... I think your patch is the wrong way round, isn't it?

What do you exactly mean? Wrong implementation?

What do you exactly mean? Wrong implementation?

Ah, I looked at the interdiff, and it has been made the reverse way round. In the first chunk, you try to create a new file scheduler.cron.inc, and likewise the other files have the reverted change. I did not look at the actual patch, but that has been made the right way round. So you can ignore my comment, other than nothing when you make an interdiff, take a quick look to check it rather than confuse us ;-)

Pieter will have to advise on the idea of moving the whole of cron.inc to be part of the service. I will wait until your patch passes automated testing, then take a closer look.

I have now added test coverage for all the existing hooks, see #2655666-23: API Testing module - conversion to 8.x. The files that changed in this commit are SchedulerApiTestCase.php and scheduler_api_test.module so this will not affect your code changes in the patches above.

The patch in 15 no longer applies due to my recent commits. I've created a new one which does apply. I have not altered any of the actual code, hence this will fail in the same way as #15 did, but at least you can take this new patch and work on it directly with the new code-base.

I cannot see any error messages to explain the exceptions on the d.o. testing console output, but on my local site I get

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException:
The service "scheduler.manager" has a dependency on a non-existent service "logger.channel.scheduler"
in Symfony\Component\DependencyInjection\Compiler\CheckExceptionOnInvalidReferenceBehaviorPass->processReferences()
(line 58 of vendor/symfony/dependency-injection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php)

Hope that helps.

@legovaer, hope you don't mind but I wanted to work on this blocker, as it has been three weeks since your last post.

1. I took a closer look at the patches in #14 and #21 and the first thing I noticed was that logger.channel.scheduler is not defined in .services.yml, so I added that (copied the definition from core/modules/contact). That solved the first error.
2. Then I got "Fatal error: Class 'Drupal\scheduler\Manager' not found", and realised that you had named the file and class SchedulerManager, hence it needs to be class: Drupal\scheduler\SchedulerManager in .services.yml
3. Thirdly, now that we have the full scheduler.manager service, which has replaced scheduler.authorizer, the calls to isAllowed() need to be \Drupal::service('scheduler.manager')->isAllowed() but they were still \Drupal::service('scheduler.authorizer')->isAllowed()
4. Having done this, manual editing and setting publish dates worked, but cron failed - this was due to calls to the original function _scheduler_scheduler_nid_list() which had not been changed to $this->nidList()
5. Also there was a typo which caused a fatal error, where \Drupal::entityManager()->getStorage('action')... had been changed to $this->moduleHandler->getStorage('action')... when it should be $this->entityManager->getStorage('action').... This finally got cron running :-)

The automated tests fail, so that's the next step to solve.

Posting this work here, plus an interdiff of #21 to #24.

That's progress with 26 passes, up from 0.

Further fixes:

1. When calling isAllowed() the initial conversion missed the ! in front, so that had the effect of denying publication for every node (apart from the ones which were denied), and likewise for unpublishing. This fixes all but two of the failing test classes
2. "Class 'Drupal\scheduler\SchedulerNodeTypeNotEnabledException' not found" - The two exceptions were created in namespace Drupal\scheduler\Plugin\Exception\ and attempted to be used via use Drupal\scheduler\Plugin\Exception\SchedulerMissingDateException. This can be corrected by creating them in the normal Drupal\scheduler\ namespace
3. In SchedulerCronForm we need to call \Drupal::service('scheduler.manager')->runCron() instead of including the old scheduler.cron.inc file [same change as in .module scheduler_cron]

New full patch, plus interdiff from 24 to 26 to show the changes discussed above.

Nice! Great progress Jonathan! As far I can see, we have 35 passes and no fails so it seems to be working fine now. Should we wait for Pieter's review?

I've just done a full review and it looks OK to me!

Thank you Pieter for your confidence in us, and thank you Levi for reviewing. I am happy with the majority of the changes I made in #24 and #26 as they were corrections with an obvious answer. However, I would just like to ask your opinion specifically on two of them given that I am relatively new to OO and having to do lots of learning:

In #24.2

"Fatal error: Class 'Drupal\scheduler\Manager' not found" ... named the file and class SchedulerManager, hence it needs to be class: Drupal\scheduler\SchedulerManager in .services.yml

Alternatively could we rename the file manager.php and the class to be just Manager and leave the namespace as Drupal\scheduler\Manager. Is there a prefered naming convention here?

Similarly, in #26.2

"Class 'Drupal\scheduler\SchedulerNodeTypeNotEnabledException' not found" - The two exceptions were created in namespace Drupal\scheduler\Plugin\Exception\ and attempted to be used via use Drupal\scheduler\Plugin\Exception\SchedulerMissingDateException. This can be corrected by creating them in the normal Drupal\scheduler\ namespace

Did I pick a sensible correction to this? Or should the exceptions be in a more specific namespace such Drupal\scheduler\Exception. It works fine at the base level of Drupal\scheduler but I just wondered if this is actually the correct place?

Jonathan,

As you can see in the Naming Conventions in #10:

Classes and interfaces should have names that stand alone to tell what they do without having to refer to the namespace, read well, and are as short as possible without losing functionality information or leading to ambiguity.

I'd suggest to keep the name SchedulerManager. If you have a look at the core modules, you'll see that they also use this naming convention (E.g. BookManager).

About the exceptions: I'd keep them in a separate namespace. This allows us to keep the code and the PHP files structured. I don't want the src/ folder to become a mess. An example of this namespace usage can be found in the migrate module. The custom exception's namespace in migration is \Drupal\migrate\Exception\RequirementsException.

I've added a patch which updated the namespace again.

Forgot to update the namespace in the docblock.

Thanks for the explanations in #31. I am pleased I picked the right place for SchedulerManager.

Yes I agree with your suggestion for the exceptions. Just to confirm - the exception namespace is now Drupal\scheduler\Exception which is better than both the original Drupal\scheduler\Plugin\Exception\ and the working but not-to-standard Drupal\scheduler. The files are stored in /src/Exception/ not just at /src/

I have seen a few things in the comments which need adjusting, but I have noted those and will make the amendments afterwards, rather than fiddling with more interdiffs. I am happy with committing this. It will have an impact on the patch in #2669164: Introduce event subscriber for the Scheduler hooks which will have to be re-done given that .cron.inc will no longer exist. But that was always going to be the case. Getting this first commit in will allow us to work on the other things.

Let's ship it. I'll start updating the other issue so that we can get that in as well.

Here you go! Great work, thanks.

This issue is not closed yet, as I mentioned above, there are few minor tweaks, comments and adherence to standards which I will do now.

Here's the tidy-up patch

1. in schedulerManager, removed '\Plugin\' from the @throws path (x4)
2. in schedulerManager, due to moving code from .cron.inc with an extra indentation level, fixed some comments which extended more than 80 chars
3. in the two exception files, added proper explanation of the exception
4. also in the exceptions, added unpublish() and added the fully qualified namespace

I have avoided changing a couple of comments which are part of your new patch in #2669164-24: Introduce event subscriber for the Scheduler hooks so hopefully that wont need to be re-rolled again.

Looks good to me! Great work!

Very good to get this moving. We can adjust anything later, but we needed to get these files in. Thank you for all your work on this.