[5.2] Use an injected logger in the Task driver #36440
Conversation
- Modifies the Task constructor to accept a LoggerInterface instance. - Updates Task creation in Scheduler::getTask().
- ExecuteTaskEvent carries the task logger as a callable arg. - TaskPluginTrait::startRoutine() sets up callable. - TaskPluginTrait::logTask() uses injected logger.
Logger setup is done in the Scheduler and Task classes.
| @@ -255,7 +260,7 @@ public function getTask(array $options = []): ?Task | |||
| return null; | |||
| } | |||
|
|
|||
| return new Task($task); | |||
| return new Task($task, Factory::getContainer()->get(LoggerInterface::class)); | |||
There was a problem hiding this comment.
Would be nice if you can get here a logger which is injected all the way down from the component class.
There was a problem hiding this comment.
I was not able to figure how to do this, since our instantiation chain starts from a plugin. Any leads? 😕
There was a problem hiding this comment.
Do you boot the scheduler component in the plugin?
There was a problem hiding this comment.
Right now, its only done in the plugin's injectLazyJS() method. The component is otherwise booted within the Scheduler::getTask() method.
There was a problem hiding this comment.
Should we be injecting the model/component instance?
There was a problem hiding this comment.
Yes there you can start with it. In the provider.php file you can inject the logger into the component and then pass it into the scheduler.
| '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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
I can, but it needs some additional configuration specific to the task (logging to the task-specific log file).
There was a problem hiding this comment.
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.
| $app = $this->app ?? Factory::getApplication(); | ||
| $app->getLanguage()->load('com_scheduler', JPATH_ADMINISTRATOR); | ||
| $langLoaded = true; | ||
| throw new \LogicException("Logger has not been setup!"); |
There was a problem hiding this comment.
Logging should always be optional. Issue a notice or warning, if necassary, but don't stop execution.
There was a problem hiding this comment.
Good idea! Didn't think of that
| '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); |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
This pull request has automatically rebased to 4.2-dev. |
|
@ditsuke can you implement the the suggestions and fix the conflicts? |
|
This pull request has been automatically rebased to 5.0-dev. |
|
This pull request has been automatically rebased to 5.1-dev. |
|
This pull request has been automatically rebased to 5.2-dev. |
HLeithner
changed the title
Summary of Changes
A logger is injected into Task at creation, and into routines through ExecutedTaskEvent at runtime.
Testing Instructions
Tasks should be logged as expected to the scheduler log file "administrator/logs/joomla_scheduler.php".
Logs for individual tasks, if enabled, should be saved to the file as specified in the configuration.
Actual result BEFORE applying this Pull Request
No DI, ugly setup code in TaskPluginTrait etc.
Expected result AFTER applying this Pull Request
DI, less static calls (still depends on the global Log instance to setup sub-loggers! :/), more succinct.
Documentation Changes Required
-