Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add Contao Cron service & command #708

Open
wants to merge 30 commits into
base: master
from

Conversation

@fritzmg
Copy link
Collaborator

commented Sep 2, 2019

This PR adds the Contao\CoreBundle\Cron\ContaoCron service, which executes all cron jobs tagged with a new contao.cron service tag as well as all cron jobs defined in the old $GLOBALS['TL_CRON'] array.

Service Tag

The new service tag has the following options:

services:
  App\Cron\MyCron:
    tags:
      -
        name: contao.cron
        interval: daily      # minutely, hourly, daily, weekly, monthly
        method: onDaily
        priority: 100        # >0: before $GLOBALS['TL_CRON'], <0: after $GLOBALS['TL_CRON']
        cli_only: true       # job will only be executed via the contao:cron command

Command

This PR also adds a contao:cron command, which simply executes said service, without any input arguments, options or output. When using the command, all cron jobs will be executed, including the ones tagged with cli: true.

Front end Cron & Route

The /_contao/cron route still exists but now uses the new ContaoCron service instead of the Contao\FrontendCron controller class. Same with the Contao\CoreBundle\EventListener\CommandSchedulerListener (poor man's cron). Cron jobs tagged with cli: true will not be executed for either.

Annotation

This PR also introduces a Contao\CoreBundle\ServiceAnnotation\Cron service annotation. Example with all options:

namespace App\Cron;

use Contao\CoreBundle\ServiceAnnotation\Cron;
use Terminal42\ServiceAnnotationBundle\ServiceAnnotationInterface;

class MyCron implements ServiceAnnotationInterface
{
    /**
     * @Cron("minutely", priority=-1)
     */
    public function onMinutely(): void
    {
        // Do something
    }

    /**
     * @Cron("hourly", priority=32, cli=true)
     */
    public function onHourly(): void
    {
        // Do something on CLI only
    }
}

Notes

This PR does not delete the Contao\FrontendCron class for BC reasons - but it is now unused. Not sure if this is necessary - or if instead the Contoa\FrontendCron should retrieve the new service from the container.

fritzmg added 2 commits Sep 2, 2019

@fritzmg fritzmg changed the title add Contao Cron service & command Add Contao Cron service & command Sep 2, 2019

@fritzmg fritzmg self-assigned this Sep 2, 2019

@fritzmg fritzmg added this to the 4.9 milestone Sep 2, 2019

@fritzmg fritzmg added the feature label Sep 2, 2019

core-bundle/src/Cron/LegacyCron.php Show resolved Hide resolved
core-bundle/src/Cron/Cron.php Outdated Show resolved Hide resolved
@fritzmg

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 3, 2019

Note: I have named the cli parameter of the service tag this way for purely "optical" reasons, as I did not like clionly, cli_only or cliOnly ;). Looking at some Symfony service tag examples from the symfony/dependency-injection tests, it should be with underscore, i.e. cli_only.

@leofeyer

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

it should be with underscore, i.e. cli_only.

This is correct and should be changed as well.

core-bundle/src/Cron/Cron.php Outdated Show resolved Hide resolved
core-bundle/src/Resources/config/services.yml Outdated Show resolved Hide resolved
core-bundle/src/ServiceAnnotation/Cron.php Outdated Show resolved Hide resolved

@fritzmg fritzmg requested a review from ausi Sep 3, 2019

@fritzmg

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 3, 2019

Unit tests for the Cron and LegacyCron are still WIP.

@fritzmg

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 8, 2019

Unit tests are finished. Someone needs to review them though, since this is basically my first time writing more complex unit tests from scratch ;)

@fritzmg fritzmg requested a review from Toflar Sep 8, 2019

@Toflar Toflar requested a review from aschempp Sep 9, 2019

core-bundle/src/Controller/FrontendController.php Outdated Show resolved Hide resolved
$this->db->update('tl_cron', ['value' => $currentTimestamp], ['name' => $interval]);
// Add a log entry if in debug mode (see #4729)
if ($this->debug && null !== $this->logger) {

This comment has been minimized.

Copy link
@Toflar

Toflar Sep 9, 2019

Member

Is there any reason why we only log in debug mode? That makes no sense to me. I know it looks like it was already present but we should re-discuss this. Imho we should always log. I can already filter out the irrelevant log entries by setting the minimum log level.

This comment has been minimized.

Copy link
@fritzmg

fritzmg Sep 9, 2019

Author Collaborator

Well it would result in 2880 System Log entries per day, if you have a minutely cron job.

May be we should not log to the System Log at all? Just to the regular log.

This comment has been minimized.

Copy link
@Toflar

Toflar Sep 9, 2019

Member

Yeah I don't think I would log anything to the Contao table. In my opinion, logging should be as follows:

  • A general log entry whenever the check runs on debug level so that you can debug whether calls are correct.
  • A log entry only if at least one job successfully ran on info level.

Then you only get log entries on info level if actually something ran. But restricting it to $this->debug makes no sense to me.

This comment has been minimized.

Copy link
@fritzmg

fritzmg Sep 9, 2019

Author Collaborator

Fine with me. Only the cron jobs themselves should actually log into the Contao system log table if they want to (which in case of the core cron jobs they are doing right now anyway, like Purged the search cache, etc.), not the Cron service itself.

@leofeyer what do you think?

This comment has been minimized.

Copy link
@leofeyer

leofeyer Sep 10, 2019

Member

Agreed.

This comment has been minimized.

Copy link
@fritzmg

fritzmg Sep 10, 2019

Author Collaborator

Done. I have also added another debug logging entry for when a cli_only cron job is skipped when the cron is not run via the cron:run command.

core-bundle/src/Cron/Cron.php Outdated Show resolved Hide resolved
@Toflar
Copy link
Member

left a comment

LGTM, also congrats on your first PR with that many unit tests 🎉
The only conceptual thing I would question is that now we only have $cliOnly which means I cannot have a simple cron that is only for the web? I mean, maybe we could have something like web|cli if you want to run in both environments, also allowing us to add a third one if it should ever happen.
I just like the things to be as flexible as possible so we don't have to change the concept later on.

@fritzmg

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 11, 2019

LGTM, also congrats on your first PR with that many unit tests 🎉

Yay!

The only conceptual thing I would question is that now we only have $cliOnly which means I cannot have a simple cron that is only for the web? I mean, maybe we could have something like web|cli if you want to run in both environments, also allowing us to add a third one if it should ever happen.
I just like the things to be as flexible as possible so we don't have to change the concept later on.

I was thinking the same, but could not think of a use case where you would want to define a cron job as web only.

@Toflar

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

I was thinking the same, but could not think of a use case where you would want to define a cron job as web only.

Bascially, just everything that is lightweight and has a heavyweight counterpart :) So if you want to rebuild the complete search index, that's a heavyweight task and it's thus only on cli. However, a simple cleanup of search entries older than 2 months or so, could easily be done in web. But that doesn't need to be done if the heavyweight job is registered because it cleans up everything.

Also, do we really need the /_contao/cron route? As far as I can see this is never used, the controller is called in the kernel.terminate listener directly. I guess this was left there so you coud have a real cron job calling wget https....../_contao/cron which you should replace by php /var/...../vendor/bin/contao-console contao:cron now anyway.
Imho the listener should be adjusted to use the new Cron service directly and the /_contao/cron route should throw a deprecation notice and also call that service.
And while doing that, the TL_CONFIG for the disableCron setting should become a bundle config.

So three changes requested here:

  • Make sure one can register for web or cli only or both.
  • Revamp the CommandSchedulerListener to use the new Cron service directly.
  • Make disableCron a proper bundle configuration.

After that this PR is RTM imho, well done 👍

@fritzmg

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 11, 2019

Bascially, just everything that is lightweight and has a heavyweight counterpart :) So if you want to rebuild the complete search index, that's a heavyweight task and it's thus only on cli. However, a simple cleanup of search entries older than 2 months or so, could easily be done in web. But that doesn't need to be done if the heavyweight job is registered because it cleans up everything.

Fair enough. What should be the options though and how should it be named?

Imho the listener should be adjusted to use the new Cron service directly and the /_contao/cron route should throw a deprecation notice and also call that service.

They both already use the service :)

@Toflar

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Fair enough. What should be the options though and how should it be named?

Maybe just scopes? So something like this:

services:
  App\Cron\MyCron:
    tags:
      -
        name: contao.cron
        interval: daily      # minutely, hourly, daily, weekly, monthly
        method: onDaily
        priority: 100        # >0: before $GLOBALS['TL_CRON'], <0: after $GLOBALS['TL_CRON']
        scopes: [cli, web]

They both already use the service :)

Lol, true. But I think we should deprecate the /_contao/cron route.

@fritzmg

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 11, 2019

Maybe just scopes? So something like this:

Sounds good. What should be the default though? [web, cli]? That would be the current behaviour.

Lol, true. But I think we should deprecate the /_contao/cron route.

👍

@fritzmg fritzmg changed the title Add Contao Cron service & command [WIP] Add Contao Cron service & command Sep 11, 2019

@Toflar

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Guess if you did not specify any scopes, it's just all?

@m-vo

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

I guess this was left there so you coud have a real cron job calling wget https....../_contao/cron which you should replace by php /var/...../vendor/bin/contao-console contao:cron now anyway.

Well If you are using wget you get the same web php version/config as the site. This might be useful for some people on shared hostings where the CLI version differs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.