Add $this->getLogger() to BuildTask #9183
Comments
Dependency injection definitely sounds like a good idea in this case. |
|
I'd probably approach it as TaskRunner::runTask() supplied the logger to the build task in question (as it is the part of the system that should decide how things are logged), but perhaps it could be fetching a In an ideal interface, $logger would be an argument to |
Well, sorry, I meant dependency injection through setLogger, and not service discovery through Injector->get. |
chillu
added a commit
to open-sausages/silverstripe-queuedjobs
that referenced
this issue
Jun 9, 2020
It's counterintuitive to run the queue on CLI (e.g. when testing things), get zero error output, and then discover why the job was broken by looking at a tiny text field in the admin/jobs CMS UI. There's an interesting edge case where the logger *does* output to CLI only when a broken job is discovered, because that uses the logger for messages *before* adding the job-specific handlers. And in case a Monolog logger implementation doesn't have any handlers, it'll add a php://stderr by default. Very confusing :D Note that modifying the singleton during execution by adding job-specific handlers isn't ideal (didn't change that status quo). But there's no clear interface for any services being executed through a task receiving a logger *from* the task. So we have to assume they'll use the injector singleton. Technically it means any messages after the job-specific execution (e.g. during shutdown) would also be logged into the QueuedJobDescriptor database record, but you could argue that's desired behaviour. This should really be fixed by adding BuildTask->getLogger() and making it available to all tasks, but this is the first step to fix this specific task behaviour. See silverstripe/silverstripe-framework#9183
chillu
added a commit
to open-sausages/silverstripe-queuedjobs
that referenced
this issue
Jun 12, 2020
It's counterintuitive to run the queue on CLI (e.g. when testing things), get zero error output, and then discover why the job was broken by looking at a tiny text field in the admin/jobs CMS UI. There's an interesting edge case where the logger *does* output to CLI only when a broken job is discovered, because that uses the logger for messages *before* adding the job-specific handlers. And in case a Monolog logger implementation doesn't have any handlers, it'll add a php://stderr by default. Very confusing :D Note that modifying the singleton during execution by adding job-specific handlers isn't ideal (didn't change that status quo). But there's no clear interface for any services being executed through a task receiving a logger *from* the task. So we have to assume they'll use the injector singleton. Technically it means any messages after the job-specific execution (e.g. during shutdown) would also be logged into the QueuedJobDescriptor database record, but you could argue that's desired behaviour. This should really be fixed by adding BuildTask->getLogger() and making it available to all tasks, but this is the first step to fix this specific task behaviour. See silverstripe/silverstripe-framework#9183
chillu
added a commit
to open-sausages/silverstripe-queuedjobs
that referenced
this issue
Aug 27, 2020
It's counterintuitive to run the queue on CLI (e.g. when testing things), get zero error output, and then discover why the job was broken by looking at a tiny text field in the admin/jobs CMS UI. There's an interesting edge case where the logger *does* output to CLI only when a broken job is discovered, because that uses the logger for messages *before* adding the job-specific handlers. And in case a Monolog logger implementation doesn't have any handlers, it'll add a php://stderr by default. Very confusing :D Note that modifying the singleton during execution by adding job-specific handlers isn't ideal (didn't change that status quo). But there's no clear interface for any services being executed through a task receiving a logger *from* the task. So we have to assume they'll use the injector singleton. Technically it means any messages after the job-specific execution (e.g. during shutdown) would also be logged into the QueuedJobDescriptor database record, but you could argue that's desired behaviour. This should really be fixed by adding BuildTask->getLogger() and making it available to all tasks, but this is the first step to fix this specific task behaviour. See silverstripe/silverstripe-framework#9183
chillu
added a commit
to open-sausages/silverstripe-queuedjobs
that referenced
this issue
Sep 18, 2020
It's counterintuitive to run the queue on CLI (e.g. when testing things), get zero error output, and then discover why the job was broken by looking at a tiny text field in the admin/jobs CMS UI. There's an interesting edge case where the logger *does* output to CLI only when a broken job is discovered, because that uses the logger for messages *before* adding the job-specific handlers. And in case a Monolog logger implementation doesn't have any handlers, it'll add a php://stderr by default. Very confusing :D Note that modifying the singleton during execution by adding job-specific handlers isn't ideal (didn't change that status quo). But there's no clear interface for any services being executed through a task receiving a logger *from* the task. So we have to assume they'll use the injector singleton. Technically it means any messages after the job-specific execution (e.g. during shutdown) would also be logged into the QueuedJobDescriptor database record, but you could argue that's desired behaviour. This should really be fixed by adding BuildTask->getLogger() and making it available to all tasks, but this is the first step to fix this specific task behaviour. See silverstripe/silverstripe-framework#9183
|
Closing - with the refactor of sake and |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
There's been a whole large discussion about refactoring BuildTask, but one incremental fix that would make things better is to provide a
getLogger()method on the abstract base class of BuildTask. In the v4.5 implementation I would recommend that this simply prints to STDOUT and/or STDERR. We could providesetLogger()too, so that BuildTask runners might override this in the future.BuildTasks could then be written to rely on a logger object rather than echos, from 4.5 on.
The text was updated successfully, but these errors were encountered: