Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Spin-off from #1893772: Move entity-type specific storage logic into entity classes.
As of new, we've quite a bit of logic in the entity storage controller to retrieve, process and cache entity field defintitions. While the definitions of the entity base fields can stay in the controller, I do not think it should be responsible for managing the field definitions.
Suggestion:
Move EntityStorageControllerInterface::getFieldDefinitions() to the EntityManager and add EntityStorageControllerInterface::baseFieldDefinitions() for fetching base field definitions defined by the entity type.
Comment | File | Size | Author |
---|---|---|---|
#64 | d8_definition.patch | 21.2 KB | fago |
#53 | entity-manager-field-definitions-1893820-53-reroll.patch | 21.14 KB | das-peter |
#47 | entity-manager-field-definitions-1893820-47.patch | 21.17 KB | Berdir |
#40 | d8_definition.patch | 20.83 KB | fago |
#40 | d8_definition.interdiff.txt | 1.01 KB | fago |
Comments
Comment #1
fagotagging
Comment #2
tim.plunkett+1 to adding EntityStorageControllerInterface::baseFieldDefinitions to an interface somewhere, that was terribly confusing.
Comment #3
fagook, rolled a patch for this one. It turned out there were quite some doc and some code references to entity-properties left over there so I fixed those to be entity-fields on the way.
Comment #5
BerdirAs you changed the entity manager to get the module handler injected, we should probably also inject the cache bin?
language() is a bit special, not sure what's going to happen to that exactly?
As you changed the entity manager to get the module handler injected, we should probably also inject the cache bin?
language() is a bit special, not sure what's going to happen to that exactly?
Note this is only temporarily only for terms. It will at least need to be defined for nodes as well, possibly somehow configurable for all entity types that want to support path aliases.
Seems like either the definitions are now persisted for a longer time, not sure if persistent or static or something with cache clearing that worked before doesn't work anymore. Anyway, it would probably make sense to clear them in clearCachedDefinitions() on the entity manager anyway?
Looks great otherwise.
Edit: Removed weird duplicated comments.
Comment #6
das-peter CreditAttribution: das-peter commentedComment #7
das-peter CreditAttribution: das-peter commented#3: d8_entity_field_definition_moves.patch queued for re-testing.
Comment #8
das-peter CreditAttribution: das-peter commentedI tried to incorporate the changes purposed by berdir in #5.
Btw. I wasn't able to reproduce any test-failures, so I hope these were just random test-bot hickups.
I tried to figure out where this could happen but couldn't find anything :| The caching mechanism seems to be the same as before.
Comment #10
das-peter CreditAttribution: das-peter commentedReverted
hook_taxonomy_term_field_info()
as berdir requested.Comment #11
fagoThanks, improvements look good to me!
Comment #12
yched CreditAttribution: yched commentedThe entity type is specified both as a separate argument and in the constraints ?
Comment #13
BerdirYes, EntityType is actually incorrect, because it's not used here. As discussed, we want to move toward something like this:
getFieldDefinitionsByConstraints($constraints)
getFieldDefinitions($entity_type)
getFieldDefinitionsByBundle($entity_type, $bundle)
the latter two would internally just call the first one I guess.
Not sure if it makes sense to already refactor to that here. Probably not but then we should simply remove the stuff about EntityType in $constraints.
Comment #14
fagook. Once we start messing with it anyway here, I figured we can solve it properly now as well.
I've implemented our plan as noted in #13, but without the ByBundle variant. I think having an optional $bundle parameter is ok, see attached patched.
If we'd fix $entity->bundle() to return FALSE if there is no bundle (in another issue), you can just pass that on to the method as $bundle parameter.
Comment #15
fagod.o., you lost my interdiff ;)
Comment #16
yched CreditAttribution: yched commentedWhy FALSE and not NULL like for any other optional func parameter ?
Comment #17
BerdirIf we have byConstraints(), we should not separate $entity_type I think. We should throw an exception if EntityType is not there but it shouldn't be two argument. Because then we can, as discussed, directly pass the constraints of e.g. a entity reference field to that.
Comment #18
fagoI thought that we'd be nice if we fix $entity->bundle() to return FALSE either. But now as you mention it I guess $entity->bundle() should return NULL also. So changed it back to NULL.
I'm not sure. Well, if you have an entity_reference that works. But you could as well do a node_reference type, which then would not necessarily have an entity type in the constraints. But still, passing on constraints to getFieldDefinitions() would make sense. Thus, I think that's a reasonable improvement and documents the required parameter better.
Updated patch attached.
Comment #19
BerdirHm. I think the byConstraints() one should be the one that contains the implementation and have a single $constraints argument and the other one should forward to this. Having documentation about $constraints that talks about EntityType and then just hardcode Bundle and ignore everything else seems just weird :)
Comment #20
yched CreditAttribution: yched commentedAn existing bug with the current StorageController::getFieldDefinitions() is that it acts as a static cache, and there's no way to reset this static cache (e.g when a field / instance gets created). The only way is the drupal_flush_all_caches() sledgehammer.
(I bumped into this in the "field API types as data types" patch).
If moving getFieldDefinitions() to the EntityManager, there should also be a method to clear the caches.
Not sure whether this should be done as part of this patch.
Comment #21
fagook, re-rolled.
Fixed documentation to not refer to constraints when there aren't any + added the static cache clear.
@call-way: As discussed I don't mind really as it's an implementation detail, but I noticed that if we are doing it the other way round we end up building intermediate unnecessary arrays. Because of that I guess it's better to do it this way.
Comment #22
BerdirThis should be ModuleHandlerInterface.
This still has EntityType in it :)
The reference can't be resolved, it's also duplicated right below, so either of them should be left out.
This should be NULL.
Comment #23
das-peter CreditAttribution: das-peter commentedHere's a re-roll.
Comment #24
BerdirOk, let's go with this. I'm not 100% sure about the implementation and which function should call which one but we can get this in and then change the implementation without breaking the interface later.
Comment #25
fagoYes, that looks good. Thanks!
Comment #26
Berdir#23: d8-entity-definition-1893820-23.patch queued for re-testing.
Comment #28
das-peter CreditAttribution: das-peter commentedre-roll
Comment #29
BerdirSo, given that we now inject stuff into storage controllers (even though a lot of things aren't injected yet, e.g. in form controllers), we should inject the entity manager as well, but that will mean that we will run into serialization issues.
Not sure if we can or want to make an exception here?
Comment #31
fago#28: d8-entity-definition-1893820-28.patch queued for re-testing.
Comment #33
Berdir#28: d8-entity-definition-1893820-28.patch queued for re-testing.
Comment #34
fagoIndeed. I think we'll have to postpone fixing that on #2004282: Injected dependencies and serialization hell , thus go with that for now and go over it once again afterwards.
So back to RTBC then.
Comment #35
fagoComment #36
tstoecklerI think the entity manager injecting itself into the storage controller is unneeded complexity in the first place. Can we not just pass the field definitions in the first place?
Comment #37
BerdirNo, because that would mean we would have to load all entity field definitions of all bundles up front or introduce complexity with some sort of intelligent cache array/collection.
Comment #38
fago#28: d8-entity-definition-1893820-28.patch queued for re-testing.
Comment #40
fagore-rolled patch and adapt docs accordingly (typed_data() is gone)
Comment #41
fagoNote: pushed this to the "definition" branch of the entity sandbox, feel free to use that for further re-rolls.
Comment #43
fago#40: d8_definition.patch queued for re-testing.
Comment #44
BerdirBack to RTBC :)
Comment #45
fago#40: d8_definition.patch queued for re-testing.
Comment #47
BerdirRe-rolled.
Comment #48
andypostSeems better to inject this
yay!
Comment #49
fago>Seems better to inject this
Yes, but unfortunately this is out of scope for now. See #34.
Re-roll looks good, back to RTBC.
Comment #50
fagoFigured #1969728: Implement Field API "field types" as TypedData Plugins needs the ability to clear the entity field cache which is introduced here and did not exist before (ouch).
Comment #51
fago#47: entity-manager-field-definitions-1893820-47.patch queued for re-testing.
Comment #53
das-peter CreditAttribution: das-peter commentedRe-rolled.
Comment #54
yched CreditAttribution: yched commentedre: @fago #50
I think this is ultimately related to "merge FieldInfo cache and the getFieldDefinitions() cache somehow".
Meanwhile, well, the patch here introduces a clearCachedFieldDefinitions() method, right ? So I guess field.module just needs to call than when appropriate (for now in field_info_cache_clear() probably ?).
But it seems we need to so that anyway in this patch here, don't we ? I don't think I see how this is related to #1969728: Implement Field API "field types" as TypedData Plugins specifically ?
Comment #55
fagoWell, I've seen that being added in the patch over there, so I assume it's needed ?
>I think this is ultimately related to "merge FieldInfo cache and the getFieldDefinitions() cache somehow".
Not sure how this is related, but yes - we need to do that!
Comment #56
yched CreditAttribution: yched commentedAh, right, I totally forgot I had to add this in #1969728: Implement Field API "field types" as TypedData Plugins.
Here's the corresponding commitlog from the sandbox.
So it seems it was needed in order to fix a test fail in FileFieldRevisionTest (at least those were the fails I was debugging back then, it's possible other tests were affected and fixed at the same time). I'm not sure I dug into figuring out why those failed with the patch and not in HEAD, though.
Anyway - if this patch here adds clearCachedFieldDefinitions(), then we can just remove it from #1969728: Implement Field API "field types" as TypedData Plugins when this one lands...
I'm just a bit surprised that this patch adds the clear method without ever needing to call it. Fine if it's green, I guess.
But clearing the cache in entity_info_cache_clear(), like #1969728: Implement Field API "field types" as TypedData Plugins currently does, sounds reasonable, and maybe it would be best to do it here rather than in a 300k patch that's not strictly related, where it's more likely to raise reviewers eyebrows ?
I could go either way, your call.
Comment #57
BerdirThe cache clear was added as a response to @yched's comment in #20, we discussed that together in Portland :)
I had/have similar failures in test_entity/entity_test, the reason is that for non-NG entity types, the entity field definitions are irrelevant but important for NG entity types, so if you e.g. already build them and then add (another) field, we need to clear them.
Comment #59
fagoYep, it makes sense to clear it when the main entity cache is cleared. So I'd be fine with doing it here.
Comment #60
fago#53: entity-manager-field-definitions-1893820-53-reroll.patch queued for re-testing.
Comment #61
yched CreditAttribution: yched commented@Berdir: LOL. I'm getting old.
Comment #62
BerdirIt probably does, but it's not required by this patch, so let's add that call when we need it. As it only happens in tests, it might be enough to explicitly call the method there?
Back to RTBC.
Comment #63
alexpottNeeds a reroll
Comment #64
fagore-rolled.
Comment #65
BerdirPatch is the same, so back to RTBC.
@fago: You need to come up with better patch file names :p
Comment #66
fagolol - sometimes something shorter is better ;-)
Comment #67
fago#64: d8_definition.patch queued for re-testing.
Comment #68
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks everyone.
Comment #69
plachWe probably need a change notice here.
Comment #70
plachComment #71
catchTagging.
Comment #72
Berdir*Finally* managed to add this to https://drupal.org/node/1806650, and more importantly, write a documentation page about all this at https://drupal.org/node/2078241. Reviews and improvements welcome!
Comment #73
BerdirRemoving sprint tag.