Contrib.social

options_allowed_values() cache pollution

Open on Drupal.org →
Created on 24 July 2020, almost 5 years ago
Updated 23 January 2025, 3 months ago

Problem/Motivation

Under certain circumstances, it is relatively easy to cause cache polution for options_allowed_values's drupal_static.

Steps to reproduce

In custom entity:

  public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    $fields = parent::baseFieldDefinitions($entity_type);

    $fields['options_list'] = BaseFieldDefinition::create('list_string')
      ->setLabel(t('Options'))
      ->setSettings([
        'allowed_values' => ['foo' => t('Foo'), 'bar' => t('Bar')],
      ]);

  }

  public static function bundleFieldDefinitions(EntityTypeInterface $entity_type, $bundle, array $base_field_definitions) {
    if ($bundle === 'baz') {
      $fields['options_list'] = clone $base_field_definitions['options_list'];
      $fields['options_list']->setSettings([
        'allowed_values' => ['baz' => t('Baz')],
      ]);
      return $fields;
    }

    return [];
  }

Then bulk insert a bunch of entities where some have a bundle of baz and the rest are foobar. Once the foobar bundled entities are validated/saved, any attempts to perform entity validation on baz bundle entities will fail because they are passing an option value of 'baz' and the previous list of ['foo', 'bar'] was already statically cached by drupal_static().

Proposed resolution

Add the target bundle to the cache keys.

Remaining tasks

Tests

User interface changes

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component

options.module

Created by

heddn Nicaragua

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • Merge request !10990Issue #3161345: Options allowed values cache pollution → (Open) created by vidorado
  • Pipeline finished with Failed
    3 months ago
    Total: 898s
    #404244
  • Pipeline finished with Failed
    3 months ago
    Total: 571s
    #404258
  • Pipeline finished with Success
    3 months ago
    Total: 812s
    #404263
  • Comment 3 months ago →
    🇪🇸Spain vidorado Logroño (La Rioja)

    Created a MR, applied the patch from #5 and added a kernel test.

  • First commit to issue fork.
  • Comment 3 months ago →
    🇬🇧United Kingdom oily Greater London

    Updated the test, added comments to clarify each step.

  • Pipeline finished with Success
    3 months ago
    Total: 1502s
    #404310
  • Comment 3 months ago →
    🇬🇧United Kingdom oily Greater London

    All pipeline tests are green. Except test-only test which fails as it should.

  • Comment 3 months ago →
    🇬🇧United Kingdom oily Greater London

    Added comments to the test in the MR.

  • Pipeline finished with Success
    3 months ago
    Total: 895s
    #404334
  • Pipeline finished with Success
    3 months ago
    Total: 716s
    #404621
  • Comment 3 months ago →
    🇺🇸United States smustgrave

    Added some additional comments after doing some code standard reviews. I didn't resolve any of the open threads but if we can do that next please.

  • Comment 3 months ago →
    🇪🇸Spain vidorado Logroño (La Rioja)
  • Comment 2 months ago →
    🇺🇸United States smustgrave

    Small comments on MR>

  • Comment 2 months ago →
    🇪🇸Spain vidorado Logroño (La Rioja)

    @smustgrave I've replied to your comments.

    Thanks for the review!

  • Pipeline finished with Canceled
    2 months ago
    Total: 71s
    #421800
  • Pipeline finished with Success
    2 months ago
    Total: 369s
    #421802
  • Comment 2 months ago →
    🇺🇸United States smustgrave

    Feedback appears to be addressed.

    Thanks!

  • Comment about 2 months ago →
    🇬🇧United Kingdom catch

    @berdir's points in #7 and #9 still haven't been adequately answered. Should the cache key instead use the entity ID?

  • Comment about 2 months ago →
    heddn Nicaragua

    From re-reviewing the docs on options_allowed_values, I think we need to cache it on both, yes. Leaving at NW for this.

  • Comment about 2 months ago →
    🇪🇸Spain vidorado Logroño (La Rioja)

    About Berdir's comments above

    I believe they have been taken into account. We now understand that we can only add a callback function to a bundle definition to restrict the options or customize the labels, but the allowed_values setting must not be overridden.

    Regarding this, I think the only unanswered comment is #11, which asks what would happen if we did override it.

    I tested this scenario, and it turns out that it is possible. I created a custom test entity with an options_field that has a default allowed_values setting of ['Foo', 'Bar'], two bundles, and an overridden allowed_values setting of ['Baz'] for the second bundle. I was able to create entities for both bundles, and the values in the options_values field dropdown were displayed as configured, with no errors thrown.

    I'm not sure what action, if any, we should take regarding this.

    About adding the entity ID to the cache ID

    This is a separate question, and I assume you're suggesting adding the entity ID to the cache key because the allowed_values callback function receives the entity as a parameter, meaning it could depend on any part of the entity, not just the bundle.

    In that case, I agree that we should add the entity ID to the cache key. It's unfortunate, as this will result in a large number of cache entries—one per entity—but I don't see another way of handling it since the callback function does not return any cacheability metadata.

    Please confirm that we're all on the same page, regarding to this, and I'll proceed with adding the entity ID to the cache key.

    Thanks!

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024