Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When adding a facet based on an aggregated field the option "Transform entity id into label" doen't work properly.
The processor "Transform entity ID to label" (translate_entity) works as designed, to finish this issue we should:
Add a note in the description of the processor ("Transform entity ID to label" (translate_entity)) that it doesn't support aggregated fieldsThis was committed in #23.- Add a new processor of transforming the entity ID into label in aggregated fields.
Comment | File | Size | Author |
---|---|---|---|
#53 | 2844895-52.patch | 1.72 KB | phenaproxima |
#45 | interdiff_38-45.txt | 1.49 KB | Jax |
#45 | transform_entity_id-2844895-45.patch | 9.33 KB | Jax |
#21 | transform_entity_id-2844895-21.patch | 837 bytes | borisson_ |
#33 | transform_entity_id-2844895-33.patch | 8.48 KB | tamarpe |
Comments
Comment #2
Jeroen_005 CreditAttribution: Jeroen_005 at Pàu for District09 commentedAdded a possible approach in this patch file.
Comment #3
Jeroen_005 CreditAttribution: Jeroen_005 at Pàu for District09 commentedComment #4
Jeroen_005 CreditAttribution: Jeroen_005 at Pàu for District09 commentedComment #5
Jeroen_005 CreditAttribution: Jeroen_005 at Pàu for District09 commentedFixed some patch issues.
Comment #6
Jeroen_005 CreditAttribution: Jeroen_005 at Pàu for District09 commentedAdded field state to facet_setting.
Comment #8
borisson_This patch no longer applies and needs a reroll
Comment #9
Jeroen_005 CreditAttribution: Jeroen_005 at Pàu for District09 commentedRerolled patch.
Comment #10
Jeroen_005 CreditAttribution: Jeroen_005 at Pàu for District09 commentedRerolled patch.
Comment #12
Jeroen_005 CreditAttribution: Jeroen_005 at Pàu for District09 commentedLooks like there is an dependency on issue #2777217.
Comment #14
borisson_Ok, let's postpone this one.
Comment #15
borisson_The issue this depended on is committed.
Comment #17
borisson_Comment #18
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #20
borisson_We discussed this in today's hangout and decided that the current approach makes this way too complicated.
Instead, we'd like to keep this processor as-is and add documentation in the UI that a new custom processor should be created to do this.
Comment #21
borisson_Attached is what #20 suggested.
Comment #22
borisson_NR for #21
Comment #24
borisson_Comment #25
joachim CreditAttribution: joachim commented> We discussed this in today's hangout and decided that the current approach makes this way too complicated.
> Instead, we'd like to keep this processor as-is and add documentation in the UI that a new custom processor should be created to do this.
Are you really sure this is the way to go?
Doing this means duplicating code in a new processor that handles entity labels on aggregated fields.
It also means the the complication that is being saved in code is moved onto to the site builder, because there will be two 'Transform entity ID to label' processors, and the site builder will need to choose one and know what sort of Search API search index they are using.
Furthermore, I am guessing that the 'List item label' processor will be affected in the same way -- that too might need special treatment to work with aggregated fields.
Comment #26
joachim CreditAttribution: joachim commentedAlso, reopening because if this *is* the right way to go, there needs to be form validation to prevent this being added to aggregated fields -- views with this processor on an aggregated field crash with:
> Call to undefined method Drupal\search_api\Plugin\search_api\processor\Property\AggregatedFieldProperty::getPropertyDefinition
Comment #27
joachim CreditAttribution: joachim commentedAlso, I think patch #18 can be improved, with something like this
That's not far off from being able to figure out the target entity type of an aggregated field.
Rather than load fields on every page view, this could be figured out in the facet form, and saved into the processor configuration?
Comment #28
borisson_Doing this will introduce a big difference in the processor between core search and search api. Aggregated fields are only working for search api so that is somewhat expected. However this might make things very complex.
However, I think that implementing #2851851: Facet source plugins should provide metadata for facet fields would be a good start to simplify the code to decide what field it is, see also the code already added to TranslateEntityProcessor in that patch.
Comment #29
joachim CreditAttribution: joachim commented(BTW, for people who are finding this breaks their facets from older versions, a quick and dirty workaround is to copy the taxonomy term label processor plugin from an old alpha (3 in my case) into a custom module.)
Comment #30
mkalkbrennerI ran into this issue today. It leads to a fatal error:
The aggregated_field_type field has no datasource, there is no valid use for the list_item processor with this facet in ...
The aggregated field contains the types (bundles) of two different content entities. If I remove the "List Item label" option, it works.
Comment #31
mpp CreditAttribution: mpp at AmeXio for District09 commentedI ran into this issue today. Not on an aggregated field but on a custom field I created that contains a bundle value.
ListItemProcessor
contains fallback code to handle the bundle label but it doesn't reach that part of the code since my custom field doesn't have a datasource.Comment #32
tamarpe CreditAttribution: tamarpe commentedI ran it today as well with aggregated fields that contain a bundle value.
Would be a nice feature to add- a processor of transforming aggregated field entity IDs to labels
Comment #33
tamarpe CreditAttribution: tamarpe commentedEnded up with a new processor to aggregated fields
Comment #34
tamarpe CreditAttribution: tamarpe commentedComment #35
Aron NovakJust tested the patch in #33, works well not only for taxonomy terms, but for custom entities too.
Two minor code review comments:
1.
"An array of values that contain possible replacements for the orignal"
Should be:
"An array of values that contain possible replacements for the original"
2.
The processor should hide itself from the list already if not an aggregated field, so
Could be removed and added as a pre-build check, not?
Comment #36
tamarpe CreditAttribution: tamarpe commentedGood point, I fixed them both
Comment #37
tamarpe CreditAttribution: tamarpe commentedComment #39
tamarpe CreditAttribution: tamarpe commentedFix tests
Comment #40
tamarpe CreditAttribution: tamarpe commentedComment #41
Jax CreditAttribution: Jax at Dazzle commentedOk, works.
Comment #42
Jax CreditAttribution: Jax at Dazzle commentedI rescind the RTBC. Will fix a couple of small things:
Comment #43
Jax CreditAttribution: Jax at Dazzle commentedPatch with very small changes.
This was changed because computed fields don't have storage.
Comment #44
Jax CreditAttribution: Jax at Dazzle commentedI actually used the wrong previous patch. I used the one from #36 instead of #38 so my patch is worthless...
Comment #45
Jax CreditAttribution: Jax at Dazzle commentedCorrect versions of the files.
Comment #46
niles38 CreditAttribution: niles38 commentedHey @Jax. Your patch in #45 is good! Just tested it.
Thanks so much! You saved me a lot of time :)
Comment #47
lamp5Works well also with entity type bundles!
Comment #48
borisson_Looks great, we don't have specific coverage for this, but that is going to be a lot of specific setup. I like that the supportsFacet is very specific here. +1
Comment #49
Jax CreditAttribution: Jax at Dazzle commentedshould be converted to
Comment #51
Nick_vhCommitted and fixed
Comment #52
phenaproximaI think this line is a problem. It assumes that the facet source is a Search API source, which is not always true (see the core_views_facets module, for example). We should probably return early if the facet source is not Search API. Patch incoming.
Comment #53
phenaproximaHere's a patch for this.
Comment #54
borisson_Looks like a simple change, LGTM.
Comment #56
borisson_Pushed the followup.