Under certain circumstances, it is relatively easy to cause cache polution for options_allowed_values's drupal_static.
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().
Add the target bundle to the cache keys.
Tests
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
heddnComment #3
heddnComment #5
stefan.kornI would absolutely support this.
There is another case where missing bundle can hit back.
If you use the allowed_values_function callback and try to distinct options by entity bundle, you might get in trouble when using paragraphs. If you have two (or more) paragraph entities with different bundle but same field, the caching will hit and you will get the allowed values for the first entity on subsequent entities.
This could be fixed by adding the entity bundle to the cache key.
Though patch from #2 seems not valid for this. if condition is wrong and $definition->getTargetBundle() does not exist for FieldStorageDefinitionInterface as far as I can see.
Providing a different patch to solve this.
Comment #7
berdirHm. bundle field definitions are tricky. allowed values is defined as a storage level setting, it's not expected to vary between bundles, you can't do that with configurable fields. And I'm not sure if you can rely on always being called on the bundle specific definition, but these days, it might be I guess.
Comment #8
heddnI ran into this with code defined fields and migrating data. The entity validation would "randomly" start to fail. Root cause we tracked down to the first loaded field definition would break the other bundles. This wasn't an issue for non migration use cases, since we didn't typically load and save multiple entities in the same PHP memory space. But with migrate, it was relatively easy to see happen.
As far as allowed values being storage or not, that was not my decision to hack the fields that way. But except for migrate, it did work and works pretty well.
Comment #9
berdirYeah, I get that, but there are cases where that's not going to work. The function receives a field storage definition, and if you look at e.g. the ListField views filter plugin usage, that won't be able to get the bundle specific definition obviously, so your exposed views filter isn't going to work (I think views list field filters are broken anyway, but that's a detail ;)).
The documentation for the custom callback is also quite clear on "restrict the options or customize labels". The entity storage level needs to return all allowed values, and it may only be allowed to dynamically have a subset of those values for a specific entity.
Comment #11
joachim commented> The entity storage level needs to return all allowed values, and it may only be allowed to dynamically have a subset of those values for a specific entity.
So is the fix that the example code in the IS should be causing an exception to be thrown by the field system?
Comment #12
stefan.kornThe documentation of options_allowed_values says about $entity:
So patch from #5 seems legitimate and afaik necessary to be able to use this for bundles. If that's not true, then at least the documentation is wrong.
Comment #15
smustgrave commentedMoving back for the tests.
Comment #19
vidorado commentedCreated a MR, applied the patch from #5 and added a kernel test.
Comment #21
ironnuts commentedUpdated the test, added comments to clarify each step.
Comment #22
ironnuts commentedAll pipeline tests are green. Except test-only test which fails as it should.
Comment #23
ironnuts commentedAdded comments to the test in the MR.
Comment #24
smustgrave commentedAdded 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 #25
vidorado commentedComment #26
smustgrave commentedSmall comments on MR>
Comment #27
vidorado commented@smustgrave I've replied to your comments.
Thanks for the review!
Comment #28
smustgrave commentedFeedback appears to be addressed.
Thanks!
Comment #29
catch@berdir's points in #7 and #9 still haven't been adequately answered. Should the cache key instead use the entity ID?
Comment #30
heddnFrom re-reviewing the docs on
options_allowed_values, I think we need to cache it on both, yes. Leaving at NW for this.Comment #31
vidorado commentedAbout 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_valuessetting 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 overriddenallowed_valuessetting of['Baz']for the second bundle. I was able to create entities for both bundles, and the values in theoptions_valuesfield 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_valuescallback 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!
Comment #33
smustgrave commentedWill need to rebase but @berdir does #31 answer your questions?
Comment #34
nicxvan commentedWe just refactored this it would be good to see if it fails on main still.
https://git.drupalcode.org/project/drupal/-/blob/main/core/modules/optio...
At the very least we can add the test coverage.
Comment #35
smustgrave commentedThink still could use the new cache tag? Tests did fail
Comment #36
nicxvan commentedI think it makes sense, can you run the test only?
Comment #37
smustgrave commentedUnfortunately no don't have permissions :(
Comment #38
joachim commented> 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.
Now that we have the variation cache, could that be used here, so that we only have caches per-entity ID when needed?
Comment #39
smustgrave commentedMay be worth exploring. Sounds like this one definitely needs more work.