Getting OOM while enabling other modules after updating to 3.0

@pwolanin:

You have not given enough information to reproduce the problem, so I am setting the status to Postponed (MNMI). What other modules are involved?

It is hard to see how this module could cause problems during the installation phase. It does not have a .install file, and it does not define any entities, so I cannot think of anything that happens at install time. Config import is a different matter.

Perhaps some other module, or Drush, got updated when you updated this module to 3.0.0 from 2.0.1. Can you use composer-lock-diff to get a complete list of modified packages?

Do you see this problem in local copies of your site or only in your CI environment? If it is only in CI, then maybe your jobs are getting confused by the existence of .gitlab-ci.yml.

That last suggestion was a stretch. I will quit now and wait for some more information.

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

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

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

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.

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.

The error seems to happen in

\Drupal\locale\LocaleConfigManager::getTranslatableData()

which is a recursive method

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

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

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

The config schema is mentioned in this comment: #3338083-3: Create functional test for basic view functionality. → .

I would still like to have steps to reproduce. From #7:

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.

Is that all it takes? Can you share the config for that view?

Is your site multilingual?

I've been looking at rest_views_modules_installed for a bit, it's used to automatically install required submodules when the contributed modules are there.

Also can you confirm if you have the following contributed modules and if they are installed or not?

- geofield
- entity_reference_revisions (commonly used by paragraphs)
- search_api

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)

From Comment #11:

If I remove those lines from the config schema at the tip of 3.0.x branch I resolve the seg fault locally / OOM in ci

That cannot be the right fix. I tried removing those lines and ran the PHPUnit tests:

There was 1 error:

1) Drupal\Tests\rest_views\Functional\RestViewsTest::testRestView
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for views.view.rest_views_test with the following errors: views.view.rest_views_test:display.default.display_options.fields.nid_export.type missing schema, views.view.rest_views_test:display.default.display_options.fields.uid_export.type missing schema

While I was running tests, I also noticed some deprecation notices, so I added 📌 Normalizers/Denormalizers should implement ::getSupportedTypes() instead of ::hasCacheableSupportsMethod() Needs work .

From Comment #14:

I've been looking at rest_views_modules_installed for a bit, it's used to automatically install required submodules when the contributed modules are there.

I have been looking at it, too. It is problematic.

The idea is to enable any of the three submodules if one of its dependencies is being enabled, and the rest of its dependencies are already installed (or are being installed). IMO this is a bad idea. Calling $moduleInstaller->install() from an implementation of hook_modules_installed() is just asking for trouble (like infinite recursion).

But that function does not cause any trouble for me, even when I try to reproduce the bug described in this issue. (I installed the Umami profile, since it is multilingual. Then I installed rest_views, added a view that uses it, and installed entity_reference_revisions.) This is why:

      foreach ($info['dependencies'] as $dependency) {
        $dependency = explode(' ', $dependency)[0];

Those lines have not been updated since c9a7ae1 (no issue on d.o), which replaced "views (>=8.7)" (note the space) with "drupal:views" (among other changes).

Currently, that function does not do anything.

@pwolanin, if you have any patches you are applying that you neglected to mention, then you owe us a face-palm emoji. Can you test whether removing the whole function fixes your problem? Or is that more or less what you did in Comment #5?

@nicxvan, what do you want to do about this problematic function?

1. Get rid of it.
2. Convert it to showing a message instead of auto-installing the submodules.
3. Fix it so that it auto-installs the submodules.

Since the function is currently broken, (1) is not such a bad idea. I guess that (2) is more friendly.

If you do want to fix it (Option 2 or 3), then there may be some other problems. It would be nice (Option 2) or a really good idea (Option 3) to move the code into a service class so that we can inject the module services, mock them, and test the code.

I will have a look at the locale module, following up on Comments 8-10.

Looking at this further, this is likely a solution to allow contrib field types to work automatically.

Otherwise if you enabled Geofield and had those fields in the view they would not show up in the export.

I think this was done to not require the site maintainers to need to manually install submodules.
On the other side you can't just have that code in the main module because you can hit WSOD if you depend on classes that do not exist.

I think it's kind of clever, but it introduces several issues already illuminated.

I think we could solve the issue in a similar way I did in eca_commerce. I needed a way to know if a particular submodule of commerce was available before trying to load tokens otherwise the site would WSOD. I also did not want to do all of the negotiation of submodules.

The solution was to check for classes:
https://git.drupalcode.org/project/eca_commerce/-/blob/1.0.x/src/EventSu...

This solution introduces it's own problems because it involves uninstalling and replacing submodules automatically.
We'd have to be sure to avoid causing issues like what happened with purger: https://www.drupal.org/project/purge/issues/3397227 🐛 Update to 3.5 replaces purge_drush but does not disable it Needs work . It also requires multiple updates to first uninstall the sub modules and replace.

The other option is to just remove the hook and document that users have to enable the other submodules manually.

The other option is to just remove the hook and document that users have to enable the other submodules manually.

That is what I was thinking.

Even when the submodule is enabled automatically, there is still another step (I think): the user has to add a REST export field to the view before the field shows up in the export, right?

Also, removing the hook would be following the principle of least surprise. We do not expect modules to be enabled without being selected. Are the submodules even listed in the confirmation page if you enable modules in the admin UI?

Maybe some sites enable those modules but do not need to export those fields. If we force them to enable the submodule, then we are adding unneeded code to their sites.

We would need to know what happens if you add those fields to a view without those modules enabled.
Do they just not show up, does the view WSOD, is there a way to put a message that there is a submodule?

I'm looking to tackle this again. I think what I will do is the following.

1. Move the sub module functionality to the main module and wrap them in a check for the classes so they will automatically become available.
2. Make the current submodules stubs
3. Auto uninstall them on an install hook
4. Drop them in a 4.0 if we ever move to it

Does this seem reasonable?

Actually removing the submodules is more complex than I originally thought.

I still am not a fan of the auto install bit, I think though it might make sense to post a banner somewhere if modules such as geolocation are installed that mention you should install the submodule for functionality.

For now I am going to update the readme and remove the rest_views_modules_installed hook so you can test.

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.

Adding some detail to the Issue summary from slack.

Do you have any modules aliasing the config:

views.field.field_export:
type: views.field.field
label: 'Entity field (serializable)'

Added here 📌 Create functional test for basic view functionality. Fixed

I notice that this config is the only config missing a mapping. Does this just need a mapping?

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-

Honestly I'm not sure, I was moving some schema finalizing this ✨ Ability to configure absolute/relative urls Fixed and noticed the difference.

I didn't see anything in the code necessitating mapping but thought I'd comment before moving.

I might take another look later, I think it's missing something since the docs mention translation as one of the primary purposes and that is where the error is happening.

https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... →