Problem/Motivation
As discussed for the beta blockers #2116363: Unified repository of field definitions (cache + API) and #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions we need to bring the concept of storage fields (aka "FieldConfig" of configurable fields) to the Entity Field API. Thus, marking as beta-blocker-blocker.
Proposed resolution
The plan is to introduce FieldStorageDefinitionInterface
and have FieldDefinitionInterface
extend it. Then FieldConfig
should implement StorageFieldDefinitionInterface
, but stop implementing FieldDefinitionInterface
as it's lie. For the few situations where widgets need to use FieldConfig as fully-fledged fields we can provide a decorator class which contains the dumb-default implementations for remaining methods (name to be found). (Could be postponed to another issue if turns out be hard.)
Finally, the FieldConfig class should receive (yet another) rename to match the new interface name, i.e. become StorageFieldConfig. yched suggested and we agreed that FieldInstance should be FieldConfig then - but as this has high documentation and community term changes we decided to postpone this.
For providing field storage definitions, we need a new hook + getters on the Entity Manager along with hook_entity_field_info(). As there is no use case for providing field storage base fields we do not add a respective methods on entity class for now.
(Replaced the initially suggested term FieldStorageDefinition with StorageFieldDefinition, as suggested in #12)
Remaining tasks
Follow-up: #2228187: Rename FieldConfig to and FieldInstance
#2235675: Decide on moving field cardinality to the FieldDefinitionInterface
User interface changes
-
API changes
FieldItemInterface::propertyDefinitions() receives a definition object implementing StorageFieldDefinitionInterface
instead of a FieldDefinitionInterface
now
FieldItemInterface::schema() receives a definition object implementing StorageFieldDefinitionInterface
instead of a FieldDefinitionInterface
now
Comment | File | Size | Author |
---|---|---|---|
#80 | d8_field_storage_definition.interdiff.txt | 15.55 KB | fago |
#80 | d8_field_storage_definition.patch | 96.86 KB | fago |
#75 | field-storage-definition-2226197-75.patch | 94.86 KB | jessebeach |
#75 | interdiff.txt | 67.38 KB | jessebeach |
#75 | StorageFieldDefinitionInterface-to-FieldStorageDefinitionInterface.txt | 67.04 KB | jessebeach |
Comments
Comment #1
fagook, so here is a first patch. Let's see where it fails.
Where I was not so sure was whether label() and description() should be in the storage field definition, but as Views is going to need it might make sense to do whatever blackmagic it does in the field config class, so I just left it there for now.
Comment #3
fagooh no, #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition got committed. Now we've got conflicts in every field type :-/ Running out of time now, continuing later on. If someone else wants to pick up and help solving conflicts, please do! ;)
Comment #4
jessebeach CreditAttribution: jessebeach commentedSure, I'll reroll.
Comment #5
jessebeach CreditAttribution: jessebeach commentedChanges due to reroll are in the interfdiff.txt.
Comment #7
fagoGreat, thanks!!
Comment #8
fagook, worked more on it and fixed the test fails. It still misses exposing config fields via the hook and test coverage.
Comment #9
fagoComment #10
jessebeach CreditAttribution: jessebeach commentedreviewing...
Comment #11
jessebeach CreditAttribution: jessebeach commentedShould we be checking for instance of
FieldDefinitionInterface
here?Hmmm, this smells a little bit. Is this a backwards-compatibility method? Is there no way to iterate through the
$definition
and invoke getters rather than enumerating them like this?I don't see where these methods went. To another interface?
Ah, here are the methods. Can we use
git mv
here to effect a rename?Why have the instance settings been removed? Or have they been moved somewhere else?
What makes these things different?
Comment #12
fagoThanks, here some comments:
ad 1) We cannot, see the cited todo :)
ad 2) No, as getters/setters do not follow an exact patter (isTranslatable() ...)
ad 4) I'll let Git try to detect the rename for the next re-roll (via -M)
ad 5) They got removed as the field storage (aka field config for configurable fields) does not have instance settings. Previously it was acting like a field definition/instance although it wasn't, so it had to add in the defaults as well. Now as these got cleaned up, it can be removed. The defaults will be added in as soon as a FieldDefinition is created via createFromFieldStorageDefinition() anyway.
ad 6) Views, operating on fields on the entity level has no knowledge of per-bundle field metadata. So for fields put on bundles it can only go with the FieldStorageDefinition being shared across all bundles. The storage field is fine for everything related to storage, but is not suited for the regular display/widget cases which expects to operate on full field definitions. That's why we've got to come up with field definitions based on the storage fields in that special case.
I've noted my patch mixes up the terms "StorageFieldDefinition" and "FieldStorageDefintion". I could see us going with
StorageFieldDefinition
instead, as it avoid potential confusing around FieldStorageDefintion being a plugin definition of a field storage plugin (which existed in d7).Working on it again.
Comment #13
fagoCompleted the EM implementation and added docs for it now. Still missing test coverage to ensure it works as it should.
Comment #14
jessebeach CreditAttribution: jessebeach commentedI cannot figure out the code path to get
Drupal\Core\Entity\EntityManager::getStorageFieldDefinitions()
orDrupal\Core\Entity\EntityManager::buildStorageFieldDefinitions()
to invoke.Do you have a form or page your were working on while developing these methods that I can use as well?
Comment #15
jessebeach CreditAttribution: jessebeach commentedSo, just talked with fago. The two methods I mentioned in #14 are not called. They are API additions that are part of a larger refactoring to unify field definitions: #2116363: Unified repository of field definitions (cache + API).
This patch just needs unit tests for the new methods.
Comment #16
fagoComment #17
fagoFinally managed to debug mocked object calls. So here is a complete patch with test coverage.
Also, renamed
FieldStorageDefinitionInterface
toStorageFieldDefinitionInterface
as suggested in #12.This should be ready then.
Comment #18
fagoFound a docu mistake.
Comment #19
fagoComment #20
fagoComment #21
fagoUpdated the documentation example.
Comment #22
fagoAdded the potential follow-ups #2228187: Rename FieldConfig to and FieldInstance.
Comment #23
XanoIf this is a complex array, it will need some more documentation to describe the array structure.
"once" instead of "one".
Is this
@var \Drupal\Core\Field\FieldStorageDefinitionInterface
?It doesn't return the actual type (a plugin instance), but an ID, so maybe
getTypeId()
?@return mixed[]
.int
instead ofinteger
.string[]
array[]
array[]
array[]
, and the actual structure will need to be documented, or a reference will need to be added to the documentation elsewhere.Same here.
Wrong code example.
Add a
with()
expectation to test that the right module is invoked?Comment #24
plachThis patch is awesome! I think it provides the missing pieces to finally fix #2143291: Clarify handling of field translatability. I could not find much too complain about, aside from what @Xano already noted:
Missing trailing dot
Bogus empty line
Comment #25
BerdirBased on other discussion, this should somehow mention the computed fields use case.
Possibly include with('phpunit') here to verify that it is called with the correct argument? Or is that going to mess with the bas fields? But if it does, then we might be doing something unexpected internally anyway, so possibly use exact order for the mocking? phpunit is also a bit a strange module name, usually we use something like example_module or so, to make it clear what it is.
Comment #26
jessebeach CreditAttribution: jessebeach commentedAssigning this to myself as I go through the feedback. Fago can have it back whenever he wants :)
Comment #27
jessebeach CreditAttribution: jessebeach commentedre:#1, it's an array, keyed by entity type ID. Each key contains an array of
\Drupal\Core\Field\FieldDefinition
. I annotated it like this. Not sure about my docs syntax.Comments #3-#9 were extremely difficult to address because the code examples in the review contain no line context. I made best guesses. For example.
The following comment does not have enough context or explanation. I cannot address it.
This addresses all the comments in #23 except #11, which I will address with feedback from #25.
The refactor in #23-2 (the second 2) is included as a separate diff for easier scanning.
Comment #29
yched CreditAttribution: yched commentedReally not sure about getType() --> getTypeId(), the notion of "field type" is scattered all over the place, comments, docs...
At any rate, it's in no way related to the task here (and adds like 30% to the patch size), so if anything, it should really be a followup.
Comment #30
fagoNot so sure about this either - howsoever this is a pre-existing method, this patch just moves it in a upper interface. Changing it to getTypeId() would be an unrelated API change, thus needs its own issue. I.e., reverted that part (thanks for the separated interdiff!).
Here is an updated patch which also addresses #24 and #25. Also, I improved to docs to clarify that values provided by SFDI (StorageFieldDefinitionInterface) may not changed/altered by the respective bundle fields. Then, I moved isRequired() to FDI as it's defined by field instance. Also moved the field cardinality to FDI, while I've kept isMultiple() in SFDI which is the only thing we need to know for storage.
Comment #31
yched CreditAttribution: yched commentedNot sure I get that part. How can cardinaility be assigned per instance/bundle if isMultiple() is per SFD ?
Cardinality is currently a field-level, cross-bundle property, changing that seems out of scope here (IIRC, this has an impact on Views)
Comment #32
andypostThen it makes sense to file follow-up to convert cardinality into validation constraint for field item list
Comment #33
yched CreditAttribution: yched commented@andypos: regardless of where cardinality lives, I thought we already had a constraint for that ?
Comment #34
fagoyes, we have.
The thinking was that to the storage it does not matter whether it may be exactly X items or not, to the storage only being multiple or not matters.
It's not changed, as it stays that way for configurable fields. Not putting it on SFDI allows other implementations to do it differently though, what I could totally see being useful. How this implementation figures out to decide on isMultiple() for the storage field is up to them, but a developer should have no troubles deciding that.
Regarding Views I shortly checked that, and it seems to use it for providing options for the delta_limit. Not sure it's an issue to not have that, but as the Views integration is for config fields and lives in field.module it's fine to rely on FieldConfigInterface there anyway.
If it's problematic to put cardinality on FDI I'm happy to move it back. I just thought it makes more sense that way as to the storage it shouldn't matter but is more a validation constraint.
Related, I figured current storage respects the cardinality while loading/saving values to limit the loaded/saved values by the configured cardinality, what was quite a WTF to me. Validation should happen before save and storage should give me whatever is stored but do not silently filter values during load/save. So this is something that would have to go then - yes. @yched: Would you agree with that? Else it wouldn't make sense to keep it that way yeah.
Comment #35
yched CreditAttribution: yched commentedFiltering before save is because D7 never ran validation on programmatic saves. Not fully sure whether we're completely confident all saves are validated now ?
Filtering on load is for the case where you change cardinality for a field from N to
re cardinality on SFD / FI : yeah, i'd rather discuss that in a dedicated issue and not sneak this in here :-)
Comment #36
BerdirWe are not, but it's the responsibility of the caller to call validate() if he receives input that needs to be checked. But that's tricky because for example migrate can not validate, as there's no way to deal with validation errors, it has troubles with mock entities and so on.
I thought we validate to not allow to set the cardinality to lower than what you have in the database, but I'm not sure anymore...
Comment #37
yched CreditAttribution: yched commentedNot that I know of. I'm not even sure we'd be able to write that EFQ if we wanted to (find the max delta present in the DB for that field ?)
Comment #38
andypostSuppose storage should be dumb for cardinality because that property could change in definitions.
It's up to validation to limit min|max values via constraint before save
Comment #39
fagoOne can save data that does not validate - it shouldn't be the job of the storage to enforce validation passes?
So some values go automatically away? That sounds quite weird and violates our rule of keeping the users data intact.
Not sure efq can query like that either, but I think validating that when it is changed makes most sense also. As discussed over at #2002180: Entity forms skip validation of fields that are edited without widgets I think that's the general approach we have to take anyway.
Comment #40
yched CreditAttribution: yched commentedWell, that's what D7 does :-) Not sure how it violates user data more that allowing users to delete a field ? Both actions are initiated by the user ?
Aside from forbidding cardinality change (or at least forbidding cardinlity being lowered) once a field has data, I'm not sure how we can prevent that.
Comment #41
fagoTested on D7 - it indeed silently dismisses additional values without telling you anything about it :-( At least the data is not deleted as long as no one edits the entity. But if someone then edits the entity, it deletes any previously entered additional values.
Yeah, imo we should do that. Sounds like something we could spawn as separate issue to help #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions?
Comment #42
tstoecklerRegarding cardinality change we also need to prevent to change from 1 to more than one, as that has storage implications. Not for configurable fields, but for base fields.
Comment #43
andypostCardinality should be used to build a list of formatters and widgets, not sure it affects storage (that always multiple by default, from d7 as I remamber)
Comment #44
plachA change in cardinality for base fields is just one of the changes we will have to prevent, not sure whether we should add a special case for it here.
Comment #45
tstoecklerRe #44: I think it is just the case of 1 -> many or many -> 1 that we have to prevent. I.e. if for some reason I want to restrict the 'roles' field of users to cardinality 10 (or whatever) that's not a schema change.
Comment #46
plachYes, single to multiple or viceversa (just for base fields obviously).
Comment #47
chx CreditAttribution: chx commentedI do not know what is a 'storage field'; it's not evident from the IS; the patch doesn't explain it either; quite probably the name is entirely wrong. Perhaps the field is stored in some way..? but it is not a field on the storage so storageField makes no sense.
Comment #48
yched CreditAttribution: yched commented@chx: yes the current name is totally not ideal - having a name allows code to be written :-), but I really hope we can come up with something better in the end.
To clarify: the "thing" that this patch formalizes, and currently calls StorageFieldDefinition, is what D7 called $field (as opposed to $instance).
- It's unfortunate IMO to call it "storage definition". Only the EntityStorage defines a "storage".
- It is the definition of a named, abstract "data bucket" (mostly, a data schema : name, columns, is it multiple, translatable...), that some actual "fields" on some actual entity types and bundles will use to put their data.
The fact that two fields in different bundles use the same "bucket" (field name == bucket name) means that their data is in the same "place" and can be queried together: e.g. "fetch all nodes created today, whatever the node type").
It's only the EntityStorage that makes a real storage location (columns in an sql table, records in a mongo db...) out of that abstract "bucket definition".
[edit: to be extra clear, this is not a new thing conceptually, it's exactly the current split between $field and $instance. It's just that Entity Field API currently uses the single notion of "FieldDefinition" for both. This patch formalizes an official "API shape" (name & interface) for the former]
So yeah, name...
FieldDataBucketDefinition ? :-(
Comment #49
chx CreditAttribution: chx commentedI thought we used the phrase "configurable fields" as the name for the thing that was $field in Drupal 7...?
Comment #50
BerdirWe did, but the point of this issue is to have a common interface for configurable and base field "storage information".
Comment #51
chx CreditAttribution: chx commentedHuh?
Comment #52
yched CreditAttribution: yched commented@chx: Trying again.
The $field / $instance conceptual split only exists for configurable fields currently, it's a field.module thing, Core/Entity and Core/Field don't know about it and see everything as "FieldDefinitions". That's a lie: a $field is *not* a complete FieldDefinition; you shouldn't create storage tables out of $instances; etc...
This issue is about moving this conceptual split up into the Core Field system.
Comment #53
fago"storage field definition" means "bucket definition" to me. It's not the storage itself, but it defines the a field entity storage we'll have to take care of. So for me the name would work - but I'm happy to go with whatever makes most sense for people.
Comment #54
fagoDiscussed naming a bit on IRC with msonnabaum, berdir and chx. msonnabaum suggested
StorageMappingDefinition
orStorageMapping
what fits good the language of having ODM/ORMs behind. Thoughts?Comment #55
chx CreditAttribution: chx commentedMapping from what to what?
Comment #56
yched CreditAttribution: yched commented- re: StorageFieldDefinition
I read that as : definition of a "storage field"
I kind of see how "storage field" could mean what I called "data bucket" ("abstract field in an abstract storage"),
but IMO it would still be an unfortunate clash with the notions of "base fields, bundle fields, configurable fields" we already have.
In "storage field", it's "field" as in "a database field" (general IT meaning)
In "base fields, bundle fields, configurable fields", it's "field" as in "entity field" (drupalism)
Or "storage field" = the entity (base-, configurable-...)field as seen by the entity storage layer ?
-> "those two entity fields use the same storage field"
The world "field" has always been used with different meaning in different APIs, and we've lived with that. But using it with different meanings in the same API (the *Field* API :-p) feels like a recipe for confusion :-/
- re:StorageMapping
Hmm. Interesting.
Feels a bit disconnected and lacking context though. As @chx points, it's a bit vague about what it maps to what.
Comment #57
fagoThat's how I'd see it.
The mapping from StorageMapping is about mapping the PHP object(s) of a field to the storage, i.e. the database columns. As said, the commonly used terms ORM or ODM use mapping like that - it's just no terminology we've been adopting so far. Although the entity api is our ORM/ODM :-)
I've re-rolled the patch and fixed conflicts and moved field cardinality to the SFDI as per our discussions. I'll open a follow-up for discussing moving it to FDI.
Comment #58
fagoadded #2235675: Decide on moving field cardinality to the FieldDefinitionInterface
Comment #59
jessebeach CreditAttribution: jessebeach commentedIs it even necessary to check if the $entity_type is fieldable? We're getting a list of field "data buckets" that exist outside of any particular entity type, no? At least, nothing in buildStorageFieldDefinitions ever checks the fields against the particular $entity_type.
Comment #60
BerdirIt is necessary, because a) we want avoid calling those functions of non-fieldable (extendable) entity types and b) that really should be checking for a specific entity_type, looks like there's no public API anymore for that other than using the field map and then loading all fields for that?
Comment #61
jessebeach CreditAttribution: jessebeach commentedStorageFieldDefinitionInterface?
This is defined in
StorageFieldDefinitionInterface
even though most code will interact with the constant through aFieldDefinitionInterface
object. In this instance, the code comment knows too much about an interface that implements this one.StorageFieldDefinitionInterface?
StorageFieldDefinitionInterface?
Comment #62
jessebeach CreditAttribution: jessebeach commentedI've started the Change Notice here: https://drupal.org/node/2236285
Comment #63
chx CreditAttribution: chx commentedPersistableField?
Comment #64
tstoecklerSo while I like the "persist" vocabulary, the problem is that - as far as I understand - this is not about some type of field, i.e. a persistable field vs. a computed field etc. but instead this is about that part of the definition that relates to the storage.
That said I wonder if "FieldStorageDefinition" is not a better term. The separation of "StorageFieldDefinition" and "FieldDefinition" sort of implies that there are two different fields, while that is not really true. I.e. there will be a "StorageFieldDefinition" and a "FieldDefinition" for the node.title field.
So, yeah, I would propose "FieldStorageDefinition" or per #63 even "FieldPersistanceDefinition".
Comment #65
fagoNot sure what persistence vs storage buys as here, to me both terms can be used synonymously. As we usually use "storage" I think we should stick with storage, except there is a reason that this is persistence and not storage?
Yes, while for configurable fields there will be a "storage field" and "field definition", whereas still the field definition will have the *complete* definition, i.e. it includes storage information. Still, it's right that "storage field" implies this is a different flavor of fields along with "base field" and "bundle field" - what's not the case.
FieldStorageDefinition
&$EM->getFieldStorageDefinitions()
would work for me as well.Comment #66
amateescu CreditAttribution: amateescu commentedHow about going "back to basics" (and to what everyone is already used to from D7) and have FieldDefinition / FieldInstanceDefinition? That would mean we'd have to rename the current
FieldDefinition
from core toFieldInstanceDefinition
and make this propsedStorageFieldDefinition
be the newFieldDefinition
.IMO, this is what should've happened in #2114707: Allow per-bundle overrides of field definitions, but I had this idea too late and I didn't want to derail that issue.
Comment #67
jessebeach CreditAttribution: jessebeach commentedEven thought fago switched from "FieldStorageDefintion" to "StorageFieldDefinition" in #12, I'd like to suggest we switch back. fago suggests the same in #65
Here is my reason why in addition. This is the current set of "Field" related interfaces.
FieldInstanceConfigInterface
FieldConfigInterface
FieldDefinitionInterface
StorageFieldDefinitionInterface
StorageFieldDefinitionInterface
breaks the pattern. We are creating "field (storage definitions)", not "(storage field) definitions". We have a nice set of classes if we go back toFieldStorageDefintion
FieldInstanceConfigInterface
FieldConfigInterface
FieldDefinitionInterface
FieldStorageDefinitionInterface
re: #66
The concept of 'instance' is employed by configuration fields, but not by base fields, yet the
FieldDefinitionInterface
is used by both. UsingFieldInstanceDefinitionInterface
for base fields would be misapplied.I had to draw this out because the associations were getting a little to multiple for my brain. This is the UML for the relationships proposed in this issue. I've used
FieldStorargeDefinitionInterface
in the diagram.Comment #68
jessebeach CreditAttribution: jessebeach commentedSo, let's break this down. First, here is the UML that represents Drupal 8.x HEAD before this patch.
UML BEFORE
UML AFTER
Let's look at some implications, before and after for classes.
CODE BEFORE
CODE AFTER
After the changes in this patch, field instance configuration objects equate with raw field definition objects, but not with a field configuration object.
Comment #69
amateescu CreditAttribution: amateescu commentedWhen
FieldDefinitionInterface
was introduced, base fields didn't have the concept of "per bundle" overrides. Now that we realised that we do need this feature after all, and since we already have the concept of fields and instances for configurable fields, how is it misapplied when base fields have the same need?Maybe it helps if I say it this way: field and instance definitions would be the same concept for both base and configurable fields, the only difference would be that their properties are stored in code / the configuration system.
Comment #70
jessebeach CreditAttribution: jessebeach commentedamateescu, here is a link to the document I used to create the UML diagrams.
http://www.lucidchart.com/invitations/accept/5346993d-eb4c-4d7d-b9c9-75e...
Can you add another version to it with your proposed name changes? I created a tab in the document called "amateescu's proposal"
Comment #71
yched CreditAttribution: yched commentedOf the options mentioned above, I still prefer the initial FieldStorageDefinition - although I still have the same reservations than expressed in #48.
Not really sure what would be wrong with FieldDataBucket, TBH. In the posts above as well as in various live / hangout discussions, the expression "data bucket" has consistently been the one that made people understand the concept best, so why introduce a different, more ambiguous one ?
Comment #72
jessebeach CreditAttribution: jessebeach commentedI don't have any opposition to
FieldDataBucket
. I like it in fact. It neatly avoids the ambiguity of "Storage" which we already use for controllers in D8.Comment #73
fagoThe definitions have a relationship to storage, so having storage in there is no bad ambiguity to me. It establishes the relationship. FieldDataBucket sounds like it would be a separate storage for itself, but it isn't.
Yeah, but we do not have stand-a-lone storage fields (aka field_config) for non-configurable fields, so it's not exactly the same.
Comment #74
amateescu CreditAttribution: amateescu commentedWhy would we need field_config objects for non-configurable fields?
Maybe it's related to #2144263: Decouple entity field storage from configurable fields? Or maybe I didn't understand what you said at all :)
Comment #75
jessebeach CreditAttribution: jessebeach commentedThis issue is blocking #2116363: Unified repository of field definitions (cache + API). We really need to hold our noses, make a best effort, get something in, and move on. Also, I have no great stake in this issue other than to get us moving and working on followups; the name doesn't really matter to me. Only core devs will have to live with it :)
So, to that end I've taken the various comments into account and changed
StorageFieldDefinitionInterface
toFieldStorageDefinitionInterface
.Justification:
#64
#65
#71:
#73
The interface changed from
FieldStorageDefinitionInterface
toStorageFieldDefinitionInterface
in #12 with the following explanation:Naming the interface
StorageField...
sets up an anti-pattern in D8 whereby a "field" interface starts with the word "storage". I'd rather avoid inconsistencies within a major version than defend against name confusion across major versions.I've included the refactor as its own diff. The only other change I made is to a docblock
to
The constant is declared in
FieldStorageDefinitionInterface
.The test coverage for this new interface looks adequate. It's time to get this in and let postponed work continue, no?
n.b. amateescu, I'm really sorry, but I can't map your recommendation to the existing architecture. I mean mentally, I can't see how the concepts your recommending would fit together. So I'm pushing ahead here with what we've got. We really need to start working on the unified repository of field definitions in order to get the beta unblocked.
Comment #76
jessebeach CreditAttribution: jessebeach commentedComment #77
chx CreditAttribution: chx commentedWhile I can't grasp the full width of the issue and UML diagrams make me run screaming (due to some childhood^Wuniversity trauma) FieldStorageDefintion definitely sounds more sane than StorageFieldDefintion.
Comment #78
tstoecklerIs there a reason this is not happening in buildFieldStorageDefinitions()?
Minor, but we don't have to use the fully-qualified namespace here, which would make this a bit easier to read.
Wouldn't saying "Only content entities are supported be sufficiently clear?
Here we actually do need the fully-qualified namespace per our coding standards.
Typo: FieldDefin*i*tionInterface
It is strange that $entity_type is not used in the
return
line. Shouldn't this beField::fieldInfo()->getFields()[$entity_type->id()]
?It's inherited, so this works, but wouldn't it be more correct to use
FieldStorageDefinition::CARDINALITY_UNLIMITED
?Comment #79
BerdirYes, 6. has already been pointed out above, The method does not have an entity_type argument, but we must filter it somehow.
As this is about *replacing* FieldInfo, I would suggest we do not use it at all but instead switch to an entity query that filters by entity_type or possible does a STARTS_WITH on the ID, because that's something we can optimize in the entity query implementation to be very fast.
Comment #80
fagoThanks for the updates and review. I've continued from #75 and finalized the rename back to FSD - there were many occurrences in the documentation left. I hope I got all of them now.
ad #78:
1. Yes, buildFieldStorageDefinition() covers only the separately cached FSDs.
2,3. ok, changed it that way.
4,5,6,7 fixed - thanks
I've also updated field_entity_field_storage_info() as suggested in #79. Unfortunately, the previous version was broken as it did not filter by entity type. Sadly, the unit test did not cover that as this part was mocked - so I added an integration test for that to field.module.
Updated patch and interdiff attached.
Comment #81
jessebeach CreditAttribution: jessebeach commentedThanks for finding the rest of the storage-field-to-field-storage word swaps. I hadn't thought to search for them as separate words. I've verified that there are no more relevant instances of "storage field" and its various permutations.
The integration test for the fix to
field_entity_field_storage_info()
looks sufficient.This issue is ready to be committed. We've addressed all the review concerns and reached an acceptable consensus on the interface name. This will allow us to unpostpone two more beta blockers.
I've updated the change notice to reflect changes made in #75.
Comment #82
alexpottLooking at this.
Comment #83
alexpottCommitted acbd5fa and pushed to 8.x. Thanks!
Comment #86
fagoOpened the related #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info()