πŸ‡ΊπŸ‡ΈUnited States @pwolanin

Account created on 17 February 2006, over 18 years ago
#

Merge Requests

Recent comments

πŸ‡ΊπŸ‡Έ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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

The error seems to happen in

\Drupal\locale\LocaleConfigManager::getTranslatableData()

which is a recursive method

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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
πŸ‡ΊπŸ‡ΈUnited States pwolanin

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
πŸ‡ΊπŸ‡ΈUnited States pwolanin

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?

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

The test module should probably not depend on itself

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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?

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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?

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Updated patch with test change

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

Production build 0.69.0 2024