Establish conventions on events and entity lifecycle

[asmecher]

Now that we have adopted

• foreign key declarations including ON DELETE CASCADE clauses
• greater use of queued tasks, meaning the order and timing of operations is becoming less predictable, and
• greater use of events, which allow us to decouple "me-too" code from controllers and models (e.g. updating the search index on submission lifecycle events),

...we should establish some best practices around how/when lifecycle events, events, and queued tasks interact.

Questions:
(Each question has a thread below, please add your comments to the appropriate thread.)

• When do we prefer passing around objects? When do we prefer IDs? How do we decide?
• When should events get triggered in relation to CRUD operations?
• Would we have "before" and "after" events? If so, how do we manage code multiplication?
• How do we ensure "out-of-order" operations don't cause boundary condition failures? (For example, a serialized object in a job falls out of sync with the actual database conditions, causing hard-to-reproduce failures in job code that doesn't know how to handle the mismatch)

Proposal PRs (WIP -- please check with Alec before doing anything with them):

• https://github.com/pkp/pkp-lib/pull/8398/files
• https://github.com/pkp/ojs/pull/3601/checks

[asmecher]

Looking at Laravel's model conventions, they allow events to be processed both "before" and "after" a CRUD operation.

• Terminology: deleting / deleted; updating / updated; etc.

Laravel supports both string-based (named) and object-based events. Built-in model events (CRUD operations) are currently implemented using string events. See this StackOverflow discussion on the merits of string vs. object (just as an example).

Our current event discovery/caching toolset does not appear to support string-based events.

(@ewhanson notes this documentation on interlinking Composer's autoloader with Laravel service provider toolset. This might provide an alternative to our current scan-and-cache event registrant discovery toolset.)

[asmecher]

Following up on @ewhanson's suggestion of using Composer's autoloader tools -- I don't think we can do that directly, because plugins need to be installable from a web request, and Composer doesn't currently have standard ways to install new dependencies at runtime. (See Composer's list of runtime tools and some proposed approaches to invoking Composer from a web request.) As much as I like the idea of handing a lot of that stuff off to Composer, I'd hate to adopt an "unconventional-but-works" strategy that later gets broken by Composer's expectations that it's invoked via CLI.

However, the approach of having plugins e.g. register their own service providers could be good. This might allow us to use string-based events without further ado.

[touhidurabir]

the approach of having plugins e.g. register their own service providers could be good. This might allow us to use string-based events without further ado.

+1 for this approach . This is not only allow for event attachment/manipulation but can also open more future possibilities for plugins .

[NateWr]

When do we prefer passing around objects? When do we prefer IDs? How do we decide?

[NateWr]

In my opinion, we should follow these rules:

1. Always pass objects unless there is a compelling reason not to.
2. Never pass the Request object (but do include the User object for the current request).
3. Never serialize an event into the database with a job, unless you have to.

[jonasraoni]

I think it's better to pass objects, mostly due to convenience (e.g. might help to avoid doing extra queries).
Also, if we end up needing a "before create", then passing an object will be the only way :)

[touhidurabir]

Always pass objects unless there is a compelling reason not to.

it's better to pass objects, mostly due to convenience (e.g. might help to avoid doing extra queries).

100% agree with this approach . But need to be careful what is getting passed to event listener as it is possible to have queue listener and make sense to have those if the listener only going to dispatch job/s as in that case we need to make sure that object does not contain any closure(object serialization will failed in that case). Any of the event listener can be converted to queue listener which will work just as a queue job very easily which we should adopt as we have adopt more queue job based implementations .

[NateWr]

I agree about taking care with queued listeners. I'd even suggest that we set a convention that a listener is never directly queued. Instead, a listener should create a job if it wants to queue its work. That will give us a clean separation between events (synchronous) and jobs (asynchronous) that might help us avoid problems.

But it does mean more boilerplate code, and it may depart from common Laravel conventions. So I'm happy to hear pushback on this if it doesn't sound like a good idea.

[jonasraoni]

This problem also exists in the frontend sphere (lack of control over the execution order of handlers) 😂

Given that a priority-based list isn't perfect (e.g. two plugins asking to be the first/last), I think it's better to just trust people will use the before/during/after events properly.

[NateWr]

When should events get triggered in relation to CRUD operations?

[NateWr]

I'm in favor of option B: repository class always handles events. Of course there will be exceptions but as a rule I think it is best.

One thing we need to consider, though, is passing dependencies into repository methods. Right now, repository methods have the fewest method arguments required:

namespace PKP\publication;

class Repository
{
    public function add(Publication $publication): int;
}

But if we're to fire events with all the useful dependencies, those will need to be passed in:

namespace PKP\publication;

class Repository
{
    public function add(
        Publication $publication, 
        Submission $submission, 
        Context $context, 
        User $createdBy
    ): int;
}

I think this will be a general improvement that we should pursue for all of our repositories. @ewhanson ran into precisely this problem with his work on DOI versioning, which is making use of the events system.

Passing in these dependencies will further de-couple the repositories from the calling code (usually handlers), so it will be easier to use them directly in CLI tools and other places where there is no Request.

[asmecher]

passing dependencies into repository methods

Hmm, this risks bloating parameter lists. For example, creating a tombstone on submission deletion requires the submission, section, and journal. The journal and submission will generally already be available in calling code, but the section isn't. Currently, the submission deletion event only provides the submission, but the journal and section are easily fetched from the DB (as e.g. the $submission and $context could be in your example above).

I think the goal in what you're proposing is to avoid repeated fetching of objects from the database. That's a caching/optimization problem and IMO should be solved as such, only for cases where it's important, rather than forcing callers to provide a long string of parameters.

(We have a long history of adding cruft to parameter lists for specific use cases, and these are hard to understand later or undo in case of refactor.)

[touhidurabir]

the only difference from our hook system would be the lack of a "priority" (the hookSequence), which has been removed from Laravel

Some context regarding to this, if we really want to have a priority based system but something more modern other than hook, how about go for chained jobs(https://laravel.com/docs/9.x/queues#job-chaining) ? So if we have several classes (similar concept to listener ) that need to run in specific order , we can have those run in order as chained jobs and run in sync mode easily as sync jobs . we can either call those chain jobs form a listener class or from the constructor of the event class(personally against this approach) .

[touhidurabir]

how we'll evolve the relationship between hooks and events

Regarding to this, I also support the Option B . Event and Hook should placed in Repository classes and DAO should be decoupled from this kind of business logic.

One thing we need to consider, though, is passing dependencies into repository methods...

This probably will cause more confusion later than solving what we have now . One way is to use caching but that will only get more complicated with time if use extensively. Another way is to code carefully and sometime sacrifice bit of reusability.

[NateWr]

how about go for chained jobs

With our use cases, we won't know all of the jobs in advance because a plugin or third-party code needs to be able to "hook in" to perform actions. An example would be a plugin that wrote data to the submission when it is published:

1. Published event is fired
2. Listener in plugin writes data to the submission.
3. Listener in core application sends submission data to a third-party indexing service.

When 3 happens, any changes to the submission data should have already occurred so that the submission data is accurate when it is sent. Without a priority system, there's no way to say "run this listener after all other listeners".

[NateWr]

Would we have "before" and "after" events? If so, how do we manage code multiplication?

[NateWr]

From @asmecher:

Looking at Laravel's model conventions, they allow events to be processed both "before" and "after" a CRUD operation.

• Terminology: deleting / deleted; updating / updated; etc.

[NateWr]

I agree, we need before/after at least for deleted. Currently that's how we implement hooks. But add/edit operations just have a single hook.

[jonasraoni]

We don't have full control over the code (e.g. unknown plugins listening for events), therefore it's not possible to guess what the listeners might need in advance. Having a before/after event solves the problem (e.g. grab some data before it's removed).

[NateWr]

How do we ensure "out-of-order" operations don't cause boundary condition failures? (For example, a serialized object in a job falls out of sync with the actual database conditions, causing hard-to-reproduce failures in job code that doesn't know how to handle the mismatch)

[NateWr]

Part of this is unavoidable in jobs. The code has to be written carefully so that it is really self-contained within the job. It should not be "reaching out" to get things like the request, the context, a user object, etc. When it does so, it needs to be coded to assume that such objects won't exist.

It takes some getting used to, but is not difficult to do once you get your head around it.

[jonasraoni]

I'm not sure I understood the "out-of-order" problem, but if a task depends on another to succeed, then they should be tied somehow (possibly through another event).

Synchronization issues might happen whenever we copy/load data to/from somewhere, and there's no solution that fits all cases. It's better to minimize the surface by just storing what's really needed (notice: I didn't see how the jobs are working).

About the context/user: I agree, we should avoid retrieving information from singletons, data should be received/injected whenever possible.

[NateWr]

The more I think of it, the more I think it's not a problem with events. It's a problem with jobs. Here's an example with psuedo code of an out-of-order error.

class RemindReviewers extends ScheduledTask
{
    public function execute()
    {
        /** @var ReviewAssignment[] */
        $overdueReviewAssignments = $this->getOverdueReviewAssignments();

        dispatch(new SendReviewerReminderEmail($overdueReviewAssignments));
    }
}

class SendReviewerReminderEmail
{
    public $overdueReviewAssignments = [];

    public function __construct(array $overdueReviewAssignments)
    {
        $this->overdueReviewAssignments = $overdueReviewAssignments;
    }

    public function handle()
    {
        foreach ($this->overdueReviewAssignments as $assignment) {
            $reviewer = $this->getUser($assignment);
            /**
             * If we're not careful, `$reviewer` may be `null`. It's possible
             * that between when the review assignments were fetched and 
             * $this->getUser($assignment) was called, the reviewer has been
             * deleted.
             */
            $mailable = new RemindReviewer($assignment, $reviewer);
            Mail::send($mailable);
        }
    }
}

This problem is not unique to events. But Laravel encourages you to send an event listener directly to the jobs queue. So it is likely that jobs will be used more if we fire events for all CRUD operations.

[asmecher]

I think I'm agreed with the above:

• being careful with data that may be out of sync with event handler expectations is inevitable, and
• out-of-order execution is a jobs problem, not (yet) an events problem.