API page: https://api.drupal.org/api/drupal/core%21modules%21options%21options.mod...
As @dawehner_ pointed out that this cache_id is not good:)
$cache_id = implode(':', array($entity->entityType(), $entity->bundle(), $field_definition->getFieldName()));
This cache_id doesn't involve with entity level, only entity type, bundle and field name. But in fact, this function's result could depend on parameter EntityInterface $entity potentially. See
$function = $field_definition->getFieldSetting('allowed_values_function');
// If $cacheable is FALSE, then the allowed values are not statically
// cached. See options_test_dynamic_values_callback() for an example of
// generating dynamic and uncached values.
$cacheable = TRUE;
if (!empty($function)) {
$values = $function($field_definition, $entity, $cacheable);
}
It needs entity to return value, but the cache_id doesn't count the entity identifier, so the dynamic feature for each entity wouldn't fly.
While there's an issue #2012130: Regression: Views integration for "list" field types is broken about whether we need this feature, we need to adjust the cache_id to make this feature work correctly. It depends on field_definition, if this field makes use of
$function = $field_definition->getFieldSetting('allowed_values_function');
we need to count the entity identifier, e.g., 'Uuid of this entity' into cache_id. Otherwise, it's ok to use current cache_id.
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | interdiff.txt | 1.02 KB | jlbellido |
| #18 | dynamic_options-2037217-18-fix.patch | 3.88 KB | jlbellido |
| #16 | dynamic_options-2037217-16-fix.patch | 3.65 KB | jlbellido |
| #13 | dynamic_options-2037217-12.patch | 3.65 KB | drabik |
| #8 | dynamic_options-2037217-8-fix.patch | 3.68 KB | smiletrl |
Comments
Comment #1
smiletrl commentedTo better explain the cache flaw, here's an example.
An option field 'test_options' makes use of this dynamic options feature, it's definition could look like
Code comes from here
Let's assume there're two nodes. Both have node bundle 'article', and they have this field attached.
So, node_1 and node_2 will have different allowed options for this field. But see cache_id here, it won't count the second node_2, because cache_id is the same for node_1 and node_2. Consequently, this dynamic options feature won't fly.
Comment #2
smiletrl commentedAdd a pacth.
Comment #5
smiletrl commentedWierd,
$entity->uuidwill fail?Comment #6
smiletrl commentedPatch at #5 should fail.
This values should depend on the $field_definition, i.e., the field name too. New patch attached.
Need tests to cover options values and the dynamic values flaw.
Comment #7
smiletrl commentedThis test patch is to show the dynamic options cache_id flaw.
Comment #8
smiletrl commentedwhoops, extra space.
Comment #9
smiletrl commentedRemove Needs test tag.
Comment #11
smiletrl commented#8: dynamic_options-2037217-8-fix.patch queued for re-testing.
Comment #12
andypostAfter #2169983: Move type-specific logic from ListItemBase to the appropriate subclass
Comment #13
drabik commentedrerolled
Comment #15
drabik commentedComment #16
jlbellidoRerolled #8 again.
Comment #18
jlbellidoChanged
$this->entity_1->uri();To
$this->entity_1->url();Tests that was failing pass now locally. Lets try again.
Comment #19
sweetchuckThe patch #18 is applicable to the latest 8.x 0a8e34cf15f237c0672dd6ea7776d46393467ce1 Sat Mar 29 12:40:21 2014 +0100
Comment #20
andypostLooks good, except:
any reason to use uuid() here, suppose a ID is enough.
Comment #21
yched commentedThere are places that need a list of allowed values without having a specific $entity available.
Example : building the options for a dropdown select form element for a Views filter - in this context we are not restricted to a specific bundle, so we can't even build a "fake" entity.
- The current options_allowed_values() function is already flawed because it assumes an $entity param will be present.
Long time since I looked at the issue, but that's probably what #2012130: Regression: Views integration for "list" field types is broken is about.
- The current code already has a mechanism to account for "how do we statically cache if the result of the allowed_values_function depends on the $entity" : it's the $cacheable boolean variable. If an $entity is present, and the allowed_values_function returns a result that depends on it, then it is its responsibility to set $cacheable to FALSE by ref before returning the result, so that options_allowed_values() doesn't cache it. I know it's only a static cache, but it can still have nasty effects on long-running drush scripts.
- Adding the $entity->uuid (if present) to the cache id is of no use. It does not ensure the cached results are correct, since the $entity values can change: its uuid will always be the same, but the entity is different, so the allowed_values_function results can potentially be different too, and the cached data is stale.
I'd tend to won't fix this, and stay with the current behavior : if an allowed_values_function returns a result that actually depends on the $entity, it needs to tell the caller not to cache the results.
Also, given the above, I don't think this qualifies as a bug ?
Comment #22
jhedstromReroll plus a bit of work given #21.
Comment #23
yched commentedI still stand by #21 - plus, given the work happening in #2238085: [regression] options_allowed_values() signature doesn't allow for Views filter configuration, I do think this is a won't fix.