Thanks!
We have been using a variant on this patch for a long time in production, would love to see it bakcport to 10.x so we don't have to keep applying a core patch.
jdleonard → credited pwolanin → .
I merged 11.x to both branches -I don't think there was any an actual conflict
Yes, I think it's related to the closed issue, but I still think defensive coding is better than checking for exactly TRUE.
Possibly seeing this due to installing the site in our CI from existing config.
pwolanin → created an issue.
What's wrong with the original MR that tries to pop off the extra request?
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).
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.
pwolanin → created an issue. See original summary → .
@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.
The tests are running also on 9 now and passing with some tweaks to make it work across 9 and 10
I made an issue fork and included patch #7
We suddenly started getting this error - trying to track down why, maybe something wrong will file system permissions. It solved the issue enough to get things working and is clearly a pretty safe change
pwolanin → created an issue.
borisson_ → credited pwolanin → .
See ✨ Track the number of times a queue item is claimed and pass as an optional param to queue workers Active which includes this change and addition improvments, though maybe not the suggested comment.
I'd like to be able to close this as duplicate if that gets commited.
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.
Well, it has a MR now, but looks like not even code style is passing.
@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?
pwolanin → changed the visibility of the branch 3476224-target-id-only to hidden.
pwolanin → changed the visibility of the branch 3476224-jsonapi-assumes-entity to hidden.
Discussed with @tstoeckler in person who has run into similar issues using jsonapi. His feeling was the the branch where we've started adding the test is the best way to move forward - basically assuming that all entity reference fields have an "entity" computed field that we can use to associate the loaded entity
No objections to this change, so I am going to do it
ok, also sounds like moving queue_unique into core is something that could be accepted, so we just need to make a MR here
I'll see if they will add me as a maintainer there, but it's simple enough we could likely merge into core
Since we've moved from phpass to the native PHP hashing this is relevant but likely needs a little different code change.
I see in this test file that we set a different cost parameter:
core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.services.yml
The proposal here is to have a way for all tests to set this to a low value which would save time and cpu on every user login in every functional/e2e test
@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
still needs an update hook
Add a MockTime class per discussion with @longwave and
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.
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.
The proposed fix would also resolve ✨ Cron queue workers should receive queued item creation data Active
pwolanin → changed the visibility of the branch 3517969-track-the-number to hidden.
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 → created an issue.
If you want to avoid duplicate items, looks at the queue_unique contrib module
I just opened an issue fork against 11.x and made what I think are the simple changes needed for the DatabaseQueue
pwolanin → made their first commit to this issue’s fork.
IMHO - a queue is actually the wrong solution unless core adds something like https://www.drupal.org/project/queue_unique →
The problem is that a single content item may be updated multiple times while it's in the queue, but you only want to re-index it once when it's turn comes around
the queue_unique contrib module already handles this - it ignores a new queue item if there is an existing one with the same data (or really the same sha256 of the serialized data)
As a partial solution, have you looked at using queue_unique to prevent the addition of duplicate queue items?
There is nothing in the module that generates a JWT with this claim. Usually you'd have another application or some custom code generating the JWT based on access to a shared key.
Seems like this needs to be clarified in the docs?
pwolanin → created an issue.
pwolanin → created an issue.
pwolanin → created an issue.
pwolanin → created an issue.
pwolanin → created an issue.
pwolanin → created an issue.
pwolanin → created an issue.
patch #61 is failing for my colleague when filtering with jsonapi on the value of a referenced entity referenced by the main entity.
It's writing the WHERE clause such that it's filtering the main entity to the node ID of the referenced entity.
example:
- main entity house.
- house references a city node
- city references a state node
if I filter houses in jsonapi by state, the SQL where clause is filtering the house node ID by the state node ID.
greggles → credited pwolanin → .
So the difference is the \stdClass is one case where dynamic properties are allowed. I don't think there is a way to typehint the #[AllowDynamicProperties] attribute, however.
https://php.watch/versions/8.2/dynamic-properties-deprecated
Added one line to the gitlab config file
pwolanin → created an issue.
Yes, this should work. JWT modules does not generally check if the user has a session - perhaps it should!
You have to generate the JWT and pick the expiration time. Beware than anyone with access to that link will be able to download the provate file until the JWT expires
this would go in 3.x at this point
forward ported to 3.x also
This seems more-or-less duplicate to this bug reported several years ago:
#3189827: Access denied with bearer token on oauth/debug when Jwt is installed →
For this level of change I think it will need to go in 3.x. 2.x is just being minimally maintained
This is an old issue but increasingly relevant. Since it could require changes (adding a Key ID to JWT) it would also be good to handle before making 3.x stable:
✨
Support multiple keys
Active
I pulled all the trivial test and style changes over (getting tests to run on Drupal 9 and 10) to 📌 Get 2.x tests running on both Drupal 9 and 10 Active
Looks like the test pass - not really anything important otherwise, so merging