Unify our two task systems

Tackling this now, but currently in the process of succumbing to another onset of megalomania. (I.e., I'm revamping the two task systems into a single, more flexible one.)

Something like the attached. It uses events to execute the different types of tasks, thus staying fully flexible.

Still to do:

• Actually using this for datasource config changes.
• Replacing the "Execute pending tasks" local action with one for all types of tasks.
• Same for the status report.

Still setting to "Needs review" to a) see whether the test bot is happy so far (the new functionality still needs tests, though) and b) to maybe get some feedback on what I've got so far. Will probably not work on it again until Monday or Tuesday.

So I did.
Damn.
Updated patch coming soon anyways, but I'm still interested in the test results here.

Ah, now I get it! Forgot the ".patch" extension for the patch file so it got rejected. (Blame my thousands of small scripts which usually take care of this for me.)

I'd think that maybe we'd want to create a value object for executeSingleTask's argument instead of just using an array? Not sure though.

I don't think so. First of all, we'd need to do this for all $conditions arguments for the class's methods. And second, I don't think this is done anywhere in Drupal Core. Conditions on properties are still expressed with associative arrays, as far as I can see (e.g., \Drupal\Core\Entity\EntityStorageInterface::loadByProperties()).

The failures mostly seem to be a single syntax error in services.yml – which PhpStorm doesn't spot, for some reason. But you might as well wait for a thorough review until I have a patch I'd call RTBC. As long as you agree that the basic direction makes sense, nothing major will hopefully pop up then.

And several more small mistakes, ranging from tricky to dilettantish.

This adds the original objective of integration with datasource item tracking and also passes all PhpUnit tests, at least locally.

To do: local action, status report and tests.

Discovered #2727667: Subsequent submit callbacks are handled incorrectly after batch is added and #2727697: Changes to the index are discarded when it is serialized, but in the end they both don't really apply here, since it's a bad idea to create the batch as soon as $datasource->setConfiguration() is called anyways – we should change the tracking only once the index has actually been saved, not just when a property is changed (which might not be saved after all).
This was, by the way, wrong before, too. However, I have to say this solution is a lot less elegant, with relying on a hook implementation completely separated from the rest of the datasource code. But to solve that, we'd have to either add some "index save" reaction method for datasources (or all index plugins), which might be a bit overkill (or pretty nice to have in general, too – would have to be determined) or re-think the whole "write property changes only on index save" mechanism (and we're not doing that unless we really have to – was enough effort getting it to work in the first place).

Anyways, worked the whole day on this, and still not finished. But the attached patch at least changes the local task and status report functionality to encompass all kinds of tasks, and should also fix a few more bugs. Hopefully I'll be able to finish this tomorrow, by fixing the remaining test fails and adding a few tests for the new functionality.

Had to fix #2574881: Fix tracking race conditions by making tracker ignore duplicate inserts at the same time, too.
But at least that's also a beta blocker, so we get two for one here.

Wait, huh? This looks weird. Can we do $this->batchTracking++ instead? I had no idea this even worked.

I dislike the postfix increment operators, and precedence-wise there shouldn't be any problem since -> has always had highest operator precedence, as far as I know (even though it's unfortunately not listed in the PHP docs). I didn't even really think about it, to be honest.
However, if it looks weird to you, it probably also does for others, so I changed it. Better I find it ugly than anyone finding it confusing. ;)

I recently got a comment that we shouldn't introduce new objects while throwing exceptions. see #2722271: Make sure as little code as possible is triggered when throwing an exception.

Oh, you're right, apparently this has even been in our coding standards for several months now! Created #2728399: Do not use FormattableMarkup in exceptions to take care of this project-wide.
But fixed it in this patch, too. Thanks!

Again, much more expressive, love this!

Thanks, glad to hear it. I thought so, too, and it fixes nested calls elegantly (though they're probably an edge case anyways).

The attached patch implements those changes and should also, finally, fix all tests. (Additional tests are still needed, though.)

Damn Postgres tests …
Here's to hoping #2709899: php7 + postgres throws AlreadyInstalledException randomly. will get fixed soon!

Tests added, both for the general task manager functionality and the content entity datasource-specific code.

From my point of view, this is RTBC – eager to hear other comments, though, of course.

I don't understand why this change is needed?

So PhpStorm doesn't complain, since condition() is documented to return \Drupal\Core\Database\Query\ConditionInterface, and not $this – therefore, PhpStorm doesn't think it has an execute() method.

Is there a reason why these aren't in order?

Yes, because we want to make sure the correct ones are skipped when we apply conditions. If we just create them in the order they should be executed, we wouldn't really test the $conditions parameter at all.
But it probably deserves some explanation in a comment.

useless change

First off, no, it changes server_id to allow NULL.
Secondly, the order makes more sense this way, now that we always have a type, but the server might be missing. It's no functional difference, sure, but why should that stop us?

the same, should be FQDN

Ah, yes, thanks for spotting that!

Great to hear, thanks for reviewing!
Committed.

Postgres is failing (apparently for real, this time) after this commit. The attached patch completely reverts the commit – let's see whether that would fix it.

Now a dummy patch to see the apparent test failures.

Maybe this is the culprit?

Probably that's a pg related issue, suppose better to file follow-up

Of course it's a Postgres-related issue – that's why all tests pass in the other two environments. And yes, we're aware of the random fails: #2709899: php7 + postgres throws AlreadyInstalledException randomly.. But this is something different, the tests are really failing, and I'd like to fix that as soon as possible to avoid adding even more Postgres fails on top of those.

I have now managed to finally install Postgres locally and am in the process of debugging this issue.

From what I can tell, there is just a glaring bug in Core's Entity Query implementation for Postgres. Switching to a normal DB query solved it for me locally (at least one test). Let's see what the test bot says.

Oh, er, like this, I mean.
But anyways, not a proper solution anyways.

This might actually be an option until the Core bug gets fixed.
(Gotta reproduce it with a Core entity type and then file it, first, though.)

Couldn't reproduce it properly, unfortunately, but created the issue: #2734445: Postgres entity queries doesn't treat multiple IN conditions correctly.
And if no-one objects I'd like to just commit this workaround for now and then go on to more productive issues.

OK, good. Then: committed.
Thanks for your feedback!