symbiote / silverstripe-queuedjobs

A module that provides interfaces for scheduling jobs for certain times.
BSD 3-Clause "New" or "Revised" License
57 stars 73 forks source link

Should saved job messages be their own model? #110

Open kinglozzer opened 7 years ago

kinglozzer commented 7 years ago

The messages can be very useful for providing feedback, but they’re currently very difficult to read in the list view, and aren’t visible at all in the detail view. I see that we can’t pass it in __construct(), but perhaps we could inject the QueuedJobDescriptor into the job itself via a setDescriptor() method so we have access to the model for writing these to the database?

Pseudo-code:

// QueuedJobService.php
protected function initialiseJob(QueuedJobDescriptor $jobDescriptor) {
    $impl = $jobDescriptor->Implementation;
    $job = Object::create($impl);
    $job->setDescriptor($jobDescriptor); // <-- Add this
    // ... etc
}

// AbstractQueuedJob.php
public function addMessage($message, $severity = 'INFO') {
    $this->getDescriptor()->addMessage($message, $severity);
}

// QueuedJobDescriptor.php
public function addMessage($message, $severity) {
    $this->Messages()->add(
        'Message' => $message,
        'Severity' => $severity,
        'Datetime' => date('Y-m-d H:i:s')
    );
}

That’d allow us to provide a much nicer default UI for the messages, and make it easier to customise.

Thoughts?

nyeholt commented 7 years ago

I'm not against the idea, but it does need to be looked at against the context of why it was there in the first place. Originally it was just meant to capture any details of what the management service was doing around the job; however it then became a bit more 'loggy' than was probably intended. And I'd rather avoid having log messages filling up the database, when they should probably be logged to an external mechanism somehow.

From an API point of view, I'd rather the job class didn't know at all about its descriptor, so the mapping of the message list back to objects would be back at the job service level.

robbieaverill commented 7 years ago

In a SS4 context, could you handle this using a custom monolog handler?

maxime-rainville commented 4 years ago

Need to validate if this is still a problem for ss4.