Convert cron.php into a regular menu router page callback

To clarify:

We discussed this briefly in IRC and no one in #drupal-contribute was able to see why cron.php is a separate PHP file instead of a regular menu callback.

Unlike install.php/update.php (which are very special scripts) and xmlrpc.php (which is just plain horrible, but even that: #1033818: Move xmlrpc includes into xmlrpc.module), cron.php really does not do anything special.

cron.php is only ever invoked after the system is in place, at which point you know whether you will have to invoke /cron (clean) or index.php?q=cron (or upcoming index.php/cron, dirty). There is no difference to /cron.php.

The contained code is trivial and uses Drupal's standard bootstrap mechanism. Bootstrap is followed by some inlined code, but that can be easily moved into System module, as this patch demonstrates.

Adding functionality to System module generally is a won't fix ;-) but in this case it makes sense and seems to be valid (and could have done years before).

Apparently, moving the cron functionality into a regular menu router page callback allows alternative implementations in contrib to enhance or override it more easily (because no one hard-codes his cron command to cron.php anymore, which then has to be changed for alternative implementations). So there's an additional benefit in this conversion.

Almost RTBC.

+++ b/core/modules/system/system.module
@@ -5,6 +5,8 @@
+use Symfony\Component\HttpFoundation\Response;
+

Unless I'm missing something, this shouldn't in this patch :)

+++ b/core/modules/system/system.module
@@ -580,6 +582,12 @@ function system_element_info() {
+  $items['cron'] = array(
+    'title' => 'Run cron',
...
+    'type' => MENU_CALLBACK,

I realize the other items defined nearby serve as poor example, but there's no point in specifying a title for a MENU_CALLBACK, since no menu links are generated for them.

Well, we can also leave it and defer those titles to a separate issue.

+++ b/core/modules/system/system.module
@@ -1129,6 +1137,36 @@ function system_menu() {
+ * Page callback; Execute cron tasks.
...
+function system_cron_callback() {

Let's name this system_cron_page(). "callback" doesn't really mean anything. :)

+++ b/core/modules/system/system.module
@@ -1129,6 +1137,36 @@ function system_menu() {
+ *Access callback for system_cron().

Missing leading space.

errr, what I meant was to remove the 'title', not the MENU_CALLBACK type -- it has to be a MENU_CALLBACK, or the menu system will auto-generate a menu link for the router item, which then appears in the Navigation menu. Don't think we want that ;)

ActionsLookTestCase failing means that something is throwing a PHP warning or notice. The test implements hook_watchdog() and logs whatever is getting through watchdog(). Probably a typo or similar somewhere in the recent patches.

Restored the type, left the title alone. Can be cleaned up later.

I merely fixed the test failure, so RTBC it is.

Still RTBC! Just thought of a couple things that could be follow up issues...

+++ b/core/modules/system/system.moduleundefined
@@ -1129,6 +1135,36 @@ function system_menu() {
+function system_cron_page() {
+  drupal_cron_run();
+
+  // Returning nothing causes no output to be generated.

We can't just reference "drupal_cron_run" directly from system_menu(), can we?

+++ b/core/modules/system/system.moduleundefined
@@ -1129,6 +1135,36 @@ function system_menu() {
+function system_cron_access() {
+  $cron_key = isset($_GET['cron_key']) ? $_GET['cron_key'] : '';
+  if ($cron_key != variable_get('cron_key', 'drupal')) {
+    watchdog('cron', 'Cron could not run because an invalid key was used.', array(), WATCHDOG_NOTICE);
+    return FALSE;

I understand this is somewhat changing the way /cron works, but what if we used 'cron/%system_cron_key' and 'access arguments' => array(1) for the menu callback, system_cron_access($cron_key) and system_cron_key_load()?

#10 actually merits a CNW. The patch so far is really nice but why not go the full nine yards and use a nice url and use the menu capabilities.

Incorporated #10.

http://drupal.org/cron needs to be updated as a result of this patch.

system_cron_page() cannot be replaced with drupal_cron_run(), since the latter returns a status/success (boolean) code, which would be mistaken as page callback return value (response).

Two things I absolutely love about this patch:

1. The removal of direct $_GET usage
2. Another ugly looking php file bites the dust

Testbot apparently reported the same test assertion failures two times. We need to stop bashing our testing system and infrastructure and instead help that 1.4 person who desperately tries to keep it running.

Agreed.

I know it's NIH but

+curl --silent --compressed http://example.com/cron

Shouldn't this and similar examples be referencing the cron key. Don't need to fix it here, just want to make sure I'm not going mad.

Appended /YOURKEY to both scripts.

Thanks! Committed/pushed to 8.x. This will need a change notification.

I bet some sites deleted cron.php and thus prevented one DOS attack vector since running cron can be expensive. They would use `drush cron`, presumably. Now those sites are going to have to edit .htaccess in order to get similar protection. Not that big a deal. This change still makes sense.

@moshe weitzman: That's why we introduced the cron key. Without a valid one, cron is just a full bootstrap only.

That said - now that it's a page callback, do we need to prevent the page cache from kicking in when page caching is enabled?

Seems like we should open a task for that anyway, with "revisit before release" or whatever, so we don't forget?!

It is a one line patch so rather than open a new major task, lets just fix it. Patch attached.

Committed/pushed the follow-up thanks!http://drupal.org/files/no_cache.patch

Minor followup: #1873574: Cron URL on status report page is no longer an absolute URL