Account created on 17 February 2006, about 19 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States 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.

🇺🇸United States pwolanin

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.

🇺🇸United States pwolanin

Well, it has a MR now, but looks like not even code style is passing.

🇺🇸United States pwolanin

@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?

🇺🇸United States pwolanin

pwolanin changed the visibility of the branch 3476224-target-id-only to hidden.

🇺🇸United States pwolanin

pwolanin changed the visibility of the branch 3476224-jsonapi-assumes-entity to hidden.

🇺🇸United States pwolanin

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

🇺🇸United States pwolanin

No objections to this change, so I am going to do it

🇺🇸United States pwolanin

ok, also sounds like moving queue_unique into core is something that could be accepted, so we just need to make a MR here

🇺🇸United States pwolanin

I'll see if they will add me as a maintainer there, but it's simple enough we could likely merge into core

🇺🇸United States pwolanin

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

🇺🇸United States pwolanin

@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

🇺🇸United States pwolanin

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.

🇺🇸United States pwolanin

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.

🇺🇸United States pwolanin

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

🇺🇸United States pwolanin

If you want to avoid duplicate items, looks at the queue_unique contrib module

🇺🇸United States pwolanin

I just opened an issue fork against 11.x and made what I think are the simple changes needed for the DatabaseQueue

🇺🇸United States pwolanin

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

🇺🇸United States pwolanin

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)

https://www.drupal.org/project/queue_unique

🇺🇸United States pwolanin

As a partial solution, have you looked at using queue_unique to prevent the addition of duplicate queue items?

🇺🇸United States pwolanin

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?

🇺🇸United States pwolanin

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.

🇺🇸United States 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

🇺🇸United States pwolanin

Added one line to the gitlab config file

🇺🇸United States pwolanin

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

🇺🇸United States pwolanin

For this level of change I think it will need to go in 3.x. 2.x is just being minimally maintained

🇺🇸United States pwolanin

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

🇺🇸United States pwolanin

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

🇺🇸United States pwolanin

Looks like the test pass - not really anything important otherwise, so merging

🇺🇸United States pwolanin

I was playing around with the CI yesterday, sorry for the noise.

Yes, the goal is to get something ready for this branch soon.

Personally I want to at least get these 2 issues into the branch first:

🇺🇸United States pwolanin

marking as 3.x since this seems too big for 2.x at this point which is probably EOL with Drupal 10

🇺🇸United States pwolanin

Is this the module that you see conflicting? https://www.drupal.org/project/simple_oauth

A further approach could be a setting in settings.php to change the default header (or default and fallback headers) that would come via \Drupal\Core\Site\Settings which we are already injecting into this class

🇺🇸United States pwolanin

Something like this - passes a quick check, and maybe the minimum size of each segment should be larger.

🇺🇸United States pwolanin

This patch would break all existing sites, so it is not acceptable.

Clearly we already support the 'JWT-Authorization' header.

Since Oauth2 may be using a JWT as the token, I'm not sure there is an easy way to make these compatible.

A partial fix could be to change the method \Drupal\jwt\Authentication\Provider\JwtAuth::getJwtFromRequest() and make the regex more specific to the pattern of a JWT so it would ignore some other kind of token being used by OAuth2

🇺🇸United States pwolanin

pwolanin made their first commit to this issue’s fork.

🇺🇸United States pwolanin

I added dieterholvoet as a maintainer with full acccess

🇺🇸United States pwolanin

Looks like I could do this - I am a maintainer, but don't technically have "administer maintainers" on there. however, I can do it via my security team role

🇺🇸United States pwolanin

@daffie - yes, we need to support a lot of authenticated users, but a web application may not behave like a social media site in terms of the kind of data you need to load and show. Still, for our uses we are very much pained by the slowness of loading (and even more) saving entity data en mass. So, entity/field API performance one of our biggest pain points. We also use custom SQL queries at points, so it's not trivial to drop mongoDB in.

More personally in response - it feels ever more intimidating to get involved in core issues and it seems like a lot of good ideas linger with years of discussion. As a (listed) subsystem maintainer I don't feel like I have any special authority or reason to stay involved. Perhaps the core strategy will help define some technical directions that will reduce circular discussions on issues and encourage involvement.

🇺🇸United States pwolanin

@catch - that sounds like a lot of thing that may not come to pass.

Also - if I'm building a data-oriented web app, I don't think I'd start from Drupal CMS, and I think there needs to still be consideration for all the use cases that want to start from a reasonably fully functional Drupal core.

So, at the very least this issue sounds premature until the Drupal CMS being stable with a good search_api recipe is implemented. At that point, it sounds like the only concern is around user confusion?

🇺🇸United States pwolanin

Took a look at the 11.x MR. Looks like a simple and sensible fix, so moving back to RTBC.

🇺🇸United States pwolanin

This seems like a bad idea to me. The functionality of this module seems pretty essential for Drupal core to have a minima feature set.

🇺🇸United States pwolanin

There are interesting ideas from symfony about what to support. Their style of key rotation doesn't seem great for us where values are likely in the database an prod rotation happens only on the live site.

🇺🇸United States pwolanin

There is another request for a container parameter. The goal here is just to make it work better for everyone by default in a simple way

Production build 0.71.5 2024