Problem/Motivation
Field.php
is written against field config entity types, so what we used to call a field in Drupal 7.
Drupal itself evolved to have a generic notion of fields, backed up with field storage definitions.
In order to render for example node.title in views the same way as node.body we have to make Field.php unaware of field config entity types
but rather work with more generic objects.
Critical #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency is postponed on this.
Proposed resolution
Extract out changes from https://www.drupal.org/files/issues/2342045-75.patch
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#41 | interdiff.txt | 722 bytes | Gábor Hojtsy |
#41 | 2407801-41.patch | 56.09 KB | Gábor Hojtsy |
#37 | interdiff.txt | 3.17 KB | Gábor Hojtsy |
#37 | 2407801-37.patch | 55.38 KB | Gábor Hojtsy |
#33 | interdiff.txt | 724 bytes | Gábor Hojtsy |
Comments
Comment #1
dawehnerComment #2
Gábor HojtsyComment #3
Gábor HojtsyDuh, wrong title :D
Comment #4
dawehnerHa, agreed that title is more helpful.
Here is a first version with some tests and some hacks which are required to get it running (See
EntityDisplayBase::getFieldDefinitions()
).Comment #6
dawehnerFixed the failures.
Note: It contains the fix from #2322457: Error in Views Filter Nodes based on Current user that have term reference same in nodes. with unit test coverage. #2322457: Error in Views Filter Nodes based on Current user that have term reference same in nodes. is still totally valid.
Comment #8
dawehnerMeh.
Comment #9
larowlanTagging for review in today's office hours
Comment #10
jibranahh what?
Views field handler or base field handler.
Can we create an issue for that?
So you are telling me that there is some POC in views which you don't know.
Why not field handler?
EOF missing.
Can you stop manually writing views? :P
We have nice config import/export now. :)
Comment #11
jibranManual testing coming up.
Comment #12
jibranPatch no longer apply so reroll.
Comment #13
dawehnerThank you for the re(roll/view).
Well, this is the way how to enable rendering. Sadly
EntityDisplayBase
relies on a enabled rendering of base fields,but views or rather
$entity->field->view()
should be able to requiring this setup.What about using EntityField? This could be less confusing, than the existing solution.
Yeah sorry for thinking in advance :)
Ah finally fixed my IDE for that
Yeah right, its a horrible idea to be able to understand your patch, given that the configured view is part of the test you would have to understand ;)
Comment #14
dawehnerAt that point we need the french superpower.
What we want to do is to render a single base field, no matter whether it has display options set, or not.
Currently
EntityDisplayBase::getFieldDefinitions()
does not allow us to do that.Comment #15
yched CreditAttribution: yched commented@dawehner: french pedant superpower, à votre service ;-)
I saw #4 earlier, and was wondering about how we'd do this too.
So currently base field definitions in Entity::baseFieldDefinition() can use setDisplayOptions() to opt-in for "get rendered by a widget / formatter *in the regular entity forms / entity displays*". If they don't do so, it means they don't want to be displayed, or are displayed using dedicated legacy/custom code.
EntityDisplays are the ones in charge of actually building the formatter output when rendering an entity in a given view mode. Thus, they are the ones that implement this "ignore fields who didn't opt-in" behavior. That way, even if I hand-edit my core.entity_view_display.*.*.*/yml files and force-add fields that don't want to be rendered, they still don't get rendered.
That's fine for entity_view($entity, $view_mode) - the "official" display of an entity in one of the "official" view modes.
But since EntityDisplays are the only ones that know how to run a formatter, they are also used for "non-view mode based" rendering ("render a field using an arbitrary formatter") - typical case being Views.
This is done using throwaway runtime EntityDisplay objects for a fake '_custom' view mode (see EntityViewBuilder::getSingleFieldDisplay()). For those, ignoring base fields that opted out of "display me using a formatter on entity_view($regular_view_mode)" is not right.
The "mythical" plan for optimizing Views rendering in #1867518: Leverage entityDisplay to provide fast rendering for fields still intends to rely on those runtime EntityDisplay objects, just using them more efficiently. So IMO those are here to stay.
So I'm thinking we should formalize a bit more this notion of "EntityDisplay for an actual view mode" vs "EntityDisplay assembled at runtime, using _custom view mode", and have them behave differently :
- for "official view modes", only consider fields whose definition states they "usually" (regular entity_view()) want to be rendered
- for "non-official view modes", render all fields according to the options held in the EntityDisplay
Attached patch does :
- EntityDisplayBase::$mode defaults to '_custom' (could possibly be a class constant instead), so that a Display created without passing an explicit mode is considered a "custom" one,
- EntityViewBuilder::getSingleFieldDisplay() calls EntityDisplayBase::create() without passing 'mode',
- EntityDisplayBase::getFieldDefinitions() filters or not, depending on whether $this->mode is '_custom'.
Comment #16
yched CreditAttribution: yched commentedAlso - would be interesting to have @effulgentsia's thoughts about #15, since he was also involved in the design of "base fields intergration with EntityDisplays" back then.
Comment #17
yched CreditAttribution: yched commentedThis issue has been frenched.
Comment #18
dawehnerThank you a lot @yched!
#2408667: Rename Field.php to EntityField.php
Dropped it
Comment #19
jibranThank you for the fixes and french exposure.
Perfact
Comment #20
YesCT CreditAttribution: YesCT commented#2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency is postponed on this now.
Comment #21
yched CreditAttribution: yched commentedIterating on #15 [edit: but including #18 of course]
- added some comments
- for those "arbitrary runtime EntityDisplays", we should also skip populating defaults in init(). The render will be made with the options explicitly assigned by the caller.
Comment #22
yched CreditAttribution: yched commentedSide note about EntityDisplayBase::getFieldDefinitions() : #2409661: Remove duplicate check for "FieldableEntity" in EntityDisplayBase
Comment #23
jibranSome nits.
Can we change the @var name here? IMHO it is confusing.
I thought we agreed on Views field entity field handler.
Doc update needed?
We should also rename the @var name.
Needs doc update.
LOL
function doc missing.
nit pick can we move it to new line.
Any reason for this?
Are we not going to add any function desc docs for these?
function doc missing.
Class desc missing.
Comment #24
yched CreditAttribution: yched commented@jibran #23.1 : out of scope here, the patch just wraps existing code in an if().
Comment #25
dawehnerAlright
Fixed
Well, its some number.
Alright.
Sure
OH well yeah, previous patches, when we did not had the trait yet.
I'll all some, which adds value, but not pointless stuff.
Well ... its a test, we don't describe that its a test. the @covers part should be enough.
Comment #26
jibranAfter 3 days #11 here is the manual review.
To test after applying the patch I added
$data['node_field_data']['title']['field']['id'] = 'field';
in NodeViewsData rebuild cache and went to admin/structure/views/nojs/handler/content/default/field/titleBefore
We have no field formatter.
After
We have a plain text formatter.
Thank you @dawehner for fixing the review and helping me in manual testing. It is a critical bug so I don't think we need BE here.
Comment #27
Gábor HojtsyYay, that was quick. #2384863: Translation language base field handler should use views field handler, provide unified options will be able to immediately use this once it lands :)
Comment #28
alexpottA few nits.
Can we get an issue to throw an exception and prevent the UI from creating a mode with this id? @dawehner says this is an existing issue so a followup is fine.
In the long run
also... issue?$rkey is no longer used - we can remove this line.
yay! I've been wanting to remove views hard-coding of sql storage for ages.
@todo should have the same issue linked as the @todo above.
At least a small PHPDoc for the class would be nice
A @todo in an added test feels odd. I think this needs more explanation.
Comment #29
yched CreditAttribution: yched commented#28.1 : opened #2410727: Prevent UI from editing / saving EntityDisplays for '_custom' view mode
Comment #30
Gábor HojtsyUpdated the patch for those suggestions except #28.7. Opened #2410779: Entity views data uses 'entity field', naming incompatible with rest of core using 'field_name' for the entity field / field_name todo. The reason of the @todo spotted in #28.7 is not clear to me. That refers to the
$return_as_object = FALSE
argument on fieldAccess().Comment #31
jibranOnly doc fixes so back to RTBC,
Comment #32
Gábor HojtsyAs I wrote #28.7 is not fixed yet.
Comment #33
Gábor HojtsySeems like that second anything() is totally replaceable with FALSE, so I think this was intended as a pre-commit @todo... Works locally.
Comment #34
jibranThanks @Gábor Hojtsy for the fixes.
Comment #35
alexpottSome things I missed.
How come?
two types?
Comment #36
plachA minor thing if this is rerolled:
We can use
$this->entityManager()
here.Sorry if this was already explained, but why is it needed?Edit: sorry, I missed Alex's post.
Comment #37
Gábor Hojtsy@alexpott: fixed schema.
@plach: fixed the non-use of injected entity manager but this method had several global entity manager uses, so those need conversion also then.
@alexpott/@plach: FieldStorageDefinitionInterface does not have an isRequired() method on it, so it should not be expected to work pre-path already. Let's try to add it back and see if it falls down in flames or not.
Comment #39
Gábor HojtsyAnd as expected: PHP Fatal error: Call to undefined method Mock_FieldStorageDefinitionInterface_eab0a1f2::isRequired() in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Field/BaseFieldDefinition.php on line 95. Looking at the interfaces, isRequired() is on the FieldDefinitionInterface level. Why do we expect to be able to call it on a FieldStorageDefinitionInterface?
Comment #40
yched CreditAttribution: yched commentedBaseFieldDefinition::createFromFieldStorageDefinition() calling $storage_definition->isRequired() :
Right, that seems wrong. Wondering why it's only revealed by this patch though ?
Comment #41
Gábor Hojtsy@yched: the patch changes Drupal\views\Plugin\views\field::getFieldDefinition() which is the only place that invokes BaseFieldDefinition::createFromFieldStorageDefinition() to invoke it with a FieldStorageDefinition (as opposed to before this patch it was invoking it with a FieldStorageConfig which does have an isRequired method). FieldStorageConfigInterface does not have isRequired either, so although FieldStorageConfig says @inheritdoc on its isRequired implementation, none of its interfaces mandates to implement that method, it is just there in itself. FieldStorageConfig has a hardwired FALSE return for isRequired() so not taking that value and not passing into setRequired() in BaseFieldDefinition::createFromFieldStorageDefinition() does not practically change anything about the behavior.
Not sure if FieldStorageConfig::isRequired() is expected to be there elsewhere despite it not being on the interface. Opened #2411323: FieldStorageConfig::isRequired should not exist so we are not experimenting with that here.
Should be RTBC as per above, the changes since then are minor.
Comment #42
Wim LeersGreat progress here! Seems like an elegant, fitting solution.
Comment #43
yched CreditAttribution: yched commented@Gábor: Makes sense, thanks. Let's followup in #2411323: FieldStorageConfig::isRequired should not exist
Comment #44
dawehner@Gábor Hojtsy
Thank you for driving this home!
I'd used an early return, but this is just personal opinion
Comment #45
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 71cb7bb and pushed to 8.0.x. Thanks!
Comment #47
Gábor Hojtsy