Move hook_field_extra_fields() to the entity API

+++ b/core/modules/field/field.info.inc
@@ -117,9 +117,12 @@ function field_info_fields() {
+ * @deprecated As of Drupal 8.0, use
+ *   \Drupal::entityManager()->getBungdleExtraFields() instead.

When do we plan to remove this? We should say either

@deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.

or

@deprecated in Drupal 8.0, will be removed in Drupal 9.0.

Also, that method name doesn't look right.

1. +++ b/core/core.services.yml
   @@ -203,7 +203,7 @@ services:
      entity.manager:
        class: Drupal\Core\Entity\EntityManager
   -    arguments: ['@container.namespaces', '@service_container', '@module_handler', '@cache.cache', '@language_manager', '@string_translation']
   +    arguments: ['@container.namespaces', '@service_container', '@module_handler', '@cache.cache', '@cache.field', '@language_manager', '@string_translation']
        tags:
          - { name: plugin_manager_cache_clear }
   

   Let's just use the existing cache backend.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -475,6 +492,45 @@ public function getAllBundleInfo() {
   +  public function getBundleExtraFields($entity_type, $bundle) {
   

   No need for Bundle here imho, we also have getFieldDefinitions() and not getBundleFieldDefinitions().

3. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -475,6 +492,45 @@ public function getAllBundleInfo() {
   +    $cached = $this->fieldCache->get("field_info:bundle_extra:$langcode:$entity_type:$bundle");
   

   This should be something like entity_bundle_extra_fields:...

4. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -475,6 +492,45 @@ public function getAllBundleInfo() {
   +
   +    // Cache miss: read from hook_entity_extra_fields(). Note: given the current
   +    // shape of the hook, we have no other way than collecting extra fields on
   +    // all bundles.
   

   I'm not sure that note is necessary, no idea what other way there should be.

5. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -475,6 +492,45 @@ public function getAllBundleInfo() {
   +    $extra = $this->moduleHandler->invokeAll('entity_extra_fields');
   

   Wondering if the hook name should be entity_extra_field_info(), for consistenty with base_field_info() and bundle_field_info() ?

6. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -475,6 +492,45 @@ public function getAllBundleInfo() {
   +      'field_info' => TRUE,
   

   Same here, let's make this part of the entity_field_info cache tag.

+++ b/core/modules/system/tests/modules/entity_test/entity_test.module
@@ -196,9 +196,9 @@
 
 /**
- * Implements hook_entity_extra_fields().
+ * Implements hook_entity_extra_field_info().
  */
-function entity_test_entity_extra_fields() {
+function entity_test_entity_extra_field_info() {
   $extra['entity_test']['bundle_with_extra_fields'] = array(
     'display' => array(
       // Note: those extra fields do not currently display anything, they are

I guess this was only used for the removed test and as that now uses unit tests, we no longer need it?

It will be removed in #2167167: Remove field_info_*().

In that case, the comment should read
@deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.

Thanks, patch looks good. Here some remarks:

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -477,6 +484,43 @@ public function getAllBundleInfo() {
   +    $cache_id = "entity_bundle_extra_fields:$langcode:$entity_type:$bundle";
   

   field definitions are cached with a cid using entityType:bundle:langcode - so shouldn't we follow that?

2. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
   @@ -187,6 +187,19 @@ public function getController($entity_type, $controller_type);
   +   * @param string $entity_type
   

   Should be $entity_type_id - and the description as well accordingly.

3. +++ b/core/modules/system/entity.api.php
   @@ -829,3 +831,93 @@ function hook_entity_field_access_alter(array &$grants, array $context) {
   + * Exposes "pseudo-field" components on fieldable entities.
   

   on content entities.

4. +++ b/core/modules/system/entity.api.php
   @@ -829,3 +831,93 @@ function hook_entity_field_access_alter(array &$grants, array $context) {
   + * Fieldable entities or modules that want to have their components supported
   

   Content entities

5. +++ b/core/modules/system/entity.api.php
   @@ -829,3 +831,93 @@ function hook_entity_field_access_alter(array &$grants, array $context) {
   + * in a #pre_render callback added by field_attach_form() and
   

   by the entity display form now, but I guess it's enough to say "by the entity form controller".

6. +++ b/core/modules/system/entity.api.php
   @@ -829,3 +831,93 @@ function hook_entity_field_access_alter(array &$grants, array $context) {
   + * @param $info
   

   @param array $info

Thanks, changes look good except for the following.

+++ b/core/modules/system/entity.api.php
@@ -930,8 +914,9 @@
+ *  The array structure is identical to that of the return value of

Misses one space indentation.

Also, I'm wondering where one can clear the extra field cache? Is this missing now? -> Needs work.

Thought I commented already. You can clear the cache through clearCachedFieldDefinitions(), through the cache tag.

Beta-blocking as a blocker for #2116363: Unified repository of field definitions (cache + API).

+++ b/core/modules/system/tests/modules/entity_test/entity_test.module
@@ -196,9 +196,9 @@ function entity_test_entity_form_mode_info_alter(&$form_modes) {
 
 /**
- * Implements hook_field_extra_fields().
+ * Implements hook_entity_extra_field_info().
  */
-function entity_test_field_extra_fields() {
+function entity_test_entity_extra_field_info() {
   $extra['entity_test']['bundle_with_extra_fields'] = array(
     'display' => array(

This is still in it?

Also, @fago correctly pointed out that while we clear the persistent cache, we need to unset the "static" static aka property in the mentioned clearCachedDefinitions() method.

I've updated the issue summary with the current function names. This will need a change record (or an update to an existing one).

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -43,6 +43,13 @@
   +  protected $bundleExtraFields = array();
   

   If we're dropping 'bundle' from the method name, we should also drop it from the variable name.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -132,7 +139,7 @@ class EntityManager extends PluginManagerBase implements EntityManagerInterface
   -   *   The cache backend to use.
   +   *   The generic cache backend to use.
   

   Do we still want to change this if we're not using a separate field cache?

3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -817,6 +817,58 @@ public function testGetTranslationFromContext() {
   +    $language = new Language();
   +    $language->id = $language_code;
   

   Why not $language = new Language(array('id' => $language_code));?

   Not that it makes much difference if $langauge_code = 'en' anyway. I'm not very experienced with automated tests, but the new test doesn't seem to check the same things as testFieldInfoExtraFieldsTranslation(). Do we need more tests if we're going to remove that?

This looks ready to me, I just found the following minor remarks:

1. +++ b/core/modules/system/entity.api.php
   @@ -829,3 +831,78 @@ function hook_entity_field_access_alter(array &$grants, array $context) {
   + * fields, but also non-field components. For nodes, these include the title
   

   title is a bad example, it's not an extra field any more. So maybe we could copy over a different code example as well.

2. +++ b/core/modules/system/entity.api.php
   @@ -829,3 +831,78 @@ function hook_entity_field_access_alter(array &$grants, array $context) {
   + * Alter "pseudo-field" components on fieldable entities.
   

   content entities

Why not $language = new Language(array('id' => $language_code));?.
Because it's a trivial code style issue and I like my way

Passing it in the constructor means $name and $direction will be set to the correct values. Not that it really matters for this test, because they are being set to the same values anyway and are not being read.

Thanks! This was green already, so RTBC given it will be again.

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.php
@@ -363,39 +363,4 @@ protected function getExpectedFieldTypeDefinition() {
-    // Create translation for new string and save it.
-    $translated_string = $this->randomString();
-    $locale_storage->createTranslation(array(
-      'lid' => $en_string->lid,
-      'language' => 'hu',
-      'translation' => $translated_string,
-    ))->save();
-
-    // Check that the label is translated.
-    \Drupal::translation()->setDefaultLangcode('hu');
-    $field_info = \Drupal::service('field.info');
-    $user_fields = $field_info->getBundleExtraFields('user', 'user');
-    $this->assertEqual($user_fields['form']['account']['label'], $translated_string);

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
@@ -817,6 +817,58 @@ public function testGetTranslationFromContext() {
+  function testgetExtraFields() {
+    $this->setUpEntityManager();
+
+    $entity_type_id = $this->randomName();
+    $bundle = $this->randomName();
+    $language_code = 'en';
+    $hook_bundle_extra_fields = array(
+      $entity_type_id => array(
+        $bundle => array(
+          'form' => array(
+            'foo_extra_field' => array(
+              'label' => 'Foo',
+            ),
+          ),
+        ),
+      ),
+    );
+    $processed_hook_bundle_extra_fields = $hook_bundle_extra_fields;
+    $processed_hook_bundle_extra_fields[$entity_type_id][$bundle] += array(
+      'display' => array(),
+    );
+    $cache_id = 'entity_bundle_extra_fields:' . $entity_type_id . ':' . $bundle . ':' . $language_code;
+
+    $language = new Language();
+    $language->id = $language_code;
+
+    $this->languageManager->expects($this->once())
+      ->method('getCurrentLanguage')
+      ->will($this->returnValue($language));
+
+    $this->cache->expects($this->once())
+      ->method('get')
+      ->with($cache_id);
+
+    $this->moduleHandler->expects($this->once())
+      ->method('invokeAll')
+      ->with('entity_extra_field_info')
+      ->will($this->returnValue($hook_bundle_extra_fields));
+    $this->moduleHandler->expects($this->once())
+      ->method('alter')
+      ->with('entity_extra_field_info', $hook_bundle_extra_fields);
+
+    $this->cache->expects($this->once())
+      ->method('set')
+      ->with($cache_id, $processed_hook_bundle_extra_fields[$entity_type_id][$bundle]);
+
+    $this->assertSame($processed_hook_bundle_extra_fields[$entity_type_id][$bundle], $this->entityManager->getExtraFields($entity_type_id, $bundle));
+  }

Where do we test translatability here? We have no assertions or expectation set that a translated label is returned in the new test.

+1 to getting this in as it is.

I did a quick change and the old test coverage is actually bogus and does *not* verify what it should. You can remove the $langcode from the cache cid and it still passes, because it only calls the method for one language.

That's the only thing this has been written to test, all the other like adding a language and a translation was only necessary because it's the only way to detect if there is a different string in there. Not because we want to test that the string is translatable.

The new test coverage verifies that the correct cache key with the language code is used. We could even go a step further and call it again and then return a different langcode and verify that the hook is called again.

+! to RTBC, also this needs follow-up to clean-up old tests

Committed fee6ae0 and pushed to 8.x. Thanks!