scheduler/cron needs a route controller

Hi chr.fritsch,

Thanks for the patch. I'll test it.

We were already aware that this task was still remaining and the original issue was #2432341: Convert 'scheduler/cron' to a route controller. Normally we'd close the newer one and mark it as dupliacte of the earlier issue, but seeing as you have provided a patch here and there was none on that issue I will keep this newer one open.

Jonathan

I've applied the patch and scheduler/cron is working but only if I create a lightweight key. Was it your intention to force this? Looking at the code, if both the stored key in the settings and the passed url element are blank then it should still pass?

Also my cron jobs do not seem to appear everytime in the log. I had a task set for */3 and it ran once sucessfully but will not run again. Neither do I get the 'access denied' message. I'm not sure what is going on there.

I'd like Pieter to review your patches too, as he has more experience than me or D8 route controllers, etc.

But thanks for getting this started, and certainly I did see a lightweight cron run, so it is working of sorts.

I have just looked back and Pieter already asked the question about making the cron access key mandatory in comment 46 of #1982932: Drupal 8 Port of the Scheduler Module. Having thought about it, we should take this opportunity to enforce the key and not allow runs from unauthorised users (even though we discussed on 7.x that this would not pose any real problem).

So now we just need to understand exactly how the current patch is forcing an access key, as it is not clear why this is the case.

Updated summary, taken from #2432341: Convert 'scheduler/cron' to a route controller which is closed as a duplicate

Here is an updated patch. I have made a few small changes:

1. Added no_cache: TRUE to the route to ensure the job always runs.
2. Changed response to 204 (copied from CronController::run)
3. Make lightweight_access_key a required form field.
4. Improve coding standards.

The reason that the original patch forces a key to be used is that cron_key: '' was not added to the route. See this example.

Hi polynya,
Thanks for re-rolling this patch, I can see you had to do a bit of work as the patch in #1 was out of date.

You've added quotes around all the strings in .routing.yml, I presume that's the standard? Thanks for explaining why the previous patch forced a cron_key. Nice catch to make the form field required.

I have tested the change and it all seems to be working perfectly. My observations:

1. crontab job runs correctly as expected when given the right cron_key
2. crontab job runs does not run if given no key or the wrong key
3. Manually visit /scheduler/cron gives 'page not found'
4. Manually visit /scheduler/cron/{wrong-key} gives 'you are not authorised'
5. Manually visit /scheduler/cron/{correct-key} runs the lightweight cron but gives no return to the user and the existing page is unchanged. Maybe this is OK, but at first I thought that the route controller was not working or I had got the key wrong. This might also confuse other admins who are in the process of setting up their cron key. Is this a result of returning Response('', 204) instead of RedirectResponse('/') that the previous patch had. I'm ok with this if this is the right way to do it, but I'd like my other co-maintaniner to give an opinion too.
6. I think the original access callback function _scheduler_cron_access($cron_key) in .module is now redundant and can also be removed
7. The text in scheduler_help needs a slight change now that the access-key is mandatory

I've re-rolled the patch, with all your changes exactly as in #8 but also removing _scheduler_cron_access and altering the help text.

Hi Jonathan,

Thanks for responding so quickly. I've applied your new patch and get the same results for tests 1-5. Test 5 returns no content (HTTP 204), which I think is the correct response, but I'm OK if you want to change it back.

I haven't found YML files in the Drupal coding standards but there's this reference. I've just copied the format from core modules.

Adrian.

I found an issue on the cron settings form. When the module is enabled, the lightweight_cron_access_key setting is empty. It's not possible to run 'Generate new random key' on the form because the lightweight_access_key field is required but is empty.

I have got round this by setting lightweight_cron_access_key to a random value in scheduler_install(). This is better for security anyway.

I've re-rolled the patch. The only change from patch 10 is in scheduler.install.

Good catch. Tested and it works nicely.
Re-rolled with a comment added in hook_install() and a simplification of the help text now that the key is guaranteed to exist.

Do you think we need to add automated tests for this functionality? Once we've written it, it is unlikely to change and go wrong. Can we assume the route controller will always work? Do we need to check access is denied without the correct cron key? I don't want to spend effort on writing tests which are never going to trap any future fault. On the other hand, if we'd had tests for scheduler/cron in 7.x we would have discovered this omission earlier.

Here's a new patch with some tests for lightweight cron. Do you think we need any more tests?

Great, thanks for the test file. I've noticed a few things which need to be changed (problem with 'locale' and the wrong cron urls), but I have uploaded a patch of just the tests from #15 above onto issue #2594615-152: Automated testing in 8.x [meta] to see how it runs as-is.

Here's a re-roll to contain just the new test file, with the following changes:

1. Running the tests onsite via simpletest the 'locale' module caused a fatal (see attached), however removing the module is fine, the tests run just the same.
2. The first two urls were just cron where they should be scheduler/cron
3. Added some more text to the assertions
4. There is still a problem checking the default key. $key = $key_xpath[0]; is not empty - see debug output

The number of fails should be more than the 2 fails previously on https://www.drupal.org/pift-ci-job/164188

The patch in #18 looked OK to me. Seems there was a testbot CLI error. Going to re-test and see if runs this time.

Here's the error text I forgot to add in #18. I cannot see any reason to add 'locale'.

Thanks for reviewing the test. I had added the 'locale' module because it prevents the error you reported in #21. I don't know what causes the error but it only occurs for me when running the test through the UI (admin/config/development/testing). It doesn't occur when using run-tests.sh.

You tried running the test file on its own on Convert tests. It failed because it depends on the key being set when the module is enabled, which is in the full patch for this issue.

I have re-rolled the patch with your changes to the test file and minor changes from me.

I had added the 'locale' module because it prevents the error you reported in #21

That is strange, because I was getting completly the opposite! I got the error with locale added, and the error disappeared when I took 'locale' out of the list.

You tried running the test file on its own on Convert tests. It failed because it depends on the key being set when the module is enabled

Yes, exactly. That is what I wanted to show - how the tests perform without any fixes to the route code. You have to show the tests failing so that when the fix is applied and you get greens you know that the change has made a difference. It highlighted the problem with the check empty($key) returning false even when there was no value, ie a test was passing when it should have failed. From a quick glance at your new patch it looks like you have addressed this. I will test it fully and if running OK will commit the test file on its own first. Then we get the branch test results before the code fix is committed.

Thanks very much for your work on this.

Here is just the test file from 24, i.e. running without the code fixes.

Three tests fail here:

scheduler/cron with the wrong cron key returns "403 Not Authorized"
scheduler/cron with the correct cron key runs OK and returns "204 No Content"
Default lightweight cron key string length is 20

The test "Lightweight cron key field is not empty by default." does not fail but it should do. The object is not empty (which is correct) so testing !empty() on the XMLElement object does not give what we want. It should actually be testing the value, which is empty. Casting as string fixes this.

I have also added two more checks - to make sure we get the validation message 'cron key is required' and also that the form cannot be saved. With these extra two tests, plus the fix above there should be six fails.

six fails as expected:

scheduler/cron with the wrong cron key returns "403 Not Authorized"
scheduler/cron with the correct cron key runs OK and returns "204 No Content"
Default lightweight cron key field is not empty
Default lightweight cron key string length is 20
Saving configuration with a blank cron key throws the expected validation message
Saving configuration with a blank cron key is not possible

The tests are committed.
Here is a patch for the code fixes. Should get a clean set of passes for this class.

Good.

3		Scheduler.Drupal\scheduler\Tests\SchedulerLightweightCronTest
✓		- setUp
✓		- testLightweightCronRun
✓		- testLightweightCronSettingsForm

and in the dispatcher:

Drupal\scheduler\Tests\SchedulerLightweightCronTest           43 passes      

Committed and fixed.
Thanks very much Adrian (polynya) and Christian (chr.fritsch) for your work on this. You are welcome to take a look at our other issues currently blocking the first 8.x release #2662476: Progress towards 8.x release of Scheduler - your help would be appreciated :-)

Ignore the failed patches above. They were queued but not run until the 8.x committed codebase passed all tests. That happened with my commit a few minutes ago, and then the untested patches have suddenly come to life and been run. This issue is already fixed and closed.