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
When you have an entity reference to a view with the display formatter set to rendered entity you get the following error:
Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException: The "view" entity type did not specify a view_builder class. in Drupal\Core\Entity\EntityManager->getController() (line 263 of core/lib/Drupal/Core/Entity/EntityManager.php).
Proposed resolution
Add a view_builder class in the view plugin definition?
Remaining tasks
Write the patch.
User interface changes
n/a
API changes
n/a
Comment | File | Size | Author |
---|---|---|---|
#52 | the_rendered_entity-2204325-52.patch | 8.99 KB | Arla |
#50 | the_rendered_entity-2204325-50.interdiff.txt | 3.17 KB | Arla |
#47 | the_rendered_entity-2204325-47.interdiff.txt | 3.17 KB | Arla |
#47 | the_rendered_entity-2204325-47.patch | 7.86 KB | Arla |
#46 | the_rendered_entity-2204325-46.patch | 5.94 KB | Arla |
Comments
Comment #1
kokobutter CreditAttribution: kokobutter commentedcan you leave a little more info and detail about this problem.
Comment #2
benjy CreditAttribution: benjy commentedPatch attached that prevents the error.
if (empty($links) && isset($elements[$delta][$target_type][$item->target_id]['links'])) {
This was another error I found, "$result" is never defined.
Comment #3
tim.plunkettThis should have test coverage, by referencing some config_test entity that doesn't have a view builder.
I'm guessing this is for the links that are added onto nodes, and it was broken as you said. So we need coverage for that as well.
Comment #4
tim.plunkettComment #5
tim.plunkettSee #1857422: Add a ViewBuilder for Views for changing the views module.
Comment #6
Dave ReidThe 'links' functionality in this formatter is way more broken than this, so let's fix it separately with #2274975: Display/hide links on Rendered Entity formatter very broken.
Comment #7
Dave ReidI feel like we should be offering a solution a little higher-up, and not in the viewItems() method. If you think about it, the user shouldn't even be able to select this as a formatter if the entity type in the field settings doesn't support a view controller. Why offer a choice that will just never work?
Comment #8
BerdirTrue, but I don't we currently have an API that allows to check if a formatter is available for a given field type?
Comment #9
Dave ReidIdeally formatters would have some kind of access() method, but I wonder if for now, we should just add drupal_set_message warning in the formatter settings form if a view controller isn't available.
Comment #10
Dave ReidAnother great use case for having some kind of access() method is EntityReferenceTaxonomyTermRssFormatter - should only be visible to entity reference fields which have a target type of 'taxonomy_term' and not anything else.
Comment #11
Dave ReidAnother use case: I shouldn't be able to use the core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php formatter if I don't have any responsive image mappings created.
Comment #12
slashrsm CreditAttribution: slashrsm commentedaccess() method would also be useful for any contrib formatter that works on entity_reference but might be limited to a single entity_type. Can definitely see this kind of use-cases in media ecosystem.
Would it also make sense to do the same on field widgets?
Comment #13
BerdirI think so yes.
Attached patch implements the basics to make this work, manually tested by adding a entity reference to a config entity without view builder, rendered entity and the taxonomy thing no longer shows up.
Comment #14
cbr CreditAttribution: cbr commentedWill start writing the missing tests for this patch.
Comment #15
cbr CreditAttribution: cbr commentedCreated the test for the available entity reference field formatters.
Also fixed a bug in the assertFieldSelectOption and getAllOptionsList methods.
assertFieldSelectOption failed to correctly check for identical arrays, instead it was checking for identical sort() return values which were both true most of the time.
getAllOptionsList failed to correctly return all the option if the select element was using option groups, now loops trough all items in all option groups.
Comment #17
slashrsm CreditAttribution: slashrsm commentedComment #18
slashrsm CreditAttribution: slashrsm commentedmissing .
This reads a bit strange. What about "Create entity reference field with X as a target."?
Not displaying fields but display configuration.
Missing .
Comment #19
cbr CreditAttribution: cbr commentedModified comments & Added tests for field widgets.
Drupal username for attribution:
https://www.drupal.org/u/arla
Comment #20
BerdirI suggest you add a reference here to the test where this is used for. like See/Used in full\class\name::testMethod().
Explain here why you are creating this, "Creates a new field that can not be used with the multiple formatter. And add a back-reference to the code in the widget that checks for it.
Comment #21
slashrsm CreditAttribution: slashrsm commentedComment #22
cbr CreditAttribution: cbr commentedChanged a few comments and made references back and forward to and from the test field widget class.
Comment #25
yched CreditAttribution: yched commentedThis was the main reason I argued for "one separate entity_ref field type for each target entity type" instead of "one single entity_ref field, whith target type as a setting" in #1801304: Add Entity reference field : granularity of applicable formatters and widgets.
We sure can add an API for dynamic eligibility at runtime, but the pitfalls are (from memory from the discussion back then, there might be others raised in the issue over there) :
- How do we make that predictable for site admins ?
"Why is this formatter not available on this field while it is for that other field ? What do I have to do to make that formatter available ?"
The current solution gives no feedback and only lets site admins click and guess.
- What if a display has been configured for a formatter that was eligible, and the field is then modified so that the formatter is no longer eligible ? What's the good, non confusing, predictable behavior for that ?
(Widget|Formatter)PluginManager::getInstance() implement a fallback to the 'default formatter' for the field type if the configured plugin has been uninstalled, do we do the same ? That would imply that the 'default formatter' needs to always be eligible, how do we enforce that ?
[min-rant]
The current model for widgets / formatters, which provided a structuring stable ground since CCK D4.7, is "formatters are eligible per field type, model you field types accordingly". The decision in #1801304: Add Entity reference field was "meh, we'll figure it out". So let's :-p.
[/mini-rant]
Comment #26
BerdirWhile I agree that there are some pitfalls with this, per-target-type entity reference fields would a) be waaaay to many by now, with the possibility to have config entity references and b) would not really solve the problem here, we wouldn't want to hardcode the list of entity types that have a view builder?
I don't think the problems you mentioned are that problematic, the widgets/formatters would be responsible themself to have logic that makes sense to users, the specific examples we have are on the target_type, which is not something you can change and it makes more sense to me to not have the option "Rendered entity" for something like a filter format reference instead of having it and it then resulting in errors/not doing anything.
Comment #27
yched CreditAttribution: yched commentedWould be determined at runtime in hook_field_formatter_info_alter().
But anyway, yeah, I'm *not* saying we should go back to "one entity_ref field type per target entity type" at this point :-)
Sure, that happens to be true for specific the use case we're discussing here; but we're introducing a generic API that can be used for many other cases. I'm trying to point the fallbacks of the general concept.
Comment #28
yched CreditAttribution: yched commentedOops, unfortunate typo - I'm *not* saying we should go back to "one entity_ref field type per target entity type" at this point :-p
Fixed the previous post.
Comment #29
BerdirSure, but that is true for many generic concepts, like most alter hooks we have ;) But what can we do about this other than document it with "Don't be evil."? I'm fine with adding something like "The default widget/formatter of a given field_type must always be available." And we currently only apply this to the UI. We either need a generic concept or hook_form_alter() and I really don't want to go there :)
Comment #30
yched CreditAttribution: yched commentedI don't think I get the analogy. hook_field_formatter_info_alter() lets you change the fields types for which a formatter is eligible, but doesn't change the fact that eligibility is currently defined purely by the field type, and is thus not "sometimes yes sometimes no, for reasons that will remain untold" ?
Not sure I get the end of the comment either :-)
Other than that, the current patch keeps the 'field_types' entry in the annotations as a 1st eligibility criteria, and adds a new, more granular step as a static method on top of that. Is that really what we want ? Shouldn't we go with only a method then ? Other than history, is there a reason to go with a 2-level eligibility system ?
Comment #31
BerdirSorry, with the first part, I meant that without an API to do this in a generic way, entity_reference.module would have to implement hook_form_alter() on the field_ui manage display form to hide the options that shouldn't be there for this case.
And with the alter hooks, yes you can't make it dynamic, but you can do enough crazy things with most of our alter hooks (like removing or changing a default formatter for a field type). My point was that we already trust developers enough to give them alter hooks, so I don't see the difference to this.
Comment #32
yched CreditAttribution: yched commentedhook_form_alter(): ah, right - yes, agreed that we don't want that.
Comment #33
ArlaRerolled last patch to current tip of core.
Comment #34
Wim LeersHigh-level feedback: I also encountered this problem while working on the various "[entity type] cache tags" test classes (all subclasses of
EntityCacheTagsTestBase
). I fixed all of the content entity types in the process (IIRC) that should have a view builder, and added conditional logic to not render the entity but link to the entity if the entity if it doesn't have a view builder (for some entity types it simply does not make sense), but this indeed should be fixed in a generic way.This could theoretically have interesting consequences for Quick Edit, but since this relies only on the field definition, not on the values of a field instance, this is fine.
Nitpicks:
Extraneous newlines.
Strange things here: double newline, incorrect indentation (unless this is a new standard), consecutive
//
-style comments that possibly should be/* … */
, a missing period.s/formatter/formatters/
80 cols.
Extraneous newline.
Comment #35
ArlaFixed the nitpicks :)
Comment #37
BerdirThis is randomMachineName() now..
Comment #38
ArlaChanged to randomMachineName().
Comment #39
slashrsm CreditAttribution: slashrsm commentedLooks good. Found a typo.
s/data/date
Comment #40
ArlaFixed #39.
Comment #41
slashrsm CreditAttribution: slashrsm commentedComment #42
swentel CreditAttribution: swentel commentedThis bothers me as well a little bit, but it probably shouldn't hold up this patch.
Comment #43
alexpottCommitted b5c736d and pushed to 8.0.x. Thanks!
Removed the usued use on commit - this class had changed it's name to BaseFieldDefinition but fortunately neither that not FieldDefinition is actually being used in DisplayOverviewBase.
Comment #45
yched CreditAttribution: yched commented(Widget|Formatter)PluginManager::getInstance() needs to fallback to the 'default' widget/formatter if the currently configured formatter is not "applicable" anymore.
Comment #46
ArlaHere are some tests for that. They will fail until the actual fallback is implemented.
Comment #47
ArlaI corrected the tests, and also wrote a quick fix that seems to pass. It probably needs improvement.
Comment #50
ArlaPassing ManagerDisplayTest now. Also updated some comments. Still not sure if the added if-conditions in *PluginManager are reasonable.
Comment #52
ArlaOops, regenerating patch. The interdiff was correct, though.
Comment #53
slashrsm CreditAttribution: slashrsm commentedLooks good.
Comment #54
alexpottCommitted c2a74e6 and pushed to 8.0.x. Thanks!
Comment #56
yched CreditAttribution: yched commentedThanks @Arla, the committed followup looks good to me too.
Comment #58
Gábor HojtsyFix media tag.