Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Like in D7 (since December 2013, at least), we should remember all failed operations in a server plugin (deleting items, adding/removing an index, …) in the server tasks system (dedicated table) and re-try them regularly (specifically, before doing any indexing, since we don't want to index anything while tasks are pending).
Comment | File | Size | Author |
---|---|---|---|
#10 | 2230925-server-tasks-10.patch | 27.26 KB | Berdir |
#8 | interdiff.txt | 7.41 KB | amateescu |
#8 | 2230925-server-tasks-8.patch | 29.04 KB | amateescu |
Comments
Comment #1
drunken monkeyThis definitely needs to be done, and tested.
Comment #2
Nick_vhComment #3
Lukas von BlarerLooking into this...
Comment #4
Lukas von BlarerI did complete a part of the job. Remaining is getting the execute() method of the ServerTasks class working and executing it on cron run, before indexing items and before executing any action that could create tasks.
Comment #5
Lukas von BlarerAttaching a functional but not fully tested patch. Missing are tests.
This patch also includes a port of the
hook_cron
implementation and a change toServiceSpecificInterface
to make the index parameter of thedeleteAllItems
method required to prevent accidental deletion of read only indexes.Comment #6
Lukas von BlarerRe-rolling...
Comment #7
drunken monkeyAttached is a slightly improved version.
One thing still missing is calling
execute()
before calling any of the backend methods that could use exceptions, and failing if that returnedFALSE
.And, of course, tests.
Here are some remarks about my changes:
You forgot to import the exception class in some cases. But you're using an IDE, right? That should normally report such problems, maybe check your settings.
When changing the parameters of a method, please remember to also update the doc!
Since you want to load entities and are in a service, you should also dependency-inject the entity manager.
Also, it's good practice to run the tests before posting a patch (or, even more, committing, of course). That would probably have caught some of these.
Comment #8
amateescu CreditAttribution: amateescu commentedStarted to look into finishing this, but I'm going to post another cleanup first. The most important change is that we don't usually use plurals in class/service names, and the ServerTask class introduced here is often called a manager, even if its not a plugin manager.
Comment #9
drunken monkey"ServerTaskManager" makes sense, thanks!
Regarding test scenarios: Didn't Lukas already start work on this? I remember dicussing it with him, at least.
The idea is to give our test backend plugin a setting which makes it throw exceptions for all/some methods. Then set that, call the various methods on the server class that have task integration and see if tasks are created. See whether the tasks are properly executed when one of the methods is called (probably by giving the test backend also the functionality to store which methods are called on it, so we can check whether the right ones were called). Check that while executing pending tasks fails it's not possible to execute any new operation (i.e., they get added to the pending tasks automatically without calling the backend method first). Maybe also check that tasks for all servers are executed during cron runs, but that's optional.
Finally, unset the test backend plugin setting and check whether executing tasks correctly deletes them from the table, and whether calling the methods afterwards works normally.
Come to think of it, when a task in
ServerTaskManager::execute()
currently fails, do we still try to execute the others for that server? If so, that should be changed, tasks should only be executed in the order they were created in, and thus only if all previously saved tasks were successfully completed (or made redundant by other tasks, as sometimes happens).Tasks for other servers can of course still be executed, if a task on one server fails.
Since I'm just seeing this: this (and possible other instances of this pattern) should also pass
$e
as the third parameter when constructing the exception, so we have a proper chain.(But should probably be done in another issue/commit, of course.)
Comment #10
BerdirRe-rolled and pushed to 2230925-server-tasks.
Comment #11
drunken monkeyOK, let's do this!
Comment #14
drunken monkeyEverything seems to work fine – committed.