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.

Sign up for GitHub

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

Jump to bottom

[5.2] Use an injected logger in the Task driver #36440

Draft
wants to merge 7 commits into
base: 5.2-dev
Choose a base branch
from
Draft

[5.2] Use an injected logger in the Task driver #36440

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bb44ec2
Use an injected logger in the Task driver
ditsuke Dec 27, 2021
a84df0a
Use injected Task logger in Scheduler
ditsuke Dec 27, 2021
9148f7c
Use DI for TaskPluginTrait logger
ditsuke Dec 27, 2021
251c894
Cleanup todo
ditsuke Dec 27, 2021
924e40b
Fix code-style
ditsuke Dec 27, 2021
491d772
Add missing doc, fix code-style
ditsuke Dec 27, 2021
a93c958
Merge branch '4.1-dev' into scheduler-logging-di
bembelimen Dec 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Use injected Task logger in Scheduler
  • Loading branch information
@ditsuke
ditsuke committed Dec 27, 2021
commit a84df0a01d8b515ec09ce1abb9238d9e89ad80c1
13 changes: 9 additions & 4 deletions administrator/components/com_scheduler/src/Scheduler/Scheduler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@
use Joomla\CMS\Application\CMSApplication;
use Joomla\CMS\Factory;
use Joomla\CMS\Language\Text;
use Joomla\CMS\Log\DelegatingPsrLogger;
use Joomla\CMS\Log\Log;
use Joomla\Component\Scheduler\Administrator\Extension\SchedulerComponent;
use Joomla\Component\Scheduler\Administrator\Model\TaskModel;
use Joomla\Component\Scheduler\Administrator\Model\TasksModel;
use Joomla\Component\Scheduler\Administrator\Task\Status;
use Joomla\Component\Scheduler\Administrator\Task\Task;
use Psr\Log\LoggerInterface;
use Symfony\Component\OptionsResolver\Exception\AccessException;
use Symfony\Component\OptionsResolver\Exception\InvalidOptionsException;
use Symfony\Component\OptionsResolver\Exception\UndefinedOptionsException;
Expand Down Expand Up @@ -127,14 +129,17 @@ public function runTask(array $options): ?Task

$app->getLanguage()->load('com_scheduler', JPATH_ADMINISTRATOR);

$options['text_entry_format'] = '{DATE} {TIME} {PRIORITY} {MESSAGE}';
$options['text_file'] = 'joomla_scheduler.php';
Log::addLogger($options, Log::ALL, $task->logCategory);
$loggerOptions = [
'text_entry_format' => '{DATE} {TIME} {PRIORITY} {MESSAGE}',
'text_file' => 'joomla_scheduler.php'
];
/** ! No non-static methods to add a sub-logger to {@see DelegatingPsrLogger} (Task::logger) */
Log::addLogger($loggerOptions, Log::ALL, $task->logCategory);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better inject a pre-configured logger using Scheduler's constructor. Within the component, you shouild just send messages to the logger and not change its configuration. The setup is up to the front controller, not the component itself.

For example, if one wants to set up an automated evaluation of the log, it is necessary to be able to freely determine the log format and the storage of the log data. Your approach, however, prescribes these properties so that the dependency injection is practically worthless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the merit in decoupling the configuration! It's totally doable for everything but task-specific logging since the relevant information is only exposed once we get the task record from the database. What would be the best way to do that setup?

https://github.com/ditsuke/joomla-cms/blob/scheduler-logging-di/administrator/components/com_scheduler/src/Task/Task.php#L151-L160

This comment was marked as duplicate.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can, but it needs some additional configuration specific to the task (logging to the task-specific log file).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a misconception. If, how and where things are logged is totally up to the system admin. If you want (feel the need) to provide a default logging procedure, you can do so by providing a bespoke logger that is instantiated as a default, if no logger is provided be the environment.


$taskId = $task->get('id');
$taskTitle = $task->get('title');

$task->log(Text::sprintf('COM_SCHEDULER_SCHEDULER_TASK_START', $taskId, $taskTitle), 'info');
$task->log(Text::sprintf('COM_SCHEDULER_SCHEDULER_TASK_START', $taskId, $taskTitle));

// Let's try to avoid time-outs
if (\function_exists('set_time_limit'))
Expand Down