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

xjm → credited pwolanin → .

larowlan → credited 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?

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).

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.

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

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

thanks alexpott!

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

Moved to target 3.x

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

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.

pwolanin → created an issue.

pwolanin → created an issue.

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

Looks, good - thanks!

Looks good, thanks for catching those warnings

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

finish git filter-repo instruction

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

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

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

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

Created 💬 Unreserve or create "book" module in ontrib Active

pwolanin → created an issue.

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

I could be listed as a co-maintainer with smutgave

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

How about this version?

Looks good, thanks for the cleanup

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)

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

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...

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

The error seems to happen in

\Drupal\locale\LocaleConfigManager::getTranslatableData()

which is a recursive method

It seems like the problem is locally caused by a view (config) that uses rest_views and also views_streaming_data to handle a csv export of the data.

Trying to reproduce locally, I'm getting a seg fault instead of OOM at exactly the same point :(

Looks like what's happening is drush is trying to process a batch, the callback is locale_config_batch_refresh_name(), and that is trying to update config translations.

That batch job looks like it is set by function locale_system_update() which runs after modules are installed

So, this seems like it would have been happening before. IDK why updating rest_views is causing an OOM or seg fault with this update.

Applying a patch to disable the hook in the module still gives an OOM, so it's probably not that hook just something else happening after modules are enabled or caches cleared

Note that some custom modules depend on the main rest_views module, but none depend on any sub-module of rest_views.

We also install rest_views as a dependency earlier in the process, so it's not newly enabled at the point we see the OOM

It is possible it's happening in some code called from this function, or some other implementation of that hook.

function rest_views_modules_installed($modules):

The OOM happens in CI as various modules are being enabled and I see a log message from one of our custom modules from it's implementation of that hook.

Oddly the OOM is always mentioning the same 2 files/lines:

[2024-02-05T18:51:36.610Z]  [info] Executing: /var/www/drupal/vendor/bin/drush batch-process 2 --uri=default --root=/var/www/drupal/web

[2024-02-05T18:51:36.949Z] >  [info] Switched to user 1

[2024-02-05T18:51:39.954Z] > PHP Fatal error:  Allowed memory size of 1572864000 bytes exhausted (tried to allocate 262144 bytes) in /var/www/drupal/web/core/lib/Drupal/Core/Config/TypedConfigManager.php on line 208

[2024-02-05T18:51:39.954Z] > PHP Fatal error:  Allowed memory size of 1572864000 bytes exhausted (tried to allocate 262144 bytes) in /var/www/drupal/vendor/symfony/http-foundation/Response.php on line 226

pwolanin → created an issue.

The logic change looks ok, but you changed the code style to be incorrect:

12 coding standards messages
✗ 12 more than branch result

src/Transcoder/JwtTranscoder.php ✗ 12 more
line 261	Line indented incorrectly; expected 6 spaces, found 8
262	Expected newline after closing brace
263	Line indented incorrectly; expected 6 spaces, found 8
264	Line indented incorrectly; expected 8 spaces, found 12
265	Line indented incorrectly; expected 6 spaces, found 8
265	Expected newline after closing brace
266	Line indented incorrectly; expected 8 spaces, found 12
267	Line indented incorrectly; expected 6 spaces, found 8
271	Line indented incorrectly; expected 2 spaces, found 0
271	Closing brace indented incorrectly; expected 2 spaces, found 0
271	Expected 1 blank line after function; 2 found
274	The closing brace for the class must have an empty line before it

Sounds like we should add typehints in the 3.x branch if we do that any time soon.

Option #1 is maybe fine... you can change the @return param comment even and document that it just returns what was added in setUser() / setAccount() ?

As far as tests... well maybe you are right that there is not much to test. You could have a Kernel test or something that sets a \Drupal\Core\Session\UserSession as the user in a test module event subscriber? Why do you need a non-core implementation?

Looks like a good addition - we should have some tests to show it works

The test module should probably not depend on itself

I think this is duplicate to the D10 update issue fixes? MR shows a merge conflict now

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

Yes, I don't see why you'd get that kind of error based on the patch.

I want to take one more look. Also, needs to target the 2.x branch

Seems like this is not a bug. You may be interested in this issue to be able to log some info about auth failures:
✨ Create a mechanism to log the decode exception in \Drupal\jwt\Authentication\Provider\JwtAuth::authenticate() Needs review

Looks like an issue with vendor library and maybe values in your settings.php

Changing the method names seems overkill and likely to break other people's integrations.

If you really want to do that, just add the new methods and call them from the old methods and mark the old ones as deprecated. Maybe we need to do that in any case if you are changing the type hinting.

This should probably have some test coverage added?

This more-or-less copies the code used already in \Drupal\users_jwt\Authentication\Provider\UsersJwtAuth::authenticate()

here's a quick first patch, NR for testing

pwolanin → created an issue.

hestenet → credited pwolanin → .

hestenet → credited pwolanin → .

mcdruid → credited pwolanin → .

Just noticing this also for a name field where I want to sort last, first

I redid it from scratch with phpcbf and hand editing.

I don't care that phpcs complains about the JSON formatting - probably some way to disable that.

need to have a test group

pwolanin → created an issue.

Some of those changes don't make sense like:

+  /**
+   * Namespace Drupal\Tests\ocf_integration\Kernel.
+   */
   protected WebformInterface $webform;

Other cases they introduce a grammar error.

Did you just run a script on this?

Updated patch with test change

Here's a small test-only patch that should fail

Thanks - setting to NR to run the tests, but clearly we are missing a web test for this form

Unfortunately the arguments to create() vs __construct() were no longer in the same order after this change. IDK why the test coverage didn't generate a fail.

doh - it's the return value annotation that's the issue

Oops, that's not the way it works. Looks like this may require a new branch for the breaking change