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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Issue tags: +Entity Field API

tagging

tim.plunkett’s picture

+1 to adding EntityStorageControllerInterface::baseFieldDefinitions to an interface somewhere, that was terribly confusing.

fago’s picture

Status: Active » Needs review
FileSize
18.91 KB

ok, 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.

Status: Needs review » Needs work

The last submitted patch, d8_entity_field_definition_moves.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -227,4 +253,87 @@ public function getAdminPath($entity_type, $bundle) {
+      $cid = 'entity_field_definitions:' . $entity_type . ':' . language(LANGUAGE_TYPE_INTERFACE)->langcode;
+      if ($cache = cache()->get($cid)) {

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?

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -227,4 +253,87 @@ public function getAdminPath($entity_type, $bundle) {
+      $cid = 'entity_field_definitions:' . $entity_type . ':' . language(LANGUAGE_TYPE_INTERFACE)->langcode;
+      if ($cache = cache()->get($cid)) {

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?

+++ b/core/modules/path/path.moduleundefined
@@ -275,18 +275,16 @@ function path_data_type_info() {
- * Implements hook_entity_field_info().
+ * Implements hook_taxonomy_term_field_info().
  */
-function path_entity_field_info($entity_type) {

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.

das-peter’s picture

Assigned: Unassigned » das-peter
das-peter’s picture

Status: Needs work » Needs review
das-peter’s picture

I 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.

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?

I tried to figure out where this could happen but couldn't find anything :| The caching mechanism seems to be the same as before.

Status: Needs review » Needs work

The last submitted patch, interdiff-1893820-3-9.diff, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
853 bytes
19.32 KB

Reverted hook_taxonomy_term_field_info() as berdir requested.

fago’s picture

Thanks, improvements look good to me!

yched’s picture

+   * @param string $entity_type
+   *   The entity type to get field definitions for.
+   * @param array $constraints
+   *   An array of entity constraints as used for entities in typed data
+   *   definitions, i.e. an array having an 'EntityType' and optionally a
+   *   'Bundle' key. For example:
+   *   @code
+   *   array(
+   *     'EntityType' => 'node',

The entity type is specified both as a separate argument and in the constraints ?

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -227,4 +279,87 @@ public function getAdminPath($entity_type, $bundle) {
+   * @param string $entity_type
+   *   The entity type to get field definitions for.
+   * @param array $constraints
+   *   An array of entity constraints as used for entities in typed data
+   *   definitions, i.e. an array having an 'EntityType' and optionally a
+   *   'Bundle' key. For example:
+   *   @code
+   *   array(
+   *     'EntityType' => 'node',
+   *     'Bundle' => 'article',
+   *   )
+   *   @endcode

Yes, 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.

fago’s picture

Status: Needs work » Needs review
FileSize
20.43 KB

ok. 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.

fago’s picture

FileSize
6.75 KB

d.o., you lost my interdiff ;)

yched’s picture

+   * @param string $bundle
+   *   (optional) The entity bundle for which to get field definitions. If FALSE
+   *   is passed, no bundle-specific fields are included. Defaults to FALSE.

Why FALSE and not NULL like for any other optional func parameter ?

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -349,17 +342,46 @@ public function getFieldDefinitions($entity_type, array $constraints) {
+  public function getFieldDefinitionsByConstraints($entity_type, array $constraints) {

If 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.

fago’s picture

Why FALSE and not NULL like for any other optional func parameter ?

I 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.

If 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.

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.

Berdir’s picture

Hm. 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 :)

yched’s picture

An 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.

fago’s picture

ok, 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.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -39,21 +42,70 @@ class EntityManager extends PluginManagerBase {
+   * @var \Drupal\Core\Extension\ModuleHandler
...
+   * @param \Drupal\Core\Extension\ModuleHandler $module_handler
...
+  public function __construct(\Traversable $namespaces, ModuleHandler $module_handler, CacheBackendInterface $cache, LanguageManager $language_manager) {

This should be ModuleHandlerInterface.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -227,4 +279,118 @@ public function getAdminPath($entity_type, $bundle) {
+   *   An array of entity constraints as used for entities in typed data
+   *   definitions, i.e. an array optionally including a 'Bundle' key.
+   *   For example the constraints used by an entity reference could be:
+   *   @code
+   *   array(
+   *     'EntityType' => 'node',
+   *     'Bundle' => 'article',

This still has EntityType in it :)

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -227,4 +279,118 @@ public function getAdminPath($entity_type, $bundle) {
+   *   name. See EntityManager::getFieldDefinitions().

The reference can't be resolved, it's also duplicated right below, so either of them should be left out.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -223,10 +223,8 @@ public function getPropertyDefinition($name) {
+      $bundle = $this->bundle != $this->entityType ? $this->bundle : FALSE;
+      $this->fieldDefinitions = \Drupal::entityManager()->getFieldDefinitions($this->entityType, $bundle);

This should be NULL.

das-peter’s picture

Status: Needs work » Needs review
FileSize
3.38 KB
20.52 KB

Here's a re-roll.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, 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.

fago’s picture

Yes, that looks good. Thanks!

Berdir’s picture

Issue tags: -Entity Field API

Status: Reviewed & tested by the community » Needs work
Issue tags: +Entity Field API

The last submitted patch, d8-entity-definition-1893820-23.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
20.76 KB

re-roll

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -239,7 +239,7 @@ protected function attachPropertyData(array &$entities, $load_revision = FALSE)
-      $field_definition = $this->getFieldDefinitions(array());
+      $field_definition = \Drupal::entityManager()->getFieldDefinitions($this->entityType);

So, 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?

Status: Needs review » Needs work
Issue tags: -Entity Field API

The last submitted patch, d8-entity-definition-1893820-28.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, d8-entity-definition-1893820-28.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Entity Field API
fago’s picture

Status: Needs review » Reviewed & tested by the community

So, 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.

Indeed. 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.

fago’s picture

Issue tags: +sprint
tstoeckler’s picture

I 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?

Berdir’s picture

No, 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.

fago’s picture

Issue tags: -sprint, -Entity Field API

Status: Reviewed & tested by the community » Needs work
Issue tags: +sprint, +Entity Field API

The last submitted patch, d8-entity-definition-1893820-28.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
20.83 KB

re-rolled patch and adapt docs accordingly (typed_data() is gone)

fago’s picture

Note: pushed this to the "definition" branch of the entity sandbox, feel free to use that for further re-rolls.

Status: Needs review » Needs work
Issue tags: -sprint, -Entity Field API

The last submitted patch, d8_definition.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
Issue tags: +sprint, +Entity Field API

#40: d8_definition.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC :)

fago’s picture

Issue tags: -sprint, -Entity Field API

#40: d8_definition.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +sprint, +Entity Field API

The last submitted patch, d8_definition.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
21.17 KB

Re-rolled.

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -301,7 +301,7 @@ protected function attachPropertyData(array &$entities, $revision_id = FALSE) {
-      $field_definition = $this->getFieldDefinitions(array());
+      $field_definition = \Drupal::entityManager()->getFieldDefinitions($this->entityType);

Seems better to inject this

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerInterface.phpundefined
@@ -126,46 +126,16 @@ public function delete(array $entities);
-  public function getFieldDefinitions(array $constraints);
+  public function baseFieldDefinitions();

yay!

fago’s picture

Status: Needs review » Reviewed & tested by the community

>Seems better to inject this
Yes, but unfortunately this is out of scope for now. See #34.

Re-roll looks good, back to RTBC.

fago’s picture

Figured #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).

fago’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +sprint, +Entity Field API

The last submitted patch, entity-manager-field-definitions-1893820-47.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
21.14 KB

Re-rolled.

yched’s picture

re: @fago #50

Figured #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).

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 ?

fago’s picture

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 ?

Well, I've seen that being added in the patch over there, so I assume it's needed ?

  // @todo temporary - Revisit after http://drupal.org/node/1893820.
  public function clearCachedFieldDefinitions() {
    unset($this->controllers['storage']);
    cache()->deleteTags(array('entity_info'));
  }

>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!

yched’s picture

Ah, 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.

Berdir’s picture

The 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.

Status: Needs review » Needs work

The last submitted patch, entity-manager-field-definitions-1893820-53-reroll.patch, failed testing.

fago’s picture

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.

Yep, it makes sense to clear it when the main entity cache is cleared. So I'd be fine with doing it here.

fago’s picture

Status: Needs work » Needs review
yched’s picture

@Berdir: LOL. I'm getting old.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

It 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

curl https://drupal.org/files/entity-manager-field-definitions-1893820-53-reroll.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 21648  100 21648    0     0   8595      0  0:00:02  0:00:02 --:--:--  9351
error: patch failed: core/core.services.yml:150
error: core/core.services.yml: patch does not apply
fago’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
21.2 KB

re-rolled.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Patch is the same, so back to RTBC.

@fago: You need to come up with better patch file names :p

fago’s picture

lol - sometimes something shorter is better ;-)

fago’s picture

#64: d8_definition.patch queued for re-testing.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks everyone.

plach’s picture

Title: Manage entity field definitions in the entity manager » Change notice: Manage entity field definitions in the entity manager
Priority: Normal » Critical
Status: Fixed » Active

We probably need a change notice here.

plach’s picture

Issue tags: +Needs change record
catch’s picture

Tagging.

Berdir’s picture

Title: Change notice: Manage entity field definitions in the entity manager » Manage entity field definitions in the entity manager
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

*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!

Berdir’s picture

Issue tags: -sprint

Removing sprint tag.

Automatically closed -- issue fixed for 2 weeks with no activity.