Track the number of times a queue item is claimed and pass as an optional param to queue workers

I'm started the work on the issue fork from 📌 BatchQueue::claimItem() can get stuck with huge data in batch queue Needs review since we need a similar change anyhow

pwolanin → changed the visibility of the branch 3517969-track-the-number to hidden.

The proposed fix would also resolve ✨ Cron queue workers should receive queued item creation data Active

I made a rough start on a new interface. Probably we should define a specific object class that is returned by a queue to represent an item so that non-database queue implementations can handle this however they want and the queue worker is ignorant of those details.

Thinking though this - it might be preferable if the new interface extends the old one so that the queue workers adopting the new interface are still required to be compatible with older code that's not calling the new method. They should still work with the data alone.

Add a MockTime class per discussion with @longwave and

still needs an update hook

Just want to plug Dead Letter Queue → here, which solves this problem in contrib.

@dieterholvoet thanks - took a quick look. Yes, that seems similar, but it's taking a very specific action based on a fixed threshold for failures. The idea here is that core is not going to take any action or have a threshold, but this enables the queue worker to have that logic if desired.

I do think it's interesting that the number of tries is decremented if the item is released in that module's code

I think this looks good.
We deliberated that modules can support both versions by indirectly implementing the interface by extending the base class and then just add the new method which can then take advantage of this change. But as the documentation on the method signature states, it should also just work without the new object.

This issue is not about deprecating processItem but maybe we can have a discussion about that in a followup issue.

Do we really need a new interface and method here? Why not just add the metadata as an optional argument to the processItem function and add it to the interface in a new major?

@dieterholvoet I discussed quite a bit with bircher. The approach in the patch mean that you can have the same queue worker class be valid and work in both D10 and 11 as long as it extends the base class so this be just a small feature addition in 11 and not break anything.

I'm not sure in terms of current core BC and deprecation policy if we'd ever just add the argument to the existing interface since that would break all the existing implementations. PHP doesn't allow you to add even an optional argument to the interface without it being also added in all the implementations.

At some future point we could deprecate the processItem() method and just support the new method and interface?

Why not postpone adding the argument to the interface until the next major version and until then make it an optional argument in worker classes like this:

public function processItem($data, ?object $metadata = NULL): void

calling the method like this:

$worker->processItem($item->data, $metadata);

That way it works both for workers with and without the method and the necessary changes to queue workers will be a lot more straightforward.

At some future point we could deprecate the processItem() method and just support the new method and interface?

If you're going to eventually do a breaking change, might as well stick to the same interface and method name and make the breaking change the addition of the new argument to the existing method, no?

Why would I want to wait for Drupal 12 or later when we can solve the problem now? I'm proposing this because I need this kind of feature to solve problems with queue processing sooner than later. I'd rather see it in core now to benefit everyone.

I understand it would be possible to follow the path outlined in the docs to add a new argument that would be an actual part of the interface in 12. I think this is what you are suggesting? https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →

I'dd defer to the commiters if they think that's the right choice.

Yes it's possible to add an argument to an interface method, there's also some docs at https://www.drupal.org/node/3376455 →

I haven't reviewed the issue in depth, but from the MR it looks like that's worth exploring here since it might result in less changes overall.

Should QueueInterface::claimItem() return mapping be updated to include the claim_count?
Can we add a phpstan return type showing the object shape?

Can we gurantee that the claim_count is available on all possible queue types such that putting this on the primary interface will not cause issues?

I don't see an answer to #9 regarding the metadata object lcass, if a dedicated object is not created can the new parameter be object-shaped to aid phpstan diagnosis of what properties are available?

@cmlara - those are all good questions, but seem out of scope here.

This system is pretty much unchanged since Drupal 7, and this is a small improvement that fixes a related performance issue and makes it possible to track the times claimed.

If we want to enforce a specific return type for the item across queue implementations I think that's going to be more disruptive, can we have a follow-up issue to look at that possibility? It would depend on the direction we go here.

Started on a second MR to compare. The total lines changes will likely be more since this requires fixing up some core tests like core/tests/Drupal/Tests/Core/CronTest.php that fail due to a mock not expecting a second parameter being passed in.

those are all good questions, but seem out of scope here.

This system is pretty much unchanged since Drupal 7, and this is a small improvement that fixes a related performance issue and makes it possible to track the times claimed.

We appear to be extending extending the data model of claimItem() (at least for the Database Queue though in theory all of the ecosystem) it would seem we should add the documentation (in this case just a comment addition) at the time we do such an extension. To my knowledge core does not usually create "add API documentation later" issues for new features.

If we want to enforce a specific return type for the item across queue implementations I think that's going to be more disruptive, can we have a follow-up issue to look at that possibility? It would depend on the direction we go here.

From what I can see the proposals are taking the queue service data, which was essentially defacto internal to Drupal Core only, and making it public to all contrib. If we open this up as "anything goes" we can not easily close it back down again in the future.

I do agree having a specific return type will indeed be significantly more disruptive. The more minimal of having the parameter shape of $metadata well documented with the required/optional parameters however would be minimal/non disruptive and would ensure every Queue provider adheres to the defined standards.

This will also help contrib working with the metadata, Core may not encounter however those of us in Contrib using PHPStan L9 already deal with "array key does not exist" and similar. If this key is expected to exist it should be on the parameter schema. These issues will impact core some day in the future, now is the time to be 'thinking ahead' to reduce the workload for moving up in PHPStan levels for core.

Since the returned object is anything (usually stdClass) I'm not sure how you are suggesting we document the expected properties?

The previous expected properties were just the item_id and data (though not really documented - a back-end could expected any other properties needed to update/remove an item from the queue).

I don't really think the queue data was internal to core - we have a couple different services which process queues similar to Cron.php but on different schedules (including spawning parallel processes).

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I merged 11.x to both branches -I don't think there was any an actual conflict