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.

Files: 
CommentFileSizeAuthor
#64 d8_definition.patch21.2 KBfago
PASSED: [[SimpleTest]]: [MySQL] 58,073 pass(es).
[ View ]
#53 entity-manager-field-definitions-1893820-53-reroll.patch21.14 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 57,585 pass(es).
[ View ]
#47 entity-manager-field-definitions-1893820-47.patch21.17 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-manager-field-definitions-1893820-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#40 d8_definition.patch20.83 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_definition_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#40 d8_definition.interdiff.txt1.01 KBfago
#28 d8-entity-definition-1893820-28.patch20.76 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-entity-definition-1893820-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 d8-entity-definition-1893820-23.patch20.52 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-entity-definition-1893820-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 interdiff-1893820-21-23-do-not-test.diff3.38 KBdas-peter
#21 d8_definition.patch20.71 KBfago
PASSED: [[SimpleTest]]: [MySQL] 56,111 pass(es).
[ View ]
#21 d8_definition.interdiff.txt1.85 KBfago
#18 d8_definition.patch20.43 KBfago
PASSED: [[SimpleTest]]: [MySQL] 55,660 pass(es).
[ View ]
#18 d8_definition.interdiff.txt1.89 KBfago
#15 d8_definition.interdiff.txt6.75 KBfago
#14 d8_definition.patch20.43 KBfago
PASSED: [[SimpleTest]]: [MySQL] 55,658 pass(es).
[ View ]
#10 d8-move-entity-field-definition-moves-1893820-10.patch19.32 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 55,876 pass(es).
[ View ]
#10 interdiff-1893820-9-10-do-not-test.diff853 bytesdas-peter
#8 d8-move-entity-field-definition-moves-1893820-9.patch20.15 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 56,742 pass(es).
[ View ]
#8 interdiff-1893820-3-9.diff4.69 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1893820-3-9.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 d8_entity_field_definition_moves.patch18.91 KBfago
PASSED: [[SimpleTest]]: [MySQL] 55,821 pass(es).
[ View ]

Comments

Issue tags:+Entity Field API

tagging

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

Status:Active» Needs review
StatusFileSize
new18.91 KB
PASSED: [[SimpleTest]]: [MySQL] 55,821 pass(es).
[ View ]

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.

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

Assigned:Unassigned» das-peter

Status:Needs work» Needs review

Assigned:das-peter» Unassigned
StatusFileSize
new4.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1893820-3-9.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
new20.15 KB
PASSED: [[SimpleTest]]: [MySQL] 56,742 pass(es).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new853 bytes
new19.32 KB
PASSED: [[SimpleTest]]: [MySQL] 55,876 pass(es).
[ View ]

Reverted hook_taxonomy_term_field_info() as berdir requested.

Thanks, improvements look good to me!

+   * @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 ?

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.

Status:Needs work» Needs review
StatusFileSize
new20.43 KB
PASSED: [[SimpleTest]]: [MySQL] 55,658 pass(es).
[ View ]

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.

StatusFileSize
new6.75 KB

d.o., you lost my interdiff ;)

+   * @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 ?

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

StatusFileSize
new1.89 KB
new20.43 KB
PASSED: [[SimpleTest]]: [MySQL] 55,660 pass(es).
[ View ]

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.

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

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.

StatusFileSize
new1.85 KB
new20.71 KB
PASSED: [[SimpleTest]]: [MySQL] 56,111 pass(es).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new3.38 KB
new20.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-entity-definition-1893820-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's a re-roll.

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.

Yes, that looks good. Thanks!

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.

Status:Needs work» Needs review
StatusFileSize
new20.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-entity-definition-1893820-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

re-roll

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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

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.

Issue tags:+sprint

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?

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.01 KB
new20.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_definition_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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.

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

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

Status:Needs review» Reviewed & tested by the community

Back to RTBC :)

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.

Status:Needs work» Needs review
StatusFileSize
new21.17 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-manager-field-definitions-1893820-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled.

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

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new21.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,585 pass(es).
[ View ]

Re-rolled.

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 ?

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!

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.

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.

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.

Status:Needs work» Needs review

@Berdir: LOL. I'm getting old.

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.

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

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new21.2 KB
PASSED: [[SimpleTest]]: [MySQL] 58,073 pass(es).
[ View ]

re-rolled.

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

lol - sometimes something shorter is better ;-)

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

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks everyone.

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

We probably need a change notice here.

Issue tags:+Needs change record

Tagging.

Title:Change notice: Manage entity field definitions in the entity managerManage 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!

Issue tags:-sprint

Removing sprint tag.

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