Updated: Comment #N
Problem/Motivation
Right now, entity fields can optionally only exist on certain bundles but the same fields can not differ between bundles. Configurable field instances can have different settings per bundle and base fields can be different as well, for example the node title label can be configured per node type.
Due to that, Field API currently has a few workarounds that involve manually creating entity objects, which is very slow and a large performance problem.
Proposed resolution
Standardized terminology: base fields are all entity fields that exist independently of the bundle.
Unify Entity::baseFieldDefinitions() and hook_entity_field_info() extend them with bundle variants:
Entity class methods
Changed: $EntityClass::baseFieldDefinitions($entity_type_id) to $EntityClass::baseFieldDefinitions(EntityTypeInterface $entity_type)
New: $EntityClass::bundlefieldDefinitions(EntityTypeInterface $entity_type, $bundle, array $base_field_definitions)
Hooks
Renamed: hook_entity_field_info($entity_type) to hook_entity_base_field_info(EntityTypeInterface $entity_type)
Renamed: hook_entity_field_info_alter(&$info, $entity_type) to hook_entity_base_field_info(&$base_field_definitions, EntityTypeInterface $entity_type)
New: hook_entity_bundle_field_info(EntityTypeInterface $entity_type, $bundle, $base_field_definitions)
New: hook_entity_bundle_field_info_alter(&$field_definitions, EntityTypeInterface $entity_type, $bundle)
The bundle for EntityManager::getFieldDefinitions($entity_type_id, $bundle) is now mandatory. EntityManager::getBaseFieldDefinitions($entity_type_id) has been added to allow to to fetch the base field definitions. This makes it clear that the common use case is to specify the entity type and bundle.
Additionally, the patch implements this for the node title field, which allows to remove the currently existing overrides and various simplification and improvements in multiple places.
Remaining tasks
User interface changes
API changes
Original report by @effulgentsia
EntityManager::getFieldDefinitions() allows for optional fields per bundle, but doesn't allow for a field of the same name to have different definitions per bundle. However, we need to allow for node titles to have potentially different labels per node type, and likely other use cases as well.
https://docs.google.com/document/d/1A5BLd-KmrLnJW88SdLX5HG3Xm7AyinxwXNAG... has notes from Prague about this.
Comment | File | Size | Author |
---|---|---|---|
#146 | d8_field_definition_bundles-2114707-146-interdiff.txt | 4 KB | Berdir |
#146 | d8_field_definition_bundles-2114707-146.patch | 74.91 KB | Berdir |
Comments
Comment #0.0
effulgentsia CreditAttribution: effulgentsia commentedFixed Google Doc link.
Comment #1
yched CreditAttribution: yched commentedThought about this a bit over dinner after the last sprinting day in Vienna :-)
The original idea in Prague was this (copied from the gdoc linked in the OP)
Drawbacks:
- Convoluted DX, easy to mess up (the "clone" part especially is very fragile and DX--)
- This results in a lot of "the same but not completely" copies of the same FieldDefinition objects in cache and in the in-memory repository
I was thinking we could have FieldDefinitionOverride objects that act as shallow objects containing just the overriden properties :
- The methods available on FieldDefinitionOverride would only let you change what can be changed by bundle (e.g setRequired(), addConstraint(), but not setFieldType())
- The caller (EntityManager::getFieldDefinitions()) takes care of $override->setFieldName($field_name) on the results based on the array keys, just like is already done for the results of baseFieldDefinitions
- In our cache, we put the cross-bundle list of FieldDefinitions, + for each bundle the shallow list of shallow FieldDefinitionOverride objects.
- When reading out from the cache at runtime, we transform those into regular FieldDefinition objects by applying the diff. That's what we put in the in memory repository (static cache) and that's what EntityManager::getFieldDefinitions($entity_type, $bundle) returns.
- Possibly, we could be smarter than that and keep the FieldDefinitionOverride objects as wrappers around the actual FieldDefinition objects at runtime - that's as many objects, but less properties duplicated in memory
- According to @fago in Vienna, we might even consider that base fields are present on all bundles of an entity type, and then no need for the ->remove() part.
Thoughts ?
Comment #2
effulgentsia CreditAttribution: effulgentsia commented+1
Comment #3
amateescu CreditAttribution: amateescu commentedThe plan sounds good, but we also need to clear out the relationship between FieldDefinitionOverride and FieldInstance, right?
If we go ahead with this, we'll have Field + FieldInstance on one side and FieldDefinition + FieldDefinitionOverride on the other side, which.. I have to say is less than ideal and feels like one layer too much..
Comment #4
yched CreditAttribution: yched commentedBumping to critical.
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedHm. Here's a crazy thought: is there ever a use case for a FieldDefinitionOverride to be code-defined rather than configuration-defined? I mean, bundles themselves are always configuration-driven (at least in core, I think), so shouldn't bundle-specific field definition overrides also be configuration? If so, then would it make sense for per-bundle overrides to always be FieldInstance objects, and to make it so that those objects can override either configurable or nonconfigurable field definitions?
Comment #6
amateescu CreditAttribution: amateescu commentedThat's an interesting idea. I'm sure @yched will have a better answer but what bugs me about it is that it's quite hard to write a field_instance config object by hand, and it will eventually get down to that as I can't even imagine how we'll provide an UI for this..
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedPer http://buytaert.net/the-next-step-for-drupal-8-is-a-beta, this is a critical API and should be resolved before beta. This hasn't yet been approved as a "beta blocker" by a core committer, so just tagging as a beta target for now, but if we get close to beta without this being done, we should reevaluate whether to make it block a beta.
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedmsonnabaum informed me that the fake entity creation in EntityDisplayBase::getFieldDefinition() is currently causing a sizable performance regression and obscuring the ability to profile for other performance problems. Given that, it would be great to get this solved as quickly as possible, even if a follow up is needed to optimize DX after that.
Comment #9
fagoComment #10
BerdirYep, I can confirm that that function (or actually _field_create_entity_from_ids()) is extremely slow, EntityFieldTest calls it 32 times, taking 4.5s which is 20% of the whole test run. The entity factory will help, but this really needs to happen :)
Comment #11
BerdirAs discussed in the meeting today, we will try to get started here by changing the data structures/cache/hooks to be per bundle, without touching the override part. That will hopefully allow us to put field instances in there and unblock most things, then we can introduce overrides later with minimal API changes. At least that's the plan.
Will work on this asap.
Comment #12
BerdirComment #13
BerdirThis basically works without the per bundle override method as mentioned above.
Something seems to be not quite right yet, getting a fail in EntityTranslationTest. Let's see how the other things are looking.
Note that there's a slight behavior change that's actually due to a bug in the old field_entity_field_info() implementation, see #1919468: EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles, it incorrectly assumed that if $bundle == $entity_type, the entity type then doesn't support bundles and all fields are for all bundles. So it didn't pass in the bundle to getFieldDefinitions() in that case but then got it back in that function. While we could restore something similar to the previous behavior if an entity type only has a single bundle, I think this is the correct behavior as long as we don't have an explicit information if an entity type supports bundles or not.
Comment #14
BerdirComment #16
yched CreditAttribution: yched commentedBerdir++
Minor remarks for starters
If the list of fields by bundle is statically cached in the entity manager, do we need to cache it in the Entity as well ?
We're in the if (!$bundle) case, so passing the $bundle param is a bit misleading.
s/it/them/ ;-) (same below)
Comment #17
Berdir2. I could alternatively pass NULL through explicitly, but we do need to pass that value through somehow :)
Comment #18
yched CreditAttribution: yched commentedMore substantially:
Yeah, this now runs on FieldInstances, and calling setTranslatable() breaks, setTranslatable() is not part of FieldDefinitionInterface (FieldDefinition has no setters - #2143297: setters on FieldDefinitionInterface)
Not sure how we easily fix that. I think we currently misuse isTranslatable() for two different things:
- "does it support translatability" - that's always true for configurable fields, and is defined by the code that provides the field for base fields
- "is it actually translatable" - that's configuration, held by e_t.module, and it's that one that e_t tries to set with setTranslatable() here, and that the whole Entity translation API runtime code then checks with isTranslatable(). This one can be per $instance.
So it looks like the setTranslatable() / isTranslatable() pair we have right currently is the second one, it could be part of FieldDefinitionInterface, but then we need separate methods for the first one.
Comment #19
yched CreditAttribution: yched commentedrelated to that dual isTranslatable() meaning : #2143291: Clarify handling of field translatability
Comment #20
yched CreditAttribution: yched commentedre #17
Why can't the $bundle param in buildFieldDefinitions() be marked optional ? De facto, it is, no ?
Comment #21
yched CreditAttribution: yched commentedLooked into the OptionsFieldTest fails:
in OptionsFieldTest::testUpdateAllowedValues()
The $field->save() triggers field_info_cache_clear() / entity_info_cache_clear(), yet the entity_create() then creates FieldItem objects whith a wrong, stale field definition.
ContentEntityBase::getTranslatedField() calls \Drupal::typedData()->getPropertyInstance() which seems to simply reuse an existing prototype.
Comment #22
fagoGetting the field definition is on the critical path when accessing any field value, so bailing out to the EM + another method is probably too much here. At least this would need benchmarks.
as #20: Yeah, I'd vote for both - passing an explicit NULL + make it optional - that puts the important fact that $bundle can be NULL into developers' face.
yeah, this bugs us repeatedly - I think we finally need to do a proper solution here. I'd prefer to get away without yet another method that will confuse people. So maybe it's enough to differentiate between FALSE and NULL ? Aka, NULL means there is translation support but it's not enabled and FALSE | TRUE that there is translation support and its enabled/disabled ?
Comment #23
yched CreditAttribution: yched commentedReally unsure about that. Using the same getter / setter for different information at different moments in the lifecycle of a field definition is inherently broken IMO.
See #2018685-26: Remove field_is_translatable(). Right now, if I call $field_definition->isTranslatable(), I get a different result depending on where I got $field_definition from and whether e_t has kicked in and made its alters or not. That's utterly confusing.
We shouldn't try to be confusingly smart and magic IMO. If those are two separate informations provided by different sources (the field definition / e_t configuration), they should be accessed and set separately, not stacked onto each other.
Comment #24
fagoI'm not suggesting to do anything magic, I'm suggesting to go with assigning semantics to each of NULL, FALSE and TRUE instead of just having FALSE and TRUE but in two methods. But whether that's feasible is how well the semantics suit their values, i.e. the meaning makes sense for a value?
Comment #25
plachThe main reason for assigning two different semantics to the
'translatable'
key was that we had no bundle-level support at the time. Since we are going to introduce it here, I'd suggest to follow the same pattern we introduced for entity type/bundle info: in that case the'translatable'
key at entity-type level means that the entity type supports translation, while the bundle-level'translatable'
key means that the specific bundle it refers to is enabled for translation. ET alters the latter based on its configuration data, and would do the same with bundle-level field info.This would be very consistent and shouldn't impose an additional DX burden, since people will already need to understand this for entity-type/bundle info.
If we go this way, in #2018685: Remove field_is_translatable() we will have to use bundle-level field info.
Comment #26
fagoOne thing that came to my mind was the question of whether config fields should be still implementing the field definition interface once this is done. I mean we've been implementing it to make it possible to put the config fields under the entity field umbrella, but conceptually it's the field instance which is a complete field definition only.
So once we can put field instances as the field definitions into entity field info repository I guess there is no need left for config fields implementing this interface. Instead, we might want to come up with a separate interface for the notion of a "incomplete field definition that is shared across all different field definitions across bundles". yched and me have been using the term "FieldStorageDefinition" while discussing this in Vienna.
Comment #27
BerdirFieldStorageDefinition would then be used for the field stuff on the storage controller once that becomes not-config/custom-field specific, schema and so on?
Comment #28
fagoyes, that interface would be used to replace FieldInterface at onFieldCreate()/update/delete then
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedDrupal\field\Plugin\views\field\Field::buildOptionsForm() and possibly other places needs to get a formatter for a $field rather than a $instance. Those kinds of use cases was the initial thinking for having $field implement the same FieldDefinitionInterface as $instance. How would you need to adjust formatters and/or Views if you want to split the interface?
Comment #30
yched CreditAttribution: yched commentedThe discussion of FieldStorageDefinition for cross-bundle definitions came from the fact that the "unified FieldDefinitionInterface that works for both $field & $instance" is basically a lie (or "convenient abstraction"), that was introduced because back then, unlike "D7 Field API" fields, ENtityNG fields didn't intend to support the notion of by-bundle variants.
Now they do have this need, and we're trying to find a way to make that happen on top the lie we introduced under the opposite assumptions. Which feels particularly backwards :-)
Everything has by bundle variants, yet we work on a definition format that claims variants don't exist.
That abstraction / lie bites us in more and more places:
- Here with setTranslatable()
- More generally when when it comes to defining setters for FieldDefinitionInterface - we currentlly don't have any, even thouh we do have a hook were modules are supposed to alter FieldDefinitionInterface objects :-).
- #2144327: Make all field types provide a schema() has FieldItem::schema() receive a FieldDefinitionInterace. This means field types authors can write storage schemas that vary by bundle for the same field, which is inherently wrong. This is exactly what the $field / $instance dichotomy in D7 was there to avoid.
Something that works on a FieldDefinitionInterface alone doesn't know in which context it is (cross bundle or bundle specific ?), and thus doesn't know what is legit todo with it, or to what extent the informations it gets from it are workable (is the the real value, or just a default while the actual values will vary on each actual bundle ?)
So yeah, the @fago / @yched discussion in Vienna was about reintroducing a split for "cross-bundle" vs bundle-specific". We fried our brains before reaching actual conclusions or an actionable plan, though :-/.
Comment #31
yched CreditAttribution: yched commentedConfigurable fields inherently have that "cross-bundle" vs "bundle-specific" split.
What we stumbled upon in Vienna was that we need to preserve the following critical DX point for base fields:
You shouldn't be forced to declare both
a) in one method a list of bundle-agnostic FieldStorageDefinitions
b) then in another method a list of actual FieldDefinitions for each bundle.
But I'm thinking that maybe the override model described in #1 might adress that: (actual class names to be enhanced, of course)
- you specify a list of definitions that are "full definitions", but only actually implement FieldStorageDefinitionInterface, and only get passed around as such.
- then you optionally specify a "by-bundle" list of bundle-specific Override objects, that are wrappers with just the overriden properties (possibly none), and that *do* implement FieldDefinitionInterface. As mentioned above, the "empty override" case can be made implicit and automatic for less verbosity.
--> FieldStorageDefinitionInterface is only about what is common across bundles,
FieldDefinitionInterface is only about what is true for a given bundle,
which stays true to our actual conceptual model (i.e "not a lie").
Comment #32
effulgentsia CreditAttribution: effulgentsia commented#30 and #31 make sense, and I see that as we expand FieldDefinitionInterface, there are some things that really have no logical meaning in $field. But I'm still unclear on what formatters and widgets are supposed to act on. Because of Views, they need to be able to act on something regardless of whether it's per-bundle or cross-bundle. Currently, they act on FieldDefinitionInterface. I don't think it makes sense to change that to FieldStorageDefinitionInterface. Do we need yet some other new interface that both $field and $instance can implement, but that's neither FieldDefinitionInterface nor FieldStorageDefinitionInterface? Especially, since if we're adding setters to these interfaces, and formatters and widgets have no business invoking setters.
[EDIT: and by "act on", I mean methods that don't receive a $items, like settingsForm() and settingsSummary(), because for methods that do receive a $items, those are always bound to a bundle.]
Comment #33
BerdirFixing a lot of test fails with two changes:
- Allow to clear the prototype cache in the typed data manager so that injected definitions there are updated. We might want to move this to a separate method, as clearCachedDefinitions() clears way more than we're interested in I think.
- Stupid name clash in content_translation_entity_field_info() altered away all field definitions if content_translation.module was enabled. That caused most of the crazy test fails there, leaving us only with the actual problem: Fatal error: Call to undefined method Drupal\field\Entity\FieldInstance::setTranslatable(). That function certainly needs more work, we need to be specific about the bundle we're handling now, although I'm not sure how to deal with the no-bundle case. Maybe ignore? No idea.
Comment #34
yched CreditAttribution: yched commentedlol @ the content_translation_entity_field_info_alter() interdiff :-)
It's already present 3 lines above ? Is this about the ordering then ?
Comment #35
BerdirMove on, there's nothing to see in that interdiff ;)
Oh, didn't even see that existing call. No the reason it works now is that I added the override in TypedDataManager and reset $this->prototypes. Obviously not necessary to call it twice.
Comment #37
BerdirNot a big fan of having a NULL/TRUE/FALSE tristate. While we could manage it like this internally (although I'd prefer constants or two separate properties), I think it would be much better DX to have to separate methods, NULL/FALSE having a different meaning means that we have to type safe comparisons?
Comment #38
fagoSummarizing, I see the following requirements:
To address all that requirements I think an override approach as in #1 works, however we should make sure that our objects honestly implement interfaces and do not lie to us. Thus, if an object says its complete, it really should be a proper and working field definition *plus* the other way round.
I think that would be problematic for base fields as it would make them lie - if it's a complete, working field definition it should implement the respective interface.
So here is how I could see it working:
I'm not so sure yet about the DX of providing field storage definitions, but it's probably best to just define them along side with full field definitions on the per entity type level. This shows nicely that they share the same namespace, and we can easily differentiate afterwards by checking the interface and so implement two different repository methods that return a) only fully field definitions and b) all storage definitions. Full field definitions should extend the field storage definition Interface such that they are returned in case b) also, i.e. you'd get a complete list of storage fields, where some are complete field definitions and some aren't.
Comment #39
effulgentsia CreditAttribution: effulgentsia commentedSorry guys. I've been on vacation, so have only been able to intermittently follow / comment on this.
What we have is not incomplete at all from the perspective of what Views needs to do: interact with formatters/widgets. FieldDefinitionInterface was introduced initially as the stuff that formatters and widgets need to know about a field (see #1950632-75: Create a FieldDefinitionInterface and use it for formatters and widgets). Now #30 mentions setters that are needed in this issue, and storage-related getters needed in #2144327: Make all field types provide a schema(). It looks like #38 is proposing a FieldStorageDefinitionInterface to hold things like those storage getters. If we do that, then what about reconsidering something along the lines of #1950632-75: Create a FieldDefinitionInterface and use it for formatters and widgets and partitioning the full FieldDefinitionInterface into smaller interfaces, so as to allow $field to implement the ones that make sense without needing to implement the ones that would be a lie, but that way, formatters and widgets could receive the appropriate smaller interface that's implemented by both?
Comment #40
yched CreditAttribution: yched commentedWidgets / Formatters' 90% use case is to be used in a bundle-specific context, and thus have a full-fledged $instance / FieldDefinitionInterface to work on.
Crippling them to work only on a "more restricted field definition" (e.g. by contract/interface, cannot access instance settings) to account for the 10% use case where they are used in a non-bundle-specific context seems like a bad tradeoff.
FWIW:
- Although it would be nice, since 4.7 we never got to the point where a widget could be rendered as an exposed view filter. Because it's hard :-)
- So, the only cases where widgets / formatters methods are called in a non-bundle specific context is "configuration form for a formatter used in a View" (even the actual rendering is made on an actual field in an actual entity, with a defined bundle)
- For the case above, Views D7 was using a function called ctools_fields_fake_field_instance() :-) - which is actully very close to what #30 is proposing...
Not claiming that this is super clean, but at least it's not worse than what it was in D7 ?
The big D8 win is "widgets / formatters on base fields" more than "widgets / formatters on $field structures across bundles" ?
Comment #41
yched CreditAttribution: yched commentedRegarding the isTranslatable() / setTranslatable() blocker:
I do find the "same property / different meaning at different points in the code flow" thing to be confusing, in an area that's already fairly convoluted (base field, bundle override, $field, $instance...) :-).
What particularly baffles me currently is this:
- baseFieldDefinition() sets $field_definition->setTranslatable(TRUE/FALSE) - here, this means "supports translation or not"
- e_t_field_info_alter() does setTranslatable() to override with what it has in its own configuration - then, it means "actually translatable".
- To build its configuration UI, e_t admin form does
But the $field_defintion->isTranslatable() it uses here is the one it has *already altered*, since $field_defintion comes from EntityManager::getFieldDefinitions(), where hook_field_info_alter() has already ran.
Maybe this works currently, but frankly this looks a bit incestuous :-)
For configurable fields, this is further mixed up by the fact that $fields still save a useless 'translatable' flag in their YML (useless because e_t holds the real config). We should really stop doing that - #2143291: Clarify handling of field translatability
So yeah, I'd really favor a two flags approach where it's clear who states what and meaning what :-)
- isTranslatable() / setTranslatable($bool) is handy for "translatability actually enabled" :
isTranslatable() is called by all the Entity/Field runtime code,
setTranslatable() would be only called by e_t (or similar).
- supportsTranslation() / setSupportsTranslation() would be for "supports translation" :
supportsTranslation() is called by e_t to build its UI,
setSupportsTranslation() is set by baseFieldDefinitions(), and not needed for config fields (always TRUE).
setTranslatable(TRUE) throws an exception if !supportsTranslation() ?
The only problem is finding a decent name for "setSupportsTranslation()" :-)
Maybe just supportsTranslation($bool = NULL), that acts as both a getter and setter ?
Comment #42
BerdirWhile we discuss this, here's a patch that implements the baseFieldDefinitionsByBundle() part, based on the somewhat ugly but easy to implement and working clone approach. Updated the node title definition to use this.
Comment #44
BerdirThat was exceptionally stupid.
Comment #46
BerdirLooking into those fails.
As discussed in @yched in IRC, the FieldInstance's already have the correct translation setting through their field, we only need to care about base fields there, apparently? Might not be the final fix, but works.
The other one was tricky, the problem is that ConfigFieldItemList didn't call out to the parent to get constraints on the field item list level. Which I think is #2070429: Configurable fields aren't validated against the "required" constraint except in forms. So just changing that will probably cause test fails? We didn't have that problem before becaue the NodeTitle list class extend from FieldItemList.
Comment #47
yched CreditAttribution: yched commentedConfigFieldItemList::getConstraints() not calling its parent might have been originally related to #2070429: Configurable fields aren't validated against the "required" constraint except in forms - avoid "required" constraints being reported by both FAPI and field validation.
But meanwhile the approach that was agreed to fix #2070429: Configurable fields aren't validated against the "required" constraint except in forms is actually already in place in HEAD, just not at the best place currently. - see my comment #21 over there.
So my guess would be that ConfigFieldItemList::getConstraints() might be able to call its parent without triggering bugs/fails now. Let's see.
Comment #48
yched CreditAttribution: yched commentedTestbot seems stuck on #46 :-/
Just reuploading...
Comment #52
BerdirAwesome, you're right :)
This should then fix that test failure. I love it when things start to work as expected and tests point out weird bugs :)
Comment #53
fagoYeah, imo a tristate would work well as we don't have to manually exclude the not possible forth state then. Howsoever, if you think separate methods are better anyway that's fine with me. But I've the feeling this shouldn't be discussed/solved here - couldn't we do a simple class check or so to move on here and discuss on the best DX in its own issue?
Yes, I think that's the regular case as well, so that should be what they receive as interface. As said, for the other case I think it's totally fine to have some sort of helper to provides us with a complete field definition with sane defaults for the incomplete storage definition. It shouldn't be the widget/formatters job to figure out the differences, but the reasonable defaults should be passed to it. But as those defaults are not generally valid configuration, they shouldn't be part of the general object - so an interim helper object/instance seems to be the logical fit to me.
The patch looks already quite good, however baseFieldDefinitionsByBundle() parameter ordering has not been matching the docs. Also, I think we should not alter the definitions by reference but explicitly return the fields by bundle - for two reasons:
1. It makes us easily aware of the overrides. This might be helpful if we want to optimize caching strategy later on and it turns out that we want to cache per entity-type base fields separately, instead of caching them by bundle (what will lead to duplicate definitions in memory).
2. We should aim for consistency between hook_entity_field_info() and the methods on the entity class - modules defining an entity type should use the methods on the entity class instead of the hook.
Regarding that consistency the method and the hook also differ in that the method gets the non-bundle specific fields passed in, but the hook not. Maybe we should split the hook into two as we do for the methods, such that we have
- one hook for getting non-bundle specific fields, mapping to the method
- one hook for getting bundle specific fields, mapping to the method
- one final alter hook
?
Attached patch changes the byBundle method to return the fields instead and tries to improve the related docs a bit, but does not change the hook yet (let's discuss this first). Please check the interdiff.
What seems to weird about the current patch is that I think it never invokes hook_entity_field_info() with $bundle == NULL, does it? It seems to go with the old behaviour of using the entity type instead. But as NULL does make more sense for the "this applies to all bundles case", I guess it should call it with NULL as specified by the interface.
Comment #54
yched CreditAttribution: yched commentedYes, that's actually what @berdir's latest patch does, and it's totally good enough here.
But it's only after wrapping my head around what e_t does with 'translatable' in #2143291: Clarify handling of field translatability that I realized the $instance->setTranslatable() blocker could in fact be worked around that easily - the fact that it took us 30 comments here is IMO a sign that this area is currently really confusing :-/.
#2143291: Clarify handling of field translatability could probably be repurposed for this "clean up $field->translatable / $instance->translatable / isTranslatable() / setTranslatable() flow".
Comment #55
effulgentsia CreditAttribution: effulgentsia commentedFixing one more @todo in HEAD that was waiting on this issue.
Comment #56
effulgentsia CreditAttribution: effulgentsia commentedIs this comment obsolete now?
Seems like this local variable now conflicts with the new $bundle function parameter? My naive assumption is that we should remove the loop, and just use the $bundle passed to the function, but I don't know if we still need this loop for when $bundle is NULL.
Comment #57
effulgentsia CreditAttribution: effulgentsia commentedI don't see where it uses entity type as bundle, do you? Instead, looks to me like the hook gets called with $bundle = NULL when EntityManager::getFieldDefinitions() itself gets called with $bundle = NULL, and it gets called with a non-null $bundle when EntityManager::getFieldDefinitions() is called with a non-null $bundle. But what that means is that the hook, unlike the baseFieldDefinitionsByBundle() method, must return the complete set of fields that module is responsible for for that bundle, even if they're not bundle-specific. E.g., path_entity_field_info() is called once for each bundle, and returns a new FieldDefinition object for the path field each time, even though the contents of that definition doesn't vary by bundle.
That would be one way to achieve consistency. Another would be to remove the baseFieldDefinitionsByBundle() method and instead add a $bundle parameter to baseFieldDefinitions() to make it match the hook. When we discussed this in Prague, the argument against the latter was that it would be memory inefficient to call baseFieldDefinitions() repeatedly for different $bundle and keep getting back new objects containing identical content as ones we already have. But, with the current patch, we have a @todo to figure out better DX for baseFieldDefinitionsByBundle() and an inconsistency between the methods and the hooks. Would it be better instead to have this patch go the memory inefficient route, but not introduce bad DX and an inconsistency, and then have a follow up for optimizing memory and DX?
Comment #58
effulgentsia CreditAttribution: effulgentsia commentedRe #40: seems like the changes you and fago are proposing aren't needed by this issue any more, so I'll wait till we have a new issue created for it to continue that discussion.
Comment #60
effulgentsia CreditAttribution: effulgentsia commented55: d8_field_definition_bundles-2114707-55.patch queued for re-testing.
Comment #61
yched CreditAttribution: yched commentedNot yet, but that would be the scope of #2143291: Clarify handling of field translatability, which I just repurposed as discussed in #54 above
Comment #62
fagoYep, seems reasonable.
The @todo is now limited to how we do the field overrides, the general approach of baseFieldDefinitionsByBundle() would work for me.
The problem with the memory inefficient route is that it would require a rather major API change - or quite some work-a-rounds - in order to make it work memory efficiently. But given how late we are in the cycle we really should avoid 2 iterative API changes in favour of one that cuts it.
Imo, the bad DX is of baseFieldDefinitionsByBundle() is limited to the necessary clone operation. So fixing that would mean replacing the clone operation with e.g. a helper object/class, i.e. it can be improved without changing how baseFieldDefinitionsByBundle() overally works.
So if we can agree on the overall DX of baseFieldDefinitionsByBundle() as it is now, I think aligning the hooks to that would be the better way to move on.
Comment #63
BerdirAgreed on the byBundle() pattern being the better way forward.
About the clone, now that we expect to get altered fields back from the method/hook, would it help to add an alter() method that would essentially hide away the clone? Might be a bit easier to understand, although it's still easy to forget, but I doubt we find a solution that solves that completely.
One idea that I had about enforcing to use that would be to have an internal frozen flag and then the flow in the build method on the entity manager would be:
1. Call base field method
2. Call for all bundles hook
3. Freeze definitions
4. Call by Bundle method
5. Call by bundle
That would allow us to throw an exception if you try to change anything directly in the by bundle methods, stating that you need to call alter() first. It's quite an overhead (in lines of code) though, as we would need to check this in every setter. And certainly something we could discuss/do in a follow-up.
Comment #64
fagoFrom our IRC discussion the proposal would be to have:
1. base field definitions
2. base field definitions by bundle
3. hook
4. hook by bundle
5. alter hook
As discussed, it would make more sense to have two separate alter hooks, i.e. one for 3 and one for 4.
Comment #65
Berdir55: d8_field_definition_bundles-2114707-55.patch queued for re-testing.
Comment #67
BerdirJust a re-roll for now.
Comment #69
BerdirAnd now a version that might install ;)
Comment #70
effulgentsia CreditAttribution: effulgentsia commentedper #64.
Comment #71
andypostNow result is keyed array...
Would be great to have getFielddefinition($entity_type, $bundle, $field_name)
Probably follow-up
Comment #72
BerdirAnother re-roll.
Comment #74
BerdirAnother one, fixing those tests. Want to have a green patch before I try to implement what we discussed.
Comment #77
plachAccording to the PHP docs
$bundle
should be optional.Comment #78
BerdirAnother re-roll and fixing EntityManagerTest.
That one will need more work when we're done, I just dropped two methods for now as the bundle map no longer exists and the constraints stuff will be removed in the typed data issue.
Comment #79
moshe weitzman CreditAttribution: moshe weitzman commentedGreen! Anyone available to review? The perf team has identified this issue as causing slowness so it would be great if we could move quickly. Our notes say "Get rid of creating fake entities (expensive) because base field definition cannot vary by bundle". Hope this issue is the one to fix that.
Comment #80
effulgentsia CreditAttribution: effulgentsia commentedFrom what I can tell, this still doesn't address #64, so back to needs work.
Comment #81
amateescu CreditAttribution: amateescu commentedYep, this is the one. Here's where we remove the need for that fake entity:
I'm going to take a crack at #64 today, but let's get a painful reroll out of the way first. (NR is just for the bot)
Comment #83
amateescu CreditAttribution: amateescu commentedThe patch attached implements #64, with the following order:
I'm not sure if 5. should receive an additional $base_field_definitions parameter like
ContentEntityInterface::baseFieldDefinitionsByBundle
does.Comment #84
BerdirNice work, thanks so much!
Are we sure that we don't need this for by bundle fields?
I think the discussed ideas about a field storage definition interface that is returned in hook_entity_field_info() won't work, because it's still not the same behavior. That would return fields that might exist on any of the bundles (1-n), not all of them, no matter what bundle it is, which is what hook_entity_field_info() is about.
Based on the changed code, we actually have a ENTITY_TYPE_field_info(), so we could simplify this a bit as we touch it. Don't care much, though.
Apart from those minor issues and the asked question about passing the base fields to the by_bundle() hook, the changes look great and I think should address the concerns that were raised earlier. Needs more reviews! :)
Comment #85
fagoThis looks already pretty good. I found #84.4 as well, imho we should fix this to promote the existence of this hooks, but that's no biggie, yes.
The only thing in the patch that still worries me is that we use the default bundle of entity types without bundles (user -> user). Those fields configured for all users won't apply to all users any more once a module introduces different kind of user bundles. I think it would be better to move the configurable fields that the user assigns to the entity type (+default bundle) to $bundle = NULL, so it's properly mapped to be a cross-bundle field?
We should clarify here, that this can be used to add in overrides as you can do in baseFieldDefinitionsByBundle().
Comment #86
yched CreditAttribution: yched commentedNeeds reroll :-)
Comment #87
yched CreditAttribution: yched commentedre @Berdir #84:
1. Agreed, we should probably do that check for bundle fields too - the sample code in hook_entity_field_info_by_bundle() does not do a ->setName() on the field it adds :-p.
This could be moved to a prepareFieldDefinitions() method along with the code about "Ensure all basic fields are not defined as translatable", which is done on base & bundle fields too.
2. Well, that notion of "fields that exist on all bundles" is shaky IMO, since a configurable field might very well be present on all bundles. Why should such a field not be returned by buildFieldDefinitions($entity_type) ? What's the difference between node title and node body if "body" is present on all bundles ?
We had an unresolved discussion with @fago about this in Vienna - when you call buildFieldDefinitions($entity_type), what is the exact question you're asking ? "give me all the *what* ?" And for which kind of use cases is the answer we're currently giving relevant ?
i.e : a good API is clear about what "question" each method method lets you ask and what answer you get. I'd be incapable of explaining what buildFieldDefinitions($entity_type) means as opposed to buildFieldDefinitions($entity_type, $bundle)
But well, leaving that alone here :-)
3. True - also, those ENTITY_TYPE hook variants are not documented in entity.api.php :
hook_ENTITY_TYPE_field_info()
hook_ENTITY_TYPE_field_info_alter()
hook_ENTITY_TYPE_field_info_by_bundle()
hook_ENTITY_TYPE_field_info_by_bundle_alter()
More generally - all of those are used to build cached stuff. Do we really need the ENTITY_TYPE hook variants ?
Review coming next.
Comment #88
yched CreditAttribution: yched commentedYeah, the current clone approach remains a bit painful :-)
Should we open a followup and link it here ?
Looks like in practice EM::$baseFieldDefinitions & EM::$fieldDefinitions are duplicate ?
On cold caches:
- getFieldDefinitions($entity_type) calls buildFieldDefinitions($entity_type),
- buildFieldDefinitions($entity_type) computes the "list of base fields" , puts it in $this->baseFieldDefinitions[$entity_type] and returns it
- getFieldDefinitions($entity_type) then puts that same result in $this->fieldDefinitions[$entity_type]
But it's the same data :-)
It doesn't help IMO that both getFieldDefinitions() & buildFieldDefinitions() operate in two largely different "modes" ($bundle NULL or not NULL). It would be clearer to split buildFieldDefinitions() in two separate methods :
calls buildFieldDefinitions($entity_type),
and caches in $this->baseFieldDefinitions
calls buildFieldDefinitionsByBundle($entity_type, $bundle),
and caches in $this->fieldDefinitionsByBundle
calls getFieldDefinitions($entity_type) as a 1st step (leveraging static & db cache),
and then does the "by bundle" steps on top of that
?
I'd probably even advocate splitting getFieldDefinitions() itself :
calls buildBaseFieldDefinitions($entity_type)
caches in $this->baseFieldDefinitions
calls buildFieldDefinitions($entity_type, $bundle)
caches in $this->fieldDefinitions
The break statement looks wrong. Now that there's only one loop left instead of two nested loops, it exits the foreach ($field_settings), which is not what we want.
Leftover "use Drupal\Core\Field\FieldDefinition;" at the top.
Yay !
The existing @todo about node title in Node::baseFieldDefinitions() can be removed :-)
Minor : do we really need that @see TypedDataManager here and in hook_entity_field_info() ?
Not really related, and lots of @sees already :-)
Minor: The code sample is a bit contradictory : does it do generic checks on $entity_type / $bundle, or is it hardcoded on 'node' / 'article' ?
Maybe we should remove the mymodule_uses_*() metaphor and go with simpler hardcoded logic on specific $entity_type & $bundle in these examples for clarity ?
Comment #89
BerdirRe-roll and started to work on the reviews, thanks!
#85/@fago: I'm aware that you think that, but has nothing to do with this issue. Right now, entity types must have a bundle, we can not change this here. The reason I had to change that call there was that it was wrong and only worked because of an incorrect implementation of field_entity_field_info() as discussed in #1919468: EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles. That had a hardcoded check to make fields base fields when bundle == entity_type. This was wrong and already caused other hidden bugs, for example code in the entity query join stuff that forgot to pass the bundle along and then was missing all configurable fields and it only worked with entity_test due to that wrong implementation. Happy to discuss that in a separate issue, but please please not here .)
#87:
- Moved the setName() call down.
- Yeah, that is still a bit fragile, but a) that's not what the storage interface idea was about as far as I understood it and it wouldn't help with that, and b) implying that a field that exists on every existing bundle then exists on all bundles is problemematic, because adding a new bundle could change that. I've discussed that before with fago in the context of single/no-bundle entities. For example a rule condition that uses such a field would suddenly be invalid if you add one more bundle as the field would "vanish".
- Yeah, I'm considering to drop those hooks too, as they add 4 additional hook invocations per entity type/bundle and need to be documented/handled in unit tests and so on. The advantage they bring is minimal.
#88:
1. Yes, follow-up for that would be nice IMHO, we already have an improve DX issues based on feedback from Dries, which was related to definition classes by type, we could use that to discuss this?
2. Good points. Need to think a bit about this but you're right that they are the same. The current code is based on a step by step refactoring from current HEAD. So leaving this as it is to see if it is still green before making larger changes.
Also slightly changes the meaning of base field, right now, it's IMHO only things defined by the entity class, not fields added by other modules, like the path alias stuff. Not saying that is wrong, just that it would be a change..
3. Removed the break, I think that's what you are saying?
4. Removed.
5. Removed todo
6. Yes, removed. I think the reason it is there is that various documentation on available types and possible definitions lives there, but it shouldn't and we have definition classes now which are self-documenting.
7. Agreed, simplified and removed those example function calls everywhere.
=> ·@todo: removing ENTITY_TYPE hook versions and refactor the get/build field definition methods
Comment #91
fagoad #85/@fago & bundles:
I'd be happy to, but by bringing the change in here it brings the discussion with it ;) Howsoever, I give this more thought and posted a comment for further discussion to #1919468: EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles. Summarizing, I agree this is the right thing to do.
Howsoever, we need clarification on how we properly deal with the no-bundle case, so please read my comment at #1919468: EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles and respond there.
Yeah, I think the entity-type specific hooks are useful and should stay. When doing d8 projects adding custom (computed) fields per entity type will be something often needed imo.
The entity-type specific documentation is another topic. It's mostly missing anyway, e.g. consider all crud hook documentation for all config entities (hook_block_update(), etc.) - it's non-existing. I guess we should open an issue for discussing that. I could see as dropping it in favour of mentioning it at the general hooks only - as we do it with per form-id hooks.
This needs to run before the alter hook.
Comment #92
yched CreditAttribution: yched commentedYeah, this is precisely what I find shaky currently :-).
- "Base fields":
According to the current code = fields defined in [EntityClass]::baseFieldDefinitions() + baseFieldDefinitionsByBundle()
It's not a real notion you can build on, though, since there's no API that returns a list of "base fields", current API always merges with fields added by 3rd party hooks. So even though we refer to it in a lot of discussions, code comments or docs, API wise, the notion of "base fields" doesn't exist.
- fields returned by EM::getFieldDefinitions($entity_type, NULL):
It's still not clear to me what that represents, or what you should use that list for. It's not the list of "fields potentially available on an entity type across bundles", which would be the actual use case for e.g Views or Panels, since those need to include configurable fields (so this API need is unadressed for now, other than by field_info_fields(), which is *only* configurable fields)
It's there to adress Rules use cases, but I've still to hear a clear definition of the concept in generic, non-Rules terms.
- fields returned by EM::getFieldDefinitions($entity_type, $bundle):
The list of fields you'll find in an actual $entity. It's the only clear & solid concept at the moment IMO.
The win of the patch as it stands is to give us that last one, which is currently missing. It's very valid in itself, and lets us clean lots of nasty workarounds throughout current core. So yeah, we probably want to get it in asap & push the conceptual clarifications on the stack above that to #2116363: Unified repository of field definitions (cache + API)...
Comment #93
fago@base-fields:
berdir and me just discussed on IRC before. So far we've used base fields as "the fields which come with an entity type, i.e. across bundle and bundle-specific - but as you say that has no API implication. But furthermore we have across-bundle entity fields which can be added in by other modules. So to avoid confusion with yet another concept/term we've been thinking about defining "base fields" as "fields across bundles" vs bundle-specific fields. With that definition of base fields the path alias field would be a base field as well, defined by path module.
I like this idea as it avoids us having to differentiate between yet another sort of field, we'd have just base-fields and by bundle fields.
It's the list of fields available on entities of this type, regardless of which bundle they are. Going with the meaning of "base fields" as "fields across bundles", it'd give you the base fields of an entity type. :-)
Rules would use it yes, but any other module could use it as well. It's up to the module to decide whether it wants to provide configuration options only for fields that are there for sure or for all possibly there fields. But, yeah adding other options is definitly something for #2116363: Unified repository of field definitions (cache + API).
Comment #94
andypostAt the same time base fields should be used in storage controller for schema-less saving, so they still needed, not sure are the need to alter them
Comment #95
yched CreditAttribution: yched commented@fago' #93 works for me - then the method names and code flow suggested in #88.2 would make sense IMO.
@andypost makes a good point though: does this definition of "base fields" map to storage layout ?
Comment #96
fagoI think for that we should do as discussed in Prague and add a module key to the field definitions; see #2143069: Add getProvider() and hasCustomStorage to FieldDefinitionInterface.
Regarding #88.2, do you mean?
getBaseFieldDefinitions() would be fine to me, I'm not so sure about getFieldDefinitions(). Strictly speaking a base field definition is a field definition also, but you'd not be able to define it Entity::fieldDefinitions()?
Maybe getBaseFieldDefinitions() and getBundleFieldDefinitions() would be an option that makes the relationship clearer?
Along with the hooks hook_entity_base_field_info($entity_type), hook_entity_bundle_field_info($entity_type, $bundle) ?
Not sure, if you think getBaseFieldDefinitions() and getFieldDefinitions() is be clear enough, I'd be ok with that.
Comment #97
yched CreditAttribution: yched commenteda) getBaseFieldDefinitions($entity_type), getFieldDefinitions($entity_type, $bundle)
b) getBaseFieldDefinitions($entity_type), getBundleFieldDefinitions($entity_type, $bundle)
Yeah, not 100% sure either.
I guess my reasons for favoring a) would be:
- I'd rather have "get the list of fields for this $e_t and $bundle" method be the most intuitive / easiest to find, since AFAIK it's going to be by far the most common use case.
- The fact that getFieldDefinitions() has a non-optional $bundle argument makes it clear that this is about "a specific bundle", without needing to get overly verbose in the method name.
- I feel b) gives a wrong impression that there's a notion of "bundle field" next to the notion of "base field".
Also, true, the hooks should probably be named in a way that relates to the method they are tied to, which might be part of the naming equation...
Comment #98
BerdirNeed to read the whole discussion, but a) sounds good to me as well. And yes, the methods on the entity classes and hooks should then probably be re(-named) as possible.
Yeah. And all you save there is a simple if ($entity_type_id == 'node'). Often you will do it for a specific bundle ony anyway, so you will need an if () anyway, and just two instead of one condition. That's not that complicated. This is not the same as e.g. hook_node_presave(), where you can type hint as NodeInterface.
As mentioned, we'd save *4* hook invocations per entity_type/bundle for that additional line. That's imho worth to think about. It was fine when we had 2 + 2 alters including per entity_type, but now we suddenly have 8.
Will try to update the patch tomorrow.
Comment #99
fagoTrue. And yeah, we don't want to create a new notion of "bundle field" so this is a good point for a)
However, it came to my mind that this might lead people to do
But that would result in a per-bundle copy of the field instead of a single field across all bundles?
Not if you are adding a base field ;-) I must say that I'm not concerned by the number of hook invocations, they should be pretty fast, and the result is cached anyway. But yeah, we could still add per entity type hook if it turns out they would be useful later on.
Comment #100
yched CreditAttribution: yched commentedHm, yeah.
- EM::getFieldDefinitions($entity_type, $bundle) is what we want to promote to the outside world.
- [Entity]::fieldDefinitions($entity_type, $bundle) is *not* what we want to promote for entity type providers
Which kind of collides when deciding which one to put forward...
Then maybe we do disconnect naming for the "by bundle" variants between the "outside facing API" and the "internal builders & collectors" ?
[Entity]::baseFieldDefinitions($entity_type)
hook_entity_base_field_info($entity_type) + _alter()
[Entity]::fieldDefinitionsByBundle($entity_type, $bundle)
hook_entity_field_by_bundle_info($entity_type, $bundle) + _alter()
Regarding hook_ENTITY_TYPE_*() hooks:
I'm not too concerned about performance either, which is why I'm not sure the ENTITY_TYPE variants are useful :-)
AFAIK, those more specific variants are used to avoid too many calls to a single generic hook on mission-critical code paths, that would result in a lot of no-ops after "if ($trivial_condition)" checks in lots of implementations.
I don't think that it's a concern here ?
Comment #101
fagoThe naming suggestion of #100 seems good to me!
So, we've the per-type variants for performance and type-hinting, neither is a reason here. Still, I'd say hook_node_base_field_info() has a better DX though, e.g.
I'd see
nicer than
No biggie though.
Comment #102
BerdirI completely agree that it would be nicer. The only question is, is it worth the effort (additional calls, code, documentation, test coverage and unit test mocking).
Especially the last part is uglier than you might think, EntityManagerTest is currently quite ugly to work with, as you need to set up tons of mocked calls (so that it returns an empty array and not NULL, otherwise it results in array_merge($array, NULL) or similar. I had to mess with that here and in #2039435: Convert EntityManager to extend DefaultPluginManager for example, and they're going to conflict quite a bit. And we'd kind of need test coverage for every single of those hooks :)
If it's OK, I'm going to roll a patch tonight without per-entity type hooks and if @fago is OK with it, we can still open a follow-up issue to consider to add them again? I don't want to block this issue on discussing them and the documentation and test coverage gates that we're supposed to have :)
Comment #103
fagoWorks for me - I guess we should rethink how we test and document all of this hooks, but yeah - this really shouldn't postpone this one.
Comment #104
Berdir#100 it is!
Implemented that, dropped the entity_type hooks for now (will open a follow-up asap), updated calls to it. Also had to make setting the bundle in ContentEntityBase::getDataDefinition() non-conditional and EntityDataDefinition call the correct method, to be consistent with the changed behavior.
Comment #105
yched CreditAttribution: yched commentedYay, looks much cleaner IMO.
Can't do a full review atm, just :
It should be empty after that :-)
Comment #108
BerdirAh, but you can never be sure! We should probably do something like this:
:)
Had to grep for the test fails, let's see if I found all of them. Stupid phpunit integration :(
This somehow had an interesting side effect on the Comment Validator Test, previously, the comment_body field was ignored but it is required, I guess it's related to the bundle handling?
Comment #109
fagoThis is missing from getBaseFieldDefinitions().
This shouldn't include the $base_field_definitions in the result. It should only build overrides / per bundle fields and merge upon retrieval, after loading from cache.
If there is more than 1 bundle, adding in the bundle fields of the first bundle is not valid. That's why it has check for 1 bundle.
Unrelated?
That looks weird. Maybe add a comment to explain it.
Unrelated?
Comment #110
yched CreditAttribution: yched commentedre: doc change in field.attach.inc : yeah not strictly related, this just updates a stale phpdoc to reflect the actual current state of the code.
Don't sweat too much on it, field.attach.inc is removed in #2095195: Remove deprecated field_attach_form_*() anyway.
Comment #111
BerdirRe #109:
1. Is it possibe that you reviewed the previous patch? I moved it from byBundle to Base in the last one.
2. Good point, let's see if I got that right.
3. Agreed, I did not mean to remove the == 1. That said, multiple bundles are broken either way ;)
5. Comment added. Not that different from a ('user', 'user').
6. I don't remember the exact reason but I think it was required to fix something.
Comment #112
yched CreditAttribution: yched commentedThe comment is no longer relevant here, then.
Could maybe move in getFieldDefinitions() :
Comment #113
yched CreditAttribution: yched commentedBleh, never mind #112, I guess the comment still kind of applies - invoking $class::baseFieldDefinitionsByBundle() & actually merging the results happen in separate places, so where the "override" actually happens is debatable :-)
Comment #114
BerdirEither way, looks like we definitely arrived in the nitpick phase, so we're hopefully close :)
Left the comment and added a new one to getFieldDefinitions().
Also renamed the baseFieldDefinitionsByBundle() to fieldDefinitionsByBundle() now, missed that one. And renamed $entity_type to $entity_type_id for baseFieldDefinitions(), so everything should be consistent now.
Updating the issue summary, a change notice is next.
Comment #117
tstoecklerCouldn't find anything standing out on a cursory review. The node example does in fact show that the DX could be improved, IMO, but there's a @todo for that, and the current way of doing it is documented well, so I think that's fine.
Found one nitpick:
Missing newline.
Comment #118
sunI can't really comment on the patch, because I'm still not very familiar with the internals yet. (Looks good! ;))
Just one question: Can we rename the hooks to the following?
hook_entity_field_base_info()
hook_entity_field_bundle_info()
For consistency, simplicity, namespacing, and clarity. If the latter clashes with an existing hook, then simply make "field" plural ("fields").
Comment #119
tstoecklerRe #118: I'm all for consistency and namespacing, but I'm not sure whether that's a good pattern here. Right next to each other the two look good, but given that I wouldn't know the purpose already and see "hook_field_bundle_info()" I'd think it was used to declare bundles of fields or something like that.
I'm not necessarily opposed to the suggestion, just wanted to bring that point up, as well.
Comment #120
BerdirHm, not sure about that @stoeckler has a good point.
Also, PhpStorm told me that the LogicException wasn't documented.
Edit: The reason for those test fails were that the changes resulted in a different order of the fields (by bundle fields being first), found the neat little function array_replace() that allows us to keep the order, even if base fields are overridden.
Comment #121
BerdirChange notice prepared: [#2205235]
I think we're pretty much ready to go, apart from the hook name suggestion by @sun. @yched, @fago?
Comment #122
yched CreditAttribution: yched commentedStarted to nitpick on docs, but it turned out to be faster (and friendlier) to do a pass on this myself. Notably tried to unify the formulations between the various methods and hooks - and cleanup "EntityNG vs Field API"-era formulations like "entity field" ;-)
Other than that, my only remaining code remark:
Shouldn't we do as in buildBaseFieldDefinitions() and ensure that the "basic fields" are not made translatable in their "by bundle" version either ? Earlier versions of the patch did that IIRC.
The code could be moved to a helper checkFieldDefinitions() method called from both build*() methods ?
Similarly, the code that adds ->setName() and ->setTargetEntityTypeId() on non-configurable fields could be moved to a prepareFieldDefinitions() method ?
It would be simpler if all of those could happen in one single helper. The translatability check needs to happen after the alter hook, and striclty speaking it's safer to do the setName()/setTargetEntityTypeId() before the alter.
alter implementations receive the $entity_type_id as an arg, and the field names are the array keys, so maybe it's no biggie if they are not always present in the definition object themselves at this stage ?
Comment #123
tstoecklerRe @yched: If we want to do that, we probably want to fix this code, as well. I didn't bring this up earlier because it's not strictly related, but #2143729: Entity definitions miss a language entity key showed that it doesn't work as expected.
$keys
has *keys* of 'id', 'revision', etc. and then an array with an integer key is +'ed onto that. That means that currently the 'langcode' field of an entity can be marked as translatable without the exception being thrown, but a theoretical field with name0
cannot.Comment #124
fagoYes, #109 was based on #104. Too much progress ;)
I had the same thinking as #119 on the rename - it suggests me something else. Therefore, I'd prefer the existing variant.
Great to see #119.1 resolved already. However, the hooks should receive the same parameters as the method, i.e. $base_field_definitions is missing from them. You might want to add an override in the hook - just as in the method.
Generally, I wonder whether we should pass on the $entity_type domain object now? Seems preferable compared to passing around ids.
Nitpicks:
Too long now?
confusing commas?
Comment is too long now.
Alters bundle-specific field definitions. ?
It does not contain the base-fields for the bundle, so you cannot alter them here. The comment would me suggest I can though.
Comment #125
fagoThe issue summary needs an update as it misses the API changes of the hooks.
Comment #126
yched CreditAttribution: yched commentedAdresses :
#124 1. 3. Oops, IIRC at some point function descriptions were allowed to exceed 80 chars if needed, but it seems not anymore.
#124 2. Reworded
Left 4 out as it relates to the points at the beginning of #174, about what the hook receives.
Comment #127
Berdir- Added $base_field_definitions to by_bundle(), alter hooks can only have two context arguments, if we want to pass the base fields to that one as well we need to make an array out of entity_type_id/bundle :(
- Either way, you can only alter the bundle specific fields, changing base fields will have no effect, so going with fag's suggestion for 4.
- Also found another place that we can simplify now. BTW, I did some quick tests on a relatively simple 8.x site that we're experimenting with and we're spending 22% of the whole request in _field_create_from_ids() when displaying a few different nodes (of different types), this definitely has a huge performance impact on real sites.
- In regards to checking key fields (or however we want to call them), I thought about it too, but my argument for not doing it was that it is probably not allowed to do *any* by-bundle changes on those, so not sure we need to revalidate?
Changing to passing $entity_type around sounds interesting, we're changing all affected code already anyway, anyone opposed to doing that? Not sure it really is that useful, as there shouldn't be that many definitions where the fields would depend on the entity type definition? That said, field could possibly check if an entity type is fieldable, would save some calls on entity types which aren't?
Uploading a patch without that, will follow up with one that does the rename.
Comment #128
BerdirWell, let's try it. It does make the patch a bit bigger, due to the use's.
Comment #131
BerdirNice try EntityReferenceAutocompleteTest, attaching a field to an entity that is not fieldable :p
Comment #132
BerdirUpdated issue summary, added the alter hooks.
Comment #133
yched CreditAttribution: yched commentedRegarding names: I agree with @sun #118 that _by_bundle() / ByBundle() is a bit ugly, but I agree with the others that the suggested replacements are not great either.
Contrary to what I wrote in #97 :
Well, that impression is not wrong, there is a notion of "bundle fields", they are the ones who are not "base fields" :), i.e those who are specific to a bundle.
So we have :
- methods/hooks that expose/collect/alter base fields only
- methods/hooks that expose/collect/alter bundle fields only
- EM::getFieldDefinitions() that uses the above to return all (base + bundle) fields merged together, for a given entity type & bundle.
So maybe after all we could go with:
EM::buildBaseFieldDefinitions($entity_type)
[Entity]::baseFieldDefinitions($entity_type)
hook_entity_base_field_info($entity_type)
hook_entity_base_field_info_alter($entity_type)
[Entity]::BundleFieldDefinitions($entity_type, $bundle)
hook_entity_bundle_field_info($entity_type, $bundle)
hook_entity_bundle_field_info_alter($entity_type, $bundle)
?
The above shows that we miss a method on EntityManager that lets outside code get "just the bundle fields for $entity_type + $bundle" (cached), but I'm not sure there's an actual use case, can probably be added in #2116363: Unified repository of field definitions (cache + API) if needed.
Comment #134
BerdirWarning: You have only one free rename left :p
I'm fine with either, however, I'd really love to get this in ASAP and stop renaming things :) So can we agree on something, then I'll roll a patch and then we get it committed ;)
Comment #135
yched CreditAttribution: yched commented@berdir: I know, sorry :-/. Thanks for the nice and humorous phrasing :-)
As an element of excuse for my back and forth dance here, the naming suggested in #133 also builds on recent changes in the code flow (like buildBundleFieldDefinitions() only returning bundle fields, without merging them with base fields yet), previous iterations were not that clear cut. I feel #133 has a good mix of accurately representing what the current code is doing, and keeping naming that is clear, (relatively) friendly, and consistent with the underlying concepts ?
Comment #136
fagoThanks for the updates - improvements look good.
ad #133:
yeah, the way you put it makes sense to me. It also helps to have the term "bundle fields" when talking about non-base fields. Thus, the suggestion in #133 works for me assuming the parameter $base_field_definitions for the bundle fields stays?
Regarding renaming things - yep, I think we should be done here. But we should get started on another front:
#2100343: Remove 'fieldable' key in entity definitions in favour of 'field_ui_base_route'
Comment #137
BerdirOk, here we go :)
Also updated a few variables now that we have the naming part done: base field definitions + bundle field definitions => field definitions of an entity/bundle.
Updated issue summary and change record.
Comment #139
BerdirAw unit tests ;)
Comment #140
yched CreditAttribution: yched commentedThanks a lot @Berdir !
Minor code flow nitpick:
Since the whole "collect bundle fields" stack ($class::bundleFieldDefinitions(), hook_entity_bundle_field_info() + alter()) now receives $base_field_definitions as a param, it would make sense that EM::buildBundleFieldDefinitions() does too :
EM::getFieldDefinitions() fetches getBaseFieldDefinitions() and passes it down the stack to buildBundleFieldDefinitions()
rather than currently both getFieldDefinitions() & its child buildBundleFieldDefinitions() calling getBaseFieldDefinitions() separately.
Not a blocker & can be done in a followup. RTBC if green.
Thanks all !
[edit : fixed messed up method names in the paragraph above]
Comment #141
BerdirLike this?
And yay!!!
Comment #142
yched CreditAttribution: yched commentedAwesome :-)
RTBC++
Comment #144
BerdirUpdated the tests and also updated the cache keys.
Comment #146
BerdirAnd a re-roll, conflicted in content_translation_entity_base_field_info_alter() due to config() -> Drupal::config().
Comment #147
swentel CreditAttribution: swentel commentedGo go !
Comment #148
fagoYep, good changes - let's get in :-)
Comment #149
BerdirComment #150
alexpottCommitted 7a6fb33 and pushed to 8.x. Thanks!
Noticed that fieldDefinitionsByBundle() was still mentioned in comments. Fixed the following on commit.
Comment #151
andypostFiled follow-up #2213605: Rename EntityManager method buildBuildFieldDefinitions to buildBundleFieldDefinitions
Comment #153
Berdir