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.
Problem/Motivation
The views EntityField field plugin used for displaying data from entity fields does not support computed fields. Adding support for this is really easy and would be useful in core and beyond, so lets do it.
Proposed resolution
Update the EntityField plugin to support computed fields.
Remaining tasks
Review.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#63 | interdiff-63.txt | 750 bytes | amateescu |
#63 | 2852067-63.patch | 13.32 KB | amateescu |
#58 | interdiff.txt | 1.76 KB | amateescu |
#58 | 2852067-58.patch | 13.32 KB | amateescu |
#58 | 2852067-58-test-only.patch | 11.11 KB | amateescu |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedHere is the proof of concept used from the issue linked in the follow up. Needs stacks of work, but seemed to validate this was a possibility.
Comment #3
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedBefore going too much deeper on the implementation front, here is an approach to testing this which should verify the existing basic handler is working.
Comment #5
dawehnerI would have guessed that all you need is to have a formatter which outputs the computed property and be done with it. Sadly I don't get why we need an additional handler here.
Comment #6
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedYou can create a computed field of any type, so a new formatter isn't what is required. The issue is that the EntityField handler makes assumptions about the field types it can display, making it impossible to use with computed fields.
The only way to achieve it right now is to create a new views handler for each type of computed field. node_path is an example of this in core.
Comment #7
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI'm not actually sure this is going to be possible in a generic way for ALL fields. I've found that:
So the two options are: make this something that works for computed base fields only or ditch this in favour of simple field plugins for the different computed fields that exist.
Personally I think the former could still be useful, it reduces the amount of work required when using a computed field with views for most cases.
Comment #8
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI suspect this will fix the tests.
I think it still makes sense to proceed with this, but would be good to get some outside input.
Comment #9
dawehnerDo you mind elaborating this? The field handler though also has a lot of performance optimizations etc. so it might be worth fixing the handler instead of creating a new one.
Comment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI'll write up a summary of what breaks in the EntityField handler. I went down the path of fixing it a few times, then backed out each time because it didn't seem feasible.
Comment #11
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedMy issue I faced in the past was that computed fields didn't have a storage definition, so everything which relied on that broke. As it turns out BaseFieldDefintions also implement EntityStorageDefintionInterface, so it can fit comfortably in the place of normal field storage for the purposes of the handler. The result is: we can support computed fields with a minor change to \Drupal\views\Plugin\views\field\EntityField. Awesome!
Attached are these changes.
Comment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #15
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThe view needed a resave after the handler was updated.
Comment #16
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedTest only patch too.
Comment #18
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #19
jibranI don't think this a feature request. I think this is something which we missed while writing
EntityField
plugin. It is a task if not a bug imo. Overall patch look good to me with complete test coverage. Just some document improvements needed imo.Can we please explain all these conditions?
Can you please expand comment by adding the examples for a computed field? Something like "e.g. path field". I can understand the fix because there is a test but it is difficult to understand when you are only reading the code.
EOF missing.
Space missing between namespace and use statement.
Comment #20
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks for the review, will look at these.
Comment #21
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFeedback addressed. Re: point 1, the linked @todo followup actually explains why those are there. Legacy reasons only, should be removed in the next release.
Comment #22
jibranThanks!
Comment #23
dawehnerIt'd be nice to have some quick helpful comment here :)
Here is a question: Is there a reason views could not expose those fields automatically? All the information should be available to expose those fields, I guess?
Comment #24
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI think exposing it automatically is probably follow-up territory, but I think it would be possible. Maybe a BC break for people who expect computed fields not to be exposed by views?
Added a better comment to the entity class.
Comment #25
dawehnerWhat exactly is the BC break there? What wasn't there can't break. Maybe I'm missing something obvious.
Comment #26
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedYeah, I can only come up with contrived examples. People with custom entity types, with computed fields which should never be exposed in a UI? If you set displayConfigurable => false, it would still show up in the fields list. Perhaps the expectation is that all fields would exist in the views fields list, but that hasn't been the case.
Comment #27
jibran> Is there a reason views could not expose those fields automatically?
I don't think we should do that. The 'entity' is a computed property in ER field and it doesn't make sense to add entity property automatically because we can render traget_id as an entity. We have also seen with path field that there is a blurry line between computed and custom storage fields.
Maybe, a good option would be to make it configurable by adding it to basefield definition which definitely comes in a follow-up territory.
Also, we still have to figure out argument, filter, expose filter, and sorting for computed fields.
I think we should add a change notice for this so that people will now about this.
Patch still looks good to me so back to RTBC.
Comment #28
jibranAdd good feature would be to add the views support for extra fields i.e https://www.drupal.org/project/extrafield_views_integration.
Comment #30
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #31
tstoecklerSince
getBaseFieldDefinitions()
returnsFieldDefinitionInterface[]
and$field_storage
presumably implementsFieldStorageDefinitionInterface
we should instead do$field_storage = $base_fields[...]->getFieldStorageDefinition()
Comment #32
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedGood catch. BaseFieldDefinition implements FieldStorageDefinitionInterface directly, but I think this makes more sense.
Comment #33
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented@tstoeckler, any chance of another review?
Comment #34
tstoecklerSorry, missed this one. Looks good to me now, thanks!
Comment #35
seanBIt seems the same problem can occur in
\Drupal\views\FieldAPIHandlerTrait
forgetFieldStorageDefinition()
. I ran in to this issue using a computed entity reference field in a Search API view.It seems related to this issue. Since
\Drupal\views\Plugin\views\field\EntityField
uses theFieldAPIHandlerTrait
, can't we just move the changes togetFieldStorageDefinition()
to the trait and remove the method fromEntityField
?Comment #36
seanBAdded change mentioned in #35.
Comment #37
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI don't think we should do that. The implementation in EntityField contains a backwards compatibility fix that accepts both "entity field" and "field_name". We should keep that BC as narrowly scoped as possible to prevent the inconsistency from propagating to other classes.
Comment #38
seanBRemoved the BC fix in
getFieldStorageDefinition()
from\Drupal\views\FieldAPIHandlerTrait
.Comment #39
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedSo, this does make sense, but I wonder about the scope. It's an untested part of the patch now.
Comment #40
jibranComment #41
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI'm going to go back to RTBC with the caveat that there is an option between #32 and #38. I think it makes sense for #38 to be a follow up, but happy for either to land.
Comment #42
larowlanlets return in all of these and avoid the three elseifs - aiding readability
Comment #43
dawehnerI'm wondering whether its worth to rewrite the code to not fetch the basefields, unless needed, by moving it into the
if
statement.Comment #44
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedNW for those two points.
Comment #45
seanBAddressed #42 and #43. Since we now use separate
if
statements, the code to fetch the base fields is now only executed when we need it. It is not in the actualif
statement since this would make theif
statement really long and harder to read.Comment #46
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI still think the changes in FieldAPIHandlerTrait are out of scope and should be added in another issue.
Comment #47
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented@seanB, thanks for working on this, but if this issue has a chance, it has to have a clear scope. I've removed the changes to the trait and the changes introduced to EntityField which don't make sense to me (not sure where "$this->fieldStorageDefinition" from). If they don't fail the tests, they are out of scope.
Please do open a follow up to describes what the changes to trait actually does and why.
Comment #48
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #49
seanB\Drupal\views\Plugin\views\field\EntityField
overridesgetFieldStorageDefinition()
defined in\Drupal\views\FieldAPIHandlerTrait
. The issue is thatgetFieldStorageDefinition()
doesn't support computed fields. Solving this only for\Drupal\views\Plugin\views\field\EntityField
feels like a partial solution, since there are a bunch of other views plugins that use the trait.I've noticed the issue in
\Drupal\views\Plugin\views\filter\EntityReference
, but I guess the other plugins using\Drupal\views\FieldAPIHandlerTrait
probably have the same issue (this is an assumption which I haven't tested yet!).Still prefer to solve this in a generic way, but if the only way to get this committed is to create an issue for each plugin that has the issue, then I guess that is what we need to do.
Comment #50
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI think if you can put forward a case for the change to the trait in a follow-up as well as a test to prove why it's necessary, then no reason not to change it. Lets just not put forward any false pretense that it's required to fix this issue or is covered by the tests this issue introduces.
Please read Issue scope guidelines for Drupal core issues
Comment #52
rjacobs CreditAttribution: rjacobs commentedAdding link to related issue #2392845: Add a trait to standardize handling of computed item lists. Issues with views support and computed fields are referenced multiple times in that issue, but punted for follow-up... so this issue might serve, at least partially, as that follow-up.
Comment #53
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedBoth issues seem to introduce a computed field for the purposes of testing. I think if that one gets in first, this will need to be rerolled against it, to make use of the same testing classes.
Comment #54
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAnyone want to pick this up again for review? The actual chunk of code introduced is very small. The rest of the patch is centered around testing, which has no api impact and can be updated if computed fields undergo api improvements.
Comment #55
jibranPatch looks good to me. Do you think we need a change notice? Just a nit.
Entity manger can be a local variable now.
Comment #56
timmillwoodThis is a dependency for #2902187: Provide a way for users to moderate content.
Comment #57
tim.plunkettWhy the change?
These look reasonable, but it seems like the old code should have been causing notices for undefined indexes in some cases. Is there coverage for that too?
I really don't think moderation_state needs to get a shoutout here. Also, the lack of oxford comma combined with the parenthetical comment makes the whole sentence unclear.
For the next patch, can someone also post an expected fail patch (all the test changes, without the actual fix)? Thanks!
This should be easily re-RTBC-able, I will keep an eye on it.
Comment #58
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRe #57:
Here's an updated patch with a test-only one as well.
Comment #60
tim.plunkettGreat, thanks for the changes! And the clear red/green patch, what a sight :)
Comment #61
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedChanges look great, thanks everyone.
Comment #62
larowlanthis should be $this->t() (thanks
vimeo/psalm
for finding that one)leaving at rtbc
Comment #63
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedHere we go.
Comment #64
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedLGTM +1 RTBC
Comment #65
Wim LeersDoes this need a change record? And a release notes mention? This seems a pretty significant improvement?
Comment #66
timmillwoodYes, I think this is worth a release notes mention, and here's a Change Notice https://www.drupal.org/node/2904410 (feel free to update @Sam152, @seanB or @amateescu).
Comment #67
larowlanUpdating credit to add reviewers that shaped final patch
Comment #69
larowlanCommitted 1fa12fb and pushed to 8.5.x.
Have not published the change record yet - as a decision needs to be made with regards to backport.
The expectation is that in order for this to qualify for backport to 8.4.x, both #2862041: Provide useful Views filters for Content Moderation State fields and #2902187: Provide a way for users to moderate content need to be in 8.5.x too.
So to clarify - has not been cherrypicked to 8.4.x.
So leaving this at 8.4.x and at RTBC.
Comment #70
jibranAny word on the backport yet?
Comment #71
timmillwoodOther than being past the alpha/beta stages, I can't see any reason not to cherry pick this into 8.4.x.
Assigning to xjm to make a decision on this.
Comment #72
xjmWhat I'd prefer to do is to get all three issues in to 8.5.x first (once they've had complete reviews etc.), and then consider backporting them together. So let's leave this at RTBC for now. Thanks!
Comment #73
xjmLooks like @larowlan also already said exactly that. Anyway it doesn't need to be assigned to me. :) The committer team will decide together when we decide whether to backport the view itself.
Comment #76
webchickOk, we had a meeting with the other core committers where we discussed the current state of Content Moderation. The general agreement was that we're really close to beta-level stability; we just need the trio of this issue, #2862041: Provide useful Views filters for Content Moderation State fields, and then #2902187: Provide a way for users to moderate content.
In order to give Content Moderation room to get to beta for 8.4.0 (which would be a huge win), there was agreement to backport this issue to 8.4.x, despite the fact that technically this type of patch would've been cut off at 8.4.0-beta1. Hopefully we can also get the other two issues put away in time for 8.4.0's release, but at least this one we don't have to stress about anymore. :)
Soooo! Cherry-picked 1fa12fb to 8.4.x. Thanks so much for all of your hard work on this, all! <3
Comment #78
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedPublished the CR now that this is backported.
Comment #79
seanBRan into the computed entity reference filter issue again. Created a followup as suggested in #50.
#2936737: Return computed fields in FieldAPIHandlerTrait::getFieldStorageDefinition()
Comment #80
claudiu.cristeaWhat about computed fields defined via
hook_entity_bundle_field_info()
?Comment #81
jibran@claudiu.cristea I'm glad you asked would you like to review #2981047: Allow adding computed bundle fields in Views?