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

Merge Requests

More

Recent comments

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

🇺🇸United States pwolanin

@ro-no-lo sounds like you have more of a support request - maybe try Slack?

We are using the JWT path auth in production for accessing private files, so I assure you that it can work.

🇺🇸United States pwolanin

Not sure I follow. There are similar examples in core that don't have a mapping, and a couple that do

15-
16-views.field.moderation_state_field:
17:  type: views.field.field
18-  label: 'Moderation state field'

core/modules/comment/config/schema/comment.views.schema.yml
21-  label: 'Comment bulk form'
22-
23-views.field.commented_entity:
24:  type: views.field.field
25-  label: 'Commented entity'
26-
🇺🇸United States pwolanin

Doing a quick test - I am still getting the OOM with that hook removed. Will double check with the specific diff applied.

Not sure why it would be related? As I mentioned above this seems related to a change in the config schema in the 3x series and is being triggered by config translation code.

🇺🇸United States pwolanin

Node module is also responding to various changes in hooks (e.g. comment update) and calling node_reindex_node_search()

This feels like a similar case, so I think the approach in the MR is reasonable given the limitations of the system.

🇺🇸United States pwolanin

@drumm could you point to the code involved in project module? Is there some way we could make it possible to pick the release "series" when I'm making a new release?

🇺🇸United States pwolanin

In the users_jwt module I disallow an expiry more than 24 hours in the future, so I think that would be a reasonable max here also (at least for the form).

🇺🇸United States pwolanin

I agree with berdir here - pretty much every contrib release is a minor release and the availability of patch releases is a nice to have but not even necessary for most cases.

It seem like a "series" of releases used to be based on an actual git branch?

If I want to have the behavior currently being forced here I'd expect to need to create 2.0.x, 2.1.x, etc branches for each minor.

The fact that I have to specifically flag the old minor as supported and have it show on the project page seems especially weird and terrible. I don't need it to be unsupported unless it has a security vuln, but I certainly don't want it to show as a suggested download.

🇺🇸United States pwolanin

merged to 2.x and will cherry-pick to 3.x

Please make a 3.x issue to remove the old methods.

🇺🇸United States pwolanin

not sure why it shows some commits against MR 12 as well as 15

Moved to target 3.x

🇺🇸United States pwolanin

Good idea - I didn't see this existing issue and started on this in
[#427911]

🇺🇸United States pwolanin

Can you include the hashed session ID as part of the JWT payload? That can't be used alone to hijack a session, but I worry it's kind of an internal detail that isn't really exposed as part of the session API

Also - digging into this before, it's seemed hard to bootstrap the session code if there is not an expected cookie.

🇺🇸United States pwolanin

Can you clarify - you got a session cookie after logging? How did you get the JWT?

🇺🇸United States pwolanin

Looks good, thanks for catching those warnings

🇺🇸United States pwolanin

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

🇺🇸United States pwolanin

finish git filter-repo instruction

🇺🇸United States pwolanin

Adding an alternate method to split out a core module using git filter-repo

🇺🇸United States pwolanin

@drumm - I should be able to run through this process within a day or two when it's possible

🇺🇸United States pwolanin

Gave this a quick review - seems like a fine test change to back to RTBC

🇺🇸United States pwolanin

Gave this quick review - not sure altering the field label is needed, but it's not a big deal

🇺🇸United States pwolanin

I just tried to create a module with short name "book" but I was rejected: "This project short name is reserved."

🇺🇸United States pwolanin

I could be listed as a co-maintainer with smutgave

🇺🇸United States pwolanin

If people are seeing this for real, please suggest a test case we can add. I'll note the bug is fixed in a different way in the 2.x branch

🇺🇸United States pwolanin

Looks good, thanks for the cleanup

🇺🇸United States pwolanin

The site is "multilingual" in as much as we have a custom version of English enabled to use to replace some of the interface text. And yes, the problem is related to processing the config overrides.

We do not have any of those modules (- geofield - entity_reference_revisions - search_api)

🇺🇸United States pwolanin

According to git bisect:
3e77a0d12a52856316f3e5da004af82e6572be9f is the first bad commit

https://git.drupalcode.org/project/rest_views/-/commit/3e77a0d12a5285631...

That added views.field.field_export to the config schema

🇺🇸United States pwolanin

Weird - the segfault (need to test OOM) seems to be happening in the setup for the foreach loop in \Drupal\locale\LocaleConfigManager::getTranslatableData():

    if ($element instanceof TraversableTypedDataInterface) {
      foreach ($element as $key => $property) {

the foreach is going to cause this to get called:

  /**
   * {@inheritdoc}
   */
  #[\ReturnTypeWillChange]
  public function getIterator() {
    return new \ArrayIterator($this->getElements());
  }

We seem then to be into the config schema

The config schema was changed in the 3.x branch here: https://git.drupalcode.org/project/rest_views/-/commit/a986dd394e2116f42...

🇺🇸United States pwolanin

The segfault happens in \Drupal\locale\LocaleConfigManager::getTranslatableData()

the element triggering it is: display.default.display_options.fields.field_location_related_orgs_export in the view
That looks like:

        field_location_related_orgs_export:
          id: field_location_related_orgs_export
          table: node__field_location_related_orgs
          field: field_location_related_orgs_export
          relationship: none
          group_type: group
          admin_label: ''
          plugin_id: field_export
          label: 'Used by'

That plugin is provided by rest_views

Production build 0.71.5 2024