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

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

Recent comments

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

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

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Looks like this just requires changing the 3rd arg to have a bool typehint, which should be back-compatible

πŸ‡ΊπŸ‡ΈUnited States pwolanin

had to remove some trailing whitespace when applying the patch - please check for that next time.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Thanks, looks like you fixed a lot

πŸ‡ΊπŸ‡ΈUnited States pwolanin

We took the PoC patch in #75 and converted into a module since we needed this functionality.

As such, the overall design is still very similar to the core node access system of records and grants extended to custom content entity types. This is what we needed since we need something that will scale to larger number of entities and where we could easily apply the same kind of logic we are using for our custom OG-based node access records and grants.

Contributions are, of course, welcome (especially tests and review)
https://www.drupal.org/project/raft_entity_access β†’

πŸ‡ΊπŸ‡ΈUnited States pwolanin

pwolanin β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Looks like this bugs happens when using the JWT library >= 6.6.0

A 3rd param was removed before, than added back differently for the decode() call.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Need to dig in, but might need a tweak to make sure this works right:
\Drupal\views\Plugin\views\PluginBase::doFilterByDefinedOptions()

called from:
\Drupal\views\Entity\View::duplicateDisplayAsType()

πŸ‡ΊπŸ‡ΈUnited States pwolanin

It's looking for invalid cache tags in that SQL, it's not updating or deleting any.

A problem might be that it's checking for a path alias - consider turning off path aliases if you don't need them?

Drupal core doesn't seem to have a facility for indicating some path will not be aliased, or an alias should be ignored. You could subclass \Drupal\path_alias\AliasManager and replace the alias manager service to achieve that and/or propose a core patch to add the capability.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

pwolanin β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Thanks for providing a patch. Could you please update the issue summary to actually describe the problem you are seeing?

If there is a bug, we should add some regression test coverage to the patch

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Did you look at the README? it's a little sparse but has some instructions
https://git.drupalcode.org/project/jwt/-/blob/2.x/README.md

if that's not sufficient, please propose changes there

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Conceptually seems fine - maybe just hard-coded that way to 1 because it was in prior versions, or due to lack of a use case.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Ok, here's a variant on what effulgentsia first tried. I ran this in a local docker setup and checked the process list and can see that we have a orphan process that has a PPID of 1:

UID        PID  PPID  C STIME TTY          TIME CMD
...
www-data   110    84  0 20:21 ?        00:00:00 php-fpm: pool www
root       116     0  0 20:22 pts/0    00:00:00 /bin/bash
www-data   130    86  0 20:24 ?        00:00:00 nginx: worker process
www-data   131    86  0 20:24 ?        00:00:00 nginx: worker process
www-data   213     1  1 20:39 ?        00:00:00 /usr/bin/php test2.php

The output from the webserver:

PHP path /usr/bin/php 2023-06-07T20:39:03+00:00 done sleeping, exiting

The content of the temp file:

2023-06-07T20:39:13+00:00

web/test.php:

use Symfony\Component\Process\Process;
use Symfony\Component\Process\PhpExecutableFinder;

$autoloader = require_once 'autoload.php';

$phpBinaryFinder = new PhpExecutableFinder();
$phpBinaryPath = $phpBinaryFinder->find();

$process = Process::fromShellCommandline($phpBinaryPath . ' test2.php &');
$process->setWorkingDirectory(__DIR__);
$process->disableOutput();
$process->setTimeout(0);
$process->start();

sleep(1);
print"PHP path $phpBinaryPath\n ";
print gmdate('c') . "\n";
print 'done sleeping, exiting';

web/test2.php:


sleep(10);
file_put_contents("/tmp/file.txt", gmdate('c') . "\n");
πŸ‡ΊπŸ‡ΈUnited States pwolanin

Talking about this at Drupalcon, going to work on implementing this. I don't think we need to use the extra "sh" but but in any case I don't think this is going to work on windows, so those sites would need to run cron or the dedicated console command via the command line on a regular basis. We validated this restriction as acceptable with xjm in terms of the impact on future core releases.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

@anypost - this important patch has been waiting for 7+ years to get committed. we should not expand the scope. If you want to e.g. be able to use BC math, please open a follow-up issue.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Group module could also check for implementations of hook_node_grants() to decide whether to return forbidden or neutral? Trying to be 100% entity-type agnostic seems to be a nice goal but ignores reality.

For sure we need some better documentation and maybe a parallel access system for other entity types.

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Looks good, thanks - in a follow-up we should add a test, but I want to get the 1.0 out

πŸ‡ΊπŸ‡ΈUnited States pwolanin

Before enabling this, I think there needs to be a way to leave update off during the normal Drupal cron and include drush and drupal console commands to just run the updates (and for the core patch include a stand-alone script) + documentation on how to run these in a more secure mode where a system user other than the webserver user can write to the code and the command is executed by that user on an externally driven time (e.g. *nix crontab).

πŸ‡ΊπŸ‡ΈUnited States pwolanin

making a few additions to the README if you want to make further suggestions:
πŸ“Œ Update README Fixed

Production build https://api.contrib.social 0.61.6-2-g546bc20