Problem/Motivation
Since #2852067: Add support for rendering computed fields to the "field" views field handler Views can render computed fields. However, this only works for computed base fields not computed bundle fields.
The problem is that generally a view is not specific to a given bundle, so we cannot know up-front to which bundle a (computed) bundle field may belong.
Steps to reproduce
To reproduce this issue you must implement hook_entity_bundle_field_info_alter()
to add a computed bundle base field to an existing entity, and then also add a hook_views_data_alter()
implementation to expose the new bundle field to views. If you then attempt to add that bundle base field to a view, you will see an error like:
Error: Call to a member function getType() on null in Drupal\views\Plugin\views\field\EntityField->defineOptions() (line 370 of
/var/www/html/repos/drupal/core/modules/views/src/Plugin/views/field/EntityField.php)
Here are example files that can be used to test this as part of a module called computed_bundle_field_test:
computed_bundle_field_test.module
<?php
/**
* @file
* Testing computed bundle fields.
*/
use Drupal\Core\Entity\EntityTypeInterface;
use Drupal\computed_bundle_field_test\FieldStorageDefinition;
/**
* Implements hook_entity_bundle_field_info_alter().
*/
function computed_bundle_field_test_entity_bundle_field_info_alter(&$fields, EntityTypeInterface $entity_type, $bundle) {
if ($bundle === 'page') {
$fields['computed_bundle_string_field'] = FieldStorageDefinition::create('string')
->setLabel('Computed Bundle String Field')
->setName('computed_bundle_string_field')
->setDescription('A computed bundle string field')
->setComputed(TRUE)
->setClass('\Drupal\computed_bundle_field_test\ComputedBundleStringField')
->setTargetEntityTypeId('node')
->setTargetBundle('page')
->setTranslatable(FALSE)
->setRevisionable(FALSE)
->setReadOnly(TRUE)
->setDisplayConfigurable('view', TRUE)
->setDisplayOptions('view', [
'label' => 'visible',
]);
}
}
?>
computed_bundle_field_test.views.inc
<?php
/**
* @file
* Views functionality for the computed_bundle_field_test module.
*/
/**
* Implements hook_views_data_alter().
*/
function computed_bundle_field_test_views_data_alter(array &$data) {
$data['node_field_data']['computed_bundle_string_field'] = [
'title' => 'Computed Bundle String Field',
'field' => [
'title' => 'Computed Bundle String Field',
'id' => 'field',
'field_name' => 'computed_bundle_string_field',
],
];
}
?>
src/ComputedBundleStringField.php
<?php
namespace Drupal\computed_bundle_field_test;
use Drupal\Core\Field\FieldItemList;
use Drupal\Core\TypedData\ComputedItemListTrait;
/**
* The ComputedBundleStringField class.
*/
class ComputedBundleStringField extends FieldItemList {
use ComputedItemListTrait;
/**
* Compute the field value.
*/
protected function computeValue() {
$this->list[0] = $this->createItem(0, 'THIS IS A COMPUTED FIELD');
}
}
src/FieldStorageDefinition.php
<?php
namespace Drupal\computed_bundle_field_test;
use Drupal\Core\Field\BaseFieldDefinition;
/**
* A custom field storage definition class.
*
* For convenience we extend from BaseFieldDefinition although this should not
* implement FieldDefinitionInterface.
*
*/
class FieldStorageDefinition extends BaseFieldDefinition {
/**
* {@inheritdoc}
*/
public function isBaseField() {
return FALSE;
}
}
Once these files are in place then you should be able to add the Computed field to views and any/all Page nodes should display a value of THIS IS A COMPUTED FIELD for that column.
Proposed resolution
Allow adding views data for (computed) bundle fields and - if it is - fetch the respective field definition for those bundles. Note, if multiple bundles need to specify different computed fields with different underlying classes, they will need to be added separately to the views data definition.
Remaining tasks
Create 9.4.x PatchCreate 10.x PatchCreate 11.x PatchAdd Tests- Review & Merge
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
Comment | File | Size | Author |
---|
Issue fork drupal-2981047
Show commands
Start 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:
- 2981047-views-computed-bundle-fields-11-0 changes, plain diff MR !4139
- 11.x compare
- 9.5.x compare
- 9.4.x compare
- 2981047-views-computed-bundle-fields-10-0 changes, plain diff MR !1864
- 2981047-views-computed-bundle-fields-10-0-backport changes, plain diff MR !4224
- 2981047-views-computed-bundle-fields-9-5 changes, plain diff MR !2511
- 10.0.x compare
- 2981047-views-computed-bundle-fields-9-4 changes, plain diff MR !1578
- 2981047-views-computed-bundle-fields-3 changes, plain diff MR !1371
- 2981047-views-computed-bundle-fields-2 changes, plain diff MR !1019
- 2981047-views-computed-bundle-fields changes, plain diff MR !727
Comments
Comment #3
tstoecklerHere's a test based on the tests added in the parent issue. Also this test should apply.
I looked for some place to document the new behavior - including the new
'bundle'
key in the views data, but as far as I can tell we don't document any of the entity-specific keys anywhere, so I guess we should be good to go for the scope of this patch. I guess we should amend the change notice or alternatively add a separate one here.Comment #4
tstoecklerComment #6
tstoecklerAhh, I always forget that
git add -N
- while being useful - generates weird patches. Here's a proper "tests-only" patch.Comment #8
k4v CreditAttribution: k4v commentedHere is a reroll for Drupal 8.5.4
Comment #9
k4v CreditAttribution: k4v commentedThe patch works aaaawweeessoome for my problem, thanks Tobias!
Comment #11
tstoecklerRe-uploading the latest patch for the RTBC. Thanks @k4v
Comment #12
snable CreditAttribution: snable as a volunteer and commentedThanks for the patch! Great work!
Comment #13
k4v CreditAttribution: k4v commentedbtw: the 8.5.4 patch applies, but not to 8.5.x...
Comment #15
tstoecklerRandom test fail
Comment #17
TwoDJust a thought but would it be more efficient to do it this way as getting the field list with a bundle also includes base fields?
Can't bundles also override some details so checking with a bundle first would include that case?
Comment #18
tstoecklerGreat idea, let's do it!
Comment #19
TwoDI took the liberty to tweak the comment a bit as well, what do you think?
Comment #20
tstoecklerLooks great to me. Not sure I am allowed RTBC, though.
Comment #21
jibranThis patch has a very deja vu feeling to it because of #2852067: Add support for rendering computed fields to the "field" views field handler. Changes look good just a doc nit other than that it is RTBC.
I'm sure @amateescu doesn't approve of this but there is no correct API exist to create computed bundle field.
s/check those/check the
Comment #22
jibranI think we should add @todo to #21.1 linking to #2986634: Discuss the role of field storage definitions for computed fields..
Comment #23
TwoDUpdated with the changes from #21 & #22.
EDIT: Ugh, I was thinking of #2935932 when wording the comment but the linked issue lists it at the top so maybe it's ok?
Comment #24
jibranIt is perfect.
Comment #26
k4v CreditAttribution: k4v commentedThis should have a changelog entry describing the new views data structure, right?
Comment #27
jibranI think we can just update https://www.drupal.org/node/2904410 once the issue is fixed.
Comment #28
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThis isn't quite right. The method expects a storage definition to be returned and the only reason a storage definition is returned in the test case is because
BaseFieldDefinition
implements both the storage and definition interfaces.I believe if the computed bundle field had an associated storage definition (which @amateescu and @tstoeckler agreed should be the case in #2935932: Add a FieldDefinition class for defining bundle fields in code.), this would already just work.
I think the test coverage is still valuable, but this should be reevaluated once we have the capabilities to properly test this.
Comment #29
TwoD@Sam152 I'm not following your comment, what would already just work without the changes here?
I personally don't agree having storage definitions for computed bundle or base fields makes sense at all but the interface already dictates that they do so I guess we have no choice without breaking the API.
Comment #30
jibranI don't think we can fix #28 in core atm without #2935932: Add a FieldDefinition class for defining bundle fields in code.. It makes sense to postpone this on #2935932.
Comment #31
plachParent issue fixed.
Comment #32
jibranThis should address #28.
This is kind of blocked on #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info() as well but we can just add a todo and then replace the use statement in #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info().
Comment #34
jibranComment #36
jibranThis should be green.
This shows
FieldDefinition::createFromFieldStorageDefinition()
should at least runsetComputed
andsetClass
. Seems like #2935932: Add a FieldDefinition class for defining bundle fields in code. missed it.#32 shows the error when
setComputed
andsetClass
are not called onFieldStorageDefinition
#34 shows the error when
setComputed
andsetClass
are not called onFieldDefinition
Comment #37
jibranMaybe #2986836: Support computed bundle fields by adding the notion of computed field storage definitions will fix it by adding a new class.
Comment #39
jibranRerolled after #3023981: Add @trigger_error() to deprecated EntityManager->EntityRepository methods.
Comment #40
jibranReroll.
Comment #41
claudiu.cristeaThanks, @jibran for pointing me to this issue. While I'm not yet familiar with all changes to field/storage defs, I'm only warming up :)
What happens if the field is attached to more than one bundle within the same entity type, which is really used in the wild? Should we set the views data
bundle
key as array, instead of string?It seems that we have to postpone on #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info()? But reading https://www.drupal.org/node/2982512 it looks like we'll have to use the new
FieldDefinition
? Not very clear to me.Comment #42
jibranThanks, for having a look at the patch
, thoughts?
Comment #44
jibranTests were conflicting with some other patch so here is a fix only patch. #42.1 is still pending.
Comment #45
owenbush CreditAttribution: owenbush commentedI've been working on a contrib module that makes use of computed bundle fields and applying the fix only patch #44 seemed to resolve the issue for me. It would be super to get this into D8.9 and not have to wait until 9.1 because it cripples the module I'm working on's functionality.
So if there is anything I can do to get this over the line please do let me know.
Comment #48
owenbush CreditAttribution: owenbush commentedI've tried my best to wrap my head around what should actually happen in the instance that a computed bundle basefield exists on multiple bundles. For example, the question arises, which of those definitions should we use? I've assumed that it should be the first one we come across with the appropriate field_name. I'm not sure if that assumption is correct or not.
I'd love to get this moving more and happy to help out however possible.
Comment #50
owenbush CreditAttribution: owenbush commentedAttaching a new patch to fix the failing tests.
Comment #51
owenbush CreditAttribution: owenbush commentedGo tests go
Comment #52
jibranThe patch looks good now. Let's update the IS.
Let's add test for multiple bundles as well to cover all the bases.
Comment #53
owenbush CreditAttribution: owenbush commentedI guess I am slightly confused as to what to do for multiple bundles. I could update that test views data to something like
'bundles' => ['entity_test_computed_field', 'someother_bundle'],
But if that bundle does not actually exist in the test data, then it is not an accurate test. Do we need to mock up a second bundle on that entity with the shared computed field? And then set that value to something different and assert the value?
Sorry, I haven't done a lot of core unit tests so I'm not exactly sure of the correct approach.
Comment #54
LendudeBesides testing for two bundles that the field is on, I would also like to see a test where there are two bundles rendered in the View and the computed field isn't on one of them (so it would show up empty once).
Comment #57
owenbush CreditAttribution: owenbush at Lullabot commentedI have put together a merge request that includes all the work from previous patch #50 and additional changes to add more tests.
If you look at the second commit on the MR you can see the changes made since the initial branch creation using patch #50.
What I did was enabled the EntityTestComputedField entity to have bundles, then I create 3 different bundles and an entity for each. The first two bundles will have the bundle computed basefield, the third will not. This way I can check the following:
1) Do all the bundles have the computed basefield as you would expect?
2) Do the first two bundles have the bundle computed basefield?
3) Does the third bundle not have the bundle computed basefield?
Tests seem to be passing so I think I got things covered, but please let me know if there is more to do here.
Comment #58
kim.pepperComment #59
owenbush CreditAttribution: owenbush at Lullabot commented@Lendude is there any chance you could take another look at this issue, the MR in #56 has added a number of new tests which should cover the use-cases you requested in #54
Thanks, and I'm happy to make any necessary changes to get this issue moving.
Comment #60
tim.plunkettHiding the old patches now that work is taking place in an MR.
I filled in the rest of the issue summary except the steps to reproduce. Those are always good, but since this already has automated test coverage, it isn't necessary.
Assigning to @Lendude for final sign-off.
Comment #61
tim.plunkettAlso there was another commit adding something to the end of
entity_test.schema.yml
, so this needs a reroll. My approach to side-stepping these conflicts: don't add to the end of the file. Find some other nice place in the middle to add the definition.Comment #62
owenbush CreditAttribution: owenbush at Lullabot commentedUgh. Well that went horribly wrong. I'm looking at resolving this now.
Comment #65
owenbush CreditAttribution: owenbush at Lullabot commentedMy rebase destroyed the previous MR, so I've sorted it out by opening a new MR, merged back in the latest changes to 9.3.x which included the change to entity_test.schema.yml mentioned by Tim.
Comment #66
owenbush CreditAttribution: owenbush at Lullabot commentedLooks like we're green again and the MR is mergeable.
Comment #67
owenbush CreditAttribution: owenbush at Lullabot commentedOriginally, the idea with this issue was to specify a specific bundle for which a bundle field can exist, but during the process of refining the problem and implementing the solution it was decided that specifying multiple bundles was a more flexible approach. As such I have updated the issue's proposed solution to reflect the change from `bundle` to `bundles` as the views data key name.
Comment #68
LendudeThe tests look good.
One last thing I'm concerned about is docs. We are introducing a new key for Views data, but we are not providing any docs for it, how are people going to know how this works? A quick search didn't reveal much in the way of docs for views data for computed fields anyway, none were added in #2852067: Add support for rendering computed fields to the "field" views field handler. So maybe we could do that in a follow up, but here might be a good place to rectify this.
Maybe adding an example to hook_views_data in views.api.php? There doesn't seem like another place where the structure of Views data is explained better.
Comment #69
owenbush CreditAttribution: owenbush at Lullabot commented@lendude thanks for the follow-up, I really appreciate you helping to move this along.
I've added some documentation to views.api.php here is the commit that adds the documentation.
https://git.drupalcode.org/project/drupal/-/merge_requests/1019/diffs?co...
Does this satisfy the documentation requirements? If not, I'm happy to take direction and add something more.
Comment #70
joachim CreditAttribution: joachim at Factorial GmbH commentedIt would be nice to wait until #3116481: Convert EntityViewsDataTest from a unit test to a kernel test is committed, and then we could add tests for the ViewsData handler?
Comment #72
wellsDocumentation looks good to me (was looking for this functionality myself).
This was no longer applying to 9.3.x so I merged and fixed a small conflict, but now it looks like tests are failing due to some changes from #2997123: Cacheability of normalized computed fields' properties is not captured during serialization.
Comment #73
owenbush CreditAttribution: owenbush at Lullabot commentedI've rebased the MR against the latest 9.3.x branch but I guess we're still waiting on #3116481: Convert EntityViewsDataTest from a unit test to a kernel test to land so we can rework the tests.
Comment #74
owenbush CreditAttribution: owenbush at Lullabot commentedComment #75
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedSetting it back to needs works as it's not green, thanks.
Comment #76
owenbush CreditAttribution: owenbush at Lullabot commentedWe're finally back to green. Still have to wait on #3116481: Convert EntityViewsDataTest from a unit test to a kernel test making the cut.
Comment #77
owenbush CreditAttribution: owenbush at Lullabot commentedThese rebases to keep up with core are getting ... fun.
Comment #78
larowlanStatus update
Comment #80
owenbush CreditAttribution: owenbush at Lullabot commentedThe previous MR got way out of control with rebasing, and each time core changed I was losing at least an hour trying to rebase and deal with conflicts. So, I resolved the conflicts, exported a patch, and applied the patch to a new MR. I'll hide the previous one as it is no longer applicable.
Comment #82
owenbush CreditAttribution: owenbush at Lullabot commentedAt this point nothing is changing in the MR for this work, I am just keeping it up to date with the latest in 9.3.x by rebasing, so if we could get a review that would be absolutely great.
Comment #85
owenbush CreditAttribution: owenbush at Lullabot commentedI created a new PR against 9.4.x as switching the target branch did not work out well at all. So we have an MR for 9.3.x (which is now obsolete) and a new MR against 9.4.x
Comment #86
jibranThe changes look good. @Lendude's last feedback is addressed so it RTBC for me but I can't RTBC it as I worked on the patch before.
Comment #87
LendudeSorry, this fell off my radar in a big way it seems.
This now looks ready to me too, thanks for the great work!
Comment #88
larowlanThis looks good to me too, however it would be good to see a green run on 10.0.x too.
I'm going to change the target of the MR to 10.0.x and run a test run.
Comment #89
larowlanUnfortunately this doesn't apply cleanly to 10.0.x and 9.4.x
Can we get a second MR that applies cleanly against 10.0.x
Thanks folks
Comment #90
larowlanUsing the Needs reroll tag to surface contributors who may be able to create a 10.0.x version of the MR
To be clear, this doesn't need reroll, it needs a second MR created that applies to 10.0.x
Comment #93
ankithashettyHere is a new MR that applies to 10.0.x, thanks!
Comment #94
owenbush CreditAttribution: owenbush at Lullabot commentedI've rebased the 9.4.x MR, and we now also have a 10.0.x MR that's green. Can we get this reviewed again, please?
Comment #95
jucedogi CreditAttribution: jucedogi commentedI encountered this issue for a view used in the recurrent events module.
Within these patches there is none for 9.3.7 which is the version the site being worked on is being upgraded to,
I modified the patch given for 9.2 so that it worked with 9.3.
Tests have shown the site is working as intended.
Patch for 9.3.7
All credits go to owenbush since I just modified for it to work in 9.3.7
Comment #96
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedFixed CS error
Comment #97
mradcliffeI think the issue summary could use an update to identify the steps to reproduce, which are currently N/A and the Remaining tasks which is TBD.
Since we're adding an option, do we need a change record?
Comment #98
owenbush CreditAttribution: owenbush at Lullabot commentedI have altered the original description to add the steps to recreate, and the remaining tasks.
In response to the question about needing a change record, I do not know what to say on that front and would need a Views maintainer to confirm. This did make it to RTBC before the request for the 10.x patch was made, so I would assume not at this point given that the option is now documented in the views api.
Can we try and get this moving again? Nothing has changed in the patches for quite some time, we're just rebasing over and over. I am at DrupalCon if anyone wants to walk through this.
Comment #99
owenbush CreditAttribution: owenbush at Lullabot commentedComment #103
owenbush CreditAttribution: owenbush at Lullabot commentedClosed MR for 9.3.x, updated MR for 9.4.x and 10.0.x, and added new 9.5.x MR.
It would be super lovely/marvelous to try and get this reviewed and committed.
Comment #104
marcoscanoRestoring RTBC, nothing seems to have changed since #87, and the request from #89 seems to have been addressed, AFAICT.
Comment #105
pixiekatJust patched on 9.3.x after having this problem with one of my views and the patch worked flawlessly. I was able to save my work immediately after installing #96.
Comment #106
larowlanSaving issue credits
crediting @claudiu.cristea and @Sam152 for reviews
crediting @tim.plunkett for issue summary updates
Comment #107
larowlanLeft a couple of nice-to-have comments about nesting/complexity, one personal comment about in_array vs array_search which can be taken/left but one question about what to do if two bundles have the same computed field name with a different computed field class which I think we at least need to discuss.
Comment #108
joachim CreditAttribution: joachim as a volunteer commented> I think the only thing we can do is get the definition for all bundles and make sure they all have the same computed class and if not raise an exception or warning or similar? There's just not enough context to do anything else.
That doesn't seem to be ideal functionality.
What if you mean to have different classes?
We possibly need to add each bundle field as a separate field declaration in views?
Comment #109
larowlanYeah that's a great idea
So how do we move forward? There's nothing to do right, the onus is on the developer, we just need a documentation update in the views.api.php change?
Comment #110
owenbush CreditAttribution: owenbush at Lullabot commentedI've tried to address all the comments, tests are running and hopefully they will pass and I'll move back to Needs Review
Comment #111
owenbush CreditAttribution: owenbush at Lullabot commentedLooks like PHPStan did not actually like the new PHP8 NULL safe operator, so I've reverted that change.
Comment #112
owenbush CreditAttribution: owenbush at Lullabot commentedBack to needs review.
Comment #114
larowlanCan the MR be rebased onto 10.1.x
Left some comments on the MR, I think we still need to handle #107/ #108
Comment #115
larowlanAlso, I'm not sure if this is a bug or a task or a feature
Comment #116
jibranI'd say it is a task as we already support non-bundle fields so this is not a new feature. Also, nothing is broken as we don't support them yet so it is not a bug.
Comment #117
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedWe have 3 MRs open which is very confusing and not necessary. Feedback is scattered throughout. Can we please close them all except the one against 10.1.x (which I think will need to be created, or 10.0.x repurposed)
Comment #118
Prem Suthar CreditAttribution: Prem Suthar at Material for Drupal India Association commentedAdd The Patch Re-roll For 10.1.
Comment #119
wxman CreditAttribution: wxman commentedAre any of the patches workable for a 9.5.x site?
Comment #120
vlad.dancer@wxman we're using patch from MR 2511, works fine.
Comment #121
wxman CreditAttribution: wxman commented@vlad.dancer @joachim I don't know what I could be doing wrong, but still don't have access in views. I applied the patch successfully to 9.5.7, cleared the caches twice, but still I don't see the one computed field in Views. This is just a simple test site with the only content being a test of the computed field version 4. The field is working as I wanted it to, and it shows in the field list as a computed field.
Comment #122
joachim CreditAttribution: joachim as a volunteer commented> cleared the caches twice, but still I don't see the one computed field in Views.
The patch *allows* bundle fields to be used. It doesn't automatically register them.
I commented on this further up, saying the patch should do this, but I'm no longer sure, as it may make it a fair bit more complex. For base fields, it's been done in two steps -- #2852067: Add support for rendering computed fields to the "field" views field handler and #3349739: Automatically declare computed base fields to Views
Comment #123
John Pitcairn CreditAttribution: John Pitcairn commentedWith 9.5.8, using the MR 2511 patch and the example from the issue summary, I can successfully add the computed field to a view of profile entities, referencing the entity base table in views data as:
But I get no output from it. The error is
Drupal\Core\Entity\Sql\SqlContentEntityStorageException: Column information not available for the 'computed_bundle_string_field' field. in Drupal\Core\Entity\Sql\DefaultTableMapping->getFieldColumnName() (line 440 of /var/www/html/public/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php).
Should this be able to work, and I am missing some crucial mapping in the views data definition?
Comment #124
KumudBthis issues is resolved. MR has been created and merged for Drupal 10.x.1MR !1864 mergeable Please review it.
Comment #125
joachim CreditAttribution: joachim as a volunteer commented@124: There are various issues on the MR to resolve.
@123: I tried the diff from the 9.5 branch, with a bundle computed field made with Computed Field module (https://www.drupal.org/project/computed_field). It worked fine (apart from warning that was caused by a bug in Computed Field module, which I've now fixed).
I suspect it's a problem with the definition of your field.
Tagging as a contrib module blocker, since this affects https://www.drupal.org/project/computed_field.
Comment #126
Jan-E CreditAttribution: Jan-E commentedAdded a reference from Add Views data automatically as the Computed Field Plugin module also relies on this issue.
Comment #129
joachim CreditAttribution: joachim as a volunteer commentedTagging for IS update, as I don't think the proposed 'bundles' key is actually necessary -- see my prior comment.
Comment #130
owenbush CreditAttribution: owenbush at Lullabot commentedI'm working on this in Pittsburgh. Hoping to have all the requested changes in to a new MR for 11.x before the end of the conference.
Comment #132
joachim CreditAttribution: joachim as a volunteer commentedDefinitely this:
> I add the EntityTypeBundleInfo service with dependency injection to the EntityField views class.
There's no point to having the 'bundles' key - the information is mostly ignored. It's bad DX to make developers specify it.
Comment #133
owenbush CreditAttribution: owenbush at Lullabot commentedUpdating issue summary.
Comment #134
owenbush CreditAttribution: owenbush at Lullabot commentedI've spent a long time trying to figure this out without specifying the 'bundles' key in EntityTestViewsData::getViewsData or in entity_test_views_data_alter().
The bundles are configured using 'entity_test_create_bundle()' within ComputedBundleFieldTest::setUp(), however, using xdebug and setting breakpoints where the bundles are configured, the code never actually gets to that point because it dies with the following before it ever gets there:
So looking through the backtrace and adding breakpoints it appears that the test view is installed and EntityTestViewsData::getViewsData executes as part of setting up the parent of the test (parent::setUp()) and then EntityField attempts to look for the field definition of the bundle fields. Using the EntityTypeBundleInfo service to find the bundles that have the computed bundle fields but the bundles do not (yet) exist, because the rest of the test's ::setUp() function have not yet executed. This blows up as above.
If I use the EntityTypeBundleInfo service to find the bundles (there is only the default bundle at execution time) it doesn't work, but if I execute the EntityFieldManager::getFieldDefinition() on hardcoded bundle names, it does work. So it appears the bundles are not yet initialized, or in cache, but executing getFieldDefinition() seems to work.
The field map also does not contain the bundles at this point, for the same reason - the setUp code has not finished executing.
The only way I can reliably use EntityFieldManager::getFieldDefinition() is if I already know the machine name of the bundles, and the only way that I can see for that to work is to define the 'bundles' key in the views data.
At this point I'm out of ideas and if anyone else has any ideas I'd appreciate some help. I've pushed a lot of code to this issue and have hit a brick wall.
Comment #135
joachim CreditAttribution: joachim as a volunteer commentedDoes it also have this problem outside of tests?
Comment #136
owenbush CreditAttribution: owenbush at Lullabot commentedThe code as-is works for me outside of the context of the tests.
Comment #137
joachim CreditAttribution: joachim as a volunteer commentedI'm a bit confused about the need for the 'bundles' property or even the logic surrounding it, as I've just made a custom module that defines a computed bundle field and it shows up fine in Views with this in hook_views_data():
I'm attaching the module -- I've written it on D9 as I don't have a local D10 yet.
Just install it & create a node of the supplied bundle and then go to the view at /test-2981047.
EDIT: Ah, I see my mistake -- hook_entity_field_storage_info() is necessary when defining normal bundle fields, but with a computed field, you don't need it. If I comment out that hook in my demo module, the node loads and displays properly, but the view crashes.
Comment #138
joachim CreditAttribution: joachim as a volunteer commentedI've figured out the problem with the test:
that tries to import the test view, but it's run BEFORE the bundles are defined, and that's why EntityTypeBundleInfo has nothing.
Working on it.
Also, BTW, if we're using entity_test_create_bundle() then I don't think we need a bundle entity type, as that function stores dummy bundle info in state.
Comment #139
joachim CreditAttribution: joachim as a volunteer commentedGot it working on 9.5.x.
I've cherry-picked to the D10 branch, but the cherry-pick to the D11 branch is conflicting - not sure why? Could someone take a look at it?
Comment #140
joachim CreditAttribution: joachim as a volunteer commentedAh, D11 has this:
which actually does what we want, but the comment only mentions base fields! And what business is it of base fields to check every bundle???
Comment #141
owenbush CreditAttribution: owenbush at Lullabot commentedThe reason we check every bundle is because we need to find the first definition of the computed bundle field, which could be shared across one or more bundles. So EntityField is unaware of which bundles define the field, so we loop through all bundles until we find a definition for the computed field. When I was testing I wasn't able to use the Field Map, but that was likely due to the same issue you solved with the view being imported before the bundles were defined.
So we may be able to use the field map instead?
Comment #142
joachim CreditAttribution: joachim as a volunteer commentedThe D10 branch is working - works manually, and test passes.
I'm not sure what to do on the D11 branch as it looks like that's got a lot of your experimental commits on it. Could you have a look at it? There's two new commits I've made on the D10 branch which need to be brought into D11.
> The reason we check every bundle is because we need to find the first definition of the computed bundle field, which could be shared across one or more bundles
Yup, that's what I've done on D10 too.
> // Check for computed bundle base fields too.
I see why I was confused now -- it's just this comment that doesn't make sense. The fields can't be BOTH 'bundle base'. They're one or the other!
Comment #143
owenbush CreditAttribution: owenbush at Lullabot commentedI see that comment is confusing. I'll get the D11 branch figured out with what you've done in D10. Some other changes I made in the D11 branch were to make these computed bundle field tests completely separate from computed base field tests. So there was some refactoring and renaming going on.
Comment #144
owenbush CreditAttribution: owenbush at Lullabot commentedI have updated the 11.x version of the MR to factor in your changes. I am now seeing this working locally manually and in tests, so hopefully this goes green.
The 11.x MR and the 10.x MR now are quite different because of the refactoring for separating out the tests and creating a different entity and bundle name to test with.
Edit: So if the 11.x goes green and seems acceptable, maybe it can be backported for 9.5 and 10?
Comment #145
owenbush CreditAttribution: owenbush at Lullabot commentedI'm not sure what to do about the custom commands failing on the 11.x branch, EntityField::getFieldStorageDefinition() even without my changes (so as it exists currently in core: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/views...) may not actually return anything.
I tried setting the return type of the function to FieldStorageDefinitionInterface or NULL, and then returning NULL at the end if no definitions are found, but it seems like the check is still failing. I don't know how the change made to lookup the bundle fields does anything different to what is already happening in that function. Is this also a backwards incompatible API change?
Comment #146
smustgrave CreditAttribution: smustgrave at Mobomo commentedThere's a line in phpstan-baseline.neon that can be removed.
Comparing 10.x MR and 11.x MR though and see core/modules/views/tests/src/Unit/Plugin/field/FieldTest.php is updated in 11.x is that intended?
Comment #147
owenbush CreditAttribution: owenbush at Lullabot commentedThanks for pointing out the phpstan config I could remove.
As for the difference between 10.x and 11.x MR I dependency inject the entity_type.bundle.info service into the EntityField class in the 11.x MR. I don't think the 10.x MR does that. The FieldTest class instantiates an EntityField object several times, so I had to pass through the service as well.
One option to get around that would be to make the entity_type.bundle.info service optional in EntityField, and if it is not passed, instantiate it in the EntityField::__construct(). I feel like I've seen this approach elsewhere in core. That would mean we wouldn't need to update the FieldTest class, but it felt like the right thing to do, to pass all the required services.
Comment #148
owenbush CreditAttribution: owenbush at Lullabot commentedThe failures were kind of expected in the jsonapi tests as I didn't modify any of those. I'll get those looked at tomorrow.
Comment #149
joachim CreditAttribution: joachim as a volunteer commented> Some other changes I made in the D11 branch were to make these computed bundle field tests completely separate from computed base field tests
That's probably a good thing to do in D10 as well.
Another thing about the tests - if you're using entity_test_create_bundle() to mock bundles, I don't think you need to define a bundle entity at all.
> As for the difference between 10.x and 11.x MR I dependency inject the entity_type.bundle.info service into the EntityField class in the 11.x MR
Ah yes, because I was experimenting I used \Drupal::service() to get the bundle info service. It should be injected - there's a pattern to add additional services to a class in a backwards-compatible way.
Comment #150
owenbush CreditAttribution: owenbush at Lullabot commented11.x is green which is great.
So a couple of questions are outstanding:
1. The 10.x branch is missing a couple of changes from the 11.x branch such as the dependency injection of the service in EntityField, and the separation of the computed base field tests and the computed bundle field tests. So, how should we approach that? Backport a new version from 11.x to a new 10.x MR, or attempt to recreate the changes in the current 10.x MR?
2. Do we need to ensure that the service dependency injection is backwards compatible in the 10.x branch? What about the 11.x branch? My thoughts are that the service should be optional on the 10.x branch, but maybe deprecate it being optional in the 11.x branch and make it required, and trigger an error if the service is not passed in as an argument.
3. Anything else we are missing to get this through?
Comment #151
smustgrave CreditAttribution: smustgrave at Mobomo commentedMay help you decide but the code has to be merged into 11.x first and backported.
11.x is the main branch currently with D10 releases being cut from that.
So if something needs to be deprecated it will only go into 11.x branch. Don’t think policy is to backport deprecations. But if this were merged in soon it would be deprecated in 10.2 but required in D11.
As 10.2 will be tagged off 11.x
Hope that helps.
Comment #152
owenbush CreditAttribution: owenbush at Lullabot commentedThats helpful thank you so much. I had no idea about that process. So, to me this feels ready to review. If someone disagrees then it can be changed back. But for now I'll move this to Needs Review.
Comment #153
joachim CreditAttribution: joachim as a volunteer commented> 1. The 10.x branch is missing a couple of changes from the 11.x branch such as the dependency injection of the service in EntityField, and the separation of the computed base field tests and the computed bundle field tests. So, how should we approach that? Backport a new version from 11.x to a new 10.x MR, or attempt to recreate the changes in the current 10.x MR?
We should make the D10 branch as similar as possible to the D11 branch:
- Make the tests the same.
- Use the BC-compatible pattern for DI in EntityField
Comment #154
owenbush CreditAttribution: owenbush at Lullabot commentedThanks for the review, joachim. I think I've addressed the issues you pointed out in the 11.x MR
1. You were right, no bundle class was necessary, and as a result a bunch of other things changed - including no longer needing (maybe we never needed one anyway) the JSONAPI test, which I added to ensure the bundle info was represented appropriately with jsonapi, but now we don't have a test bundle we definitely do not need that.
2. I addressed the comment in the views api you mentioned.
Once the 11.x MR is looking good, I'll go ahead and work on the 10.x MR to get it in sync.
Comment #157
owenbush CreditAttribution: owenbush at Lullabot commentedDue to the number of changes in the 11.x MR it made more sense to backport the 11.x changes to a new branch rather than try and update the 10.x MR and have to force push, so I guess that can be closed (although, I couldn't do it myself).
I also closed out the 9.5 MR. So ideally we would just have the 11.x MR and the new backported 10.x MR.
Comment #158
owenbush CreditAttribution: owenbush at Lullabot commentedOk looks like we have green 11.x and 10.x MRs.
The 10.x branch differs from the 11.x branch in FieldTest and EntityField.
EntityField has an optional argument of the entity_type.bundle.info service, if the service is not passed the constructor will retrieve it from the container and triggers a deprecation error (suppressed) to warn that in 11.0.0 the service is no longer optional.
This also affected the FieldTest class which in the 10.x MR had to mock the entity_type.bundle.info service and pass it into the instantiations of EntityField.
Other than those differences the 10.x MR is a direct backport of the 11.x MR.
Marking this as Needs Review.
Comment #159
smustgrave CreditAttribution: smustgrave at Mobomo commentedAppear to be some open threads on MR 4139
Comment #160
owenbush CreditAttribution: owenbush at Lullabot commentedMR 4139 (11.x) has been updated after the review from Joachim.
MR 4224 (10.x backport) has been updated with the same changes.
Moving back to Needs Review.
Comment #161
smustgrave CreditAttribution: smustgrave at Mobomo commentedLets get this in front of the committer eyes.
Comment #162
quietone CreditAttribution: quietone at PreviousNext commented@smustgrave, It seems you have confidence that a committer should look at this. But your comment is brief and makes me wonder if there is a specific part of this a committer should look at. If not, then the comments should include the steps you have taken to determine this is ready for a committer's time. For reference there is a list of items to check in Triage the Drupal core RTBC queue. And then stating what you have done, or not done, greatly helps a committer.
Comment #163
joachim CreditAttribution: joachim as a volunteer commentedI'd agree this is RTBC. The tests are green and I've reviewed the MR several times.
Comment #164
quietone CreditAttribution: quietone at PreviousNext commented@joachim, thanks for the reply. It looks like I didn't triage this properly and I started at the bottom and not the top. Sorry about that.
I read the issue summary and then through the comments, mostly looking for questions unanswered. I saw more than one request for an Issue Summary update. Thank you! That makes it easier for committers and reviewers. (though, why I didn't start with that in July I don't know). In reading the comments I think everything has been answered. I wasn't sure about #144. But on reading the relevant comments I think that settled on adding an example to views.api.php. And that has been done. So, I didn't find any thing missed in the comments.
This is a challenging issue to triage! It is long and 3 MRs with comments!
I did not update credit or review the code.
Comment #165
quietone CreditAttribution: quietone at PreviousNext commentedI forgot to respond to the question about a CR. #3, #97, #98
Yes, it is worth adding a change record, adding tag. There were two comments about amending an existing change record, https://www.drupal.org/node/2904410. That is not an option because a change record is tied to a release. And thus each issue should have it's own change record so that the version field is correct.
Because this needs a change record, I am setting it to needs work.
Comment #166
joachim CreditAttribution: joachim as a volunteer commentedDone a CR, but feels a bit pointless to me as there is no API change, and to me this is a bug fix -- thing that used to cause a crash doesn't cause a crash any more.
Comment #167
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedCR reads well, this should be ready now.
Comment #168
mvonfrie CreditAttribution: mvonfrie as a volunteer and at trinitec IT Solutions & Consulting GmbH commentedOn Friday I tried MR !1864 but I was not able to apply that one as patch. For me it looks like that is an older MR which will be discarded in favor of !4224?
MR !4224 on 10.1.2 with Commerce 2.36.0 works for me for a computed bundle field on the commerce order item entity.
Nevertheless I found an error in the example in `views.api.php` in both MRs, as following the example in !4224 I got the error
See my review comment in !4224 for details.
Comment #169
joachim CreditAttribution: joachim as a volunteer commented> On Friday I tried MR !1864 but I was not able to apply that one as patch. For me it looks like that is an older MR which will be discarded in favor of !4224?
Yes, MR !1864 is older. It's currently marked as not applying, so should be ignored.
> Nevertheless I found an error in the example in `views.api.php` in both MRs
Yup - the 'bundle' bit should be removed.
Comment #170
mvonfrie CreditAttribution: mvonfrie as a volunteer and at trinitec IT Solutions & Consulting GmbH commented@joachim, yes the 'bundle' bit should be removed too. But the field (plugin) id should be changed from 'computed' to 'field' as no computed plugin exists.
Comment #171
owenbush CreditAttribution: owenbush at Lullabot commentedI have updated both MR 4139 (11.x) and MR 4224 (10.x backport) to change the field ID from 'computed' to 'field'.
I had previously removed the 'bundle' part though, so I assume that was seen in MR 1864 which is old and not used (I wish I could close it, but I do not have the permission to do so).
Now I've addressed the issue with the views.api.php doc change I'm moving this back to Needs Review.
Comment #172
joachim CreditAttribution: joachim as a volunteer commentedMR !4139 says it's not mergeable.
(Which is why I got confused about the 'bundles' key -- I assumed the unmergeable MR was the old one.)
Comment #173
owenbush CreditAttribution: owenbush at Lullabot commentedMR !4139 was just behind 11.x a little, I've updated the branch to the latest 11.x, it is now mergeable again.
Comment #174
smustgrave CreditAttribution: smustgrave at Mobomo commentedWas previously RTBC in 167 and seems just needed a rebase so restoring status.
Comment #175
alexpottAdded some small review nits to the MR. Saving issue credits.
Comment #178
alexpottHiding all the files are closing the out-of-date MRs.
Comment #179
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedThanks @alexpott! Reviewed your changes and they fix your own feedback :) Back to RTBC!
Comment #180
joachim CreditAttribution: joachim as a volunteer commentedSmall docs nitpic, sorry!
Comment #181
alexpott@joachim actually I think here the better thing would be to blow up with a more helpful exception. Because all the usages of this method assume that it going to return a field storage. Which I think is the correct behaviour - if you are able to configure an entity field in views this thing should be able to get its field storage - if it can't then that is a error rather than something just to continue on from. I'd rather not add documentation - I think we should open a follow-up to trigger an exception. Created #3404304: EntityField::getFieldStorageDefinition() should throw an exception when it can't return a field storage to address this.
Comment #182
alexpottI removed the |null since there's no need to add this here as we're going to address this in #3404304: EntityField::getFieldStorageDefinition() should throw an exception when it can't return a field storage
Comment #183
alexpottCommitted cc1bc30 and pushed to 11.x. Thanks!
Comment #185
joachim CreditAttribution: joachim at Factorial GmbH commentedCan this be backported to 10.3?
Filed the obvious follow-up: #3404369: Automatically declare computed bundle fields to Views.
Comment #186
longwave@joachim 11.x will (currently) become 10.3, we haven't branched 10.3 yet.
Comment #188
mike-kelly CreditAttribution: mike-kelly commentedI see that this issue is being addressed via changes to Drupal core.
Is there any workaround for using computed fields with Drupal 10 views in the meantime, or should I take a different approach? I can insert my field into my view just using my custom module, but then I have to do a lot of template formatting to emulate the output of other table-based fields.
(Edited for clarity.)