Remove usage of field_info_instances(), use EntityManager::getFieldDefinitions() instead

Stumbled upon this issue; fixed typo in the title.

@kgoel are you still working on this ?

Sorry, picked this issue to work on during ladder for quick patch but got busy.

ok, no problem.

@berdir - should we add method on the EntityManager to get the configurable fields ? See this diff, it might become a little annoying

diff --git a/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php
index 01c65ed..1251abe 100644
--- a/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php
+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php
@@ -10,9 +10,9 @@
 use Drupal\Core\Entity\EntityManagerInterface;
 use Drupal\Core\Extension\ModuleHandlerInterface;
 use Drupal\Core\Field\FieldTypePluginManagerInterface;
-use Drupal\field_ui\OverviewBase;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Drupal\field\Entity\FieldConfig;
+use Drupal\field\FieldInstanceConfigInterface;
 
 /**
  * Field UI field overview form.
@@ -88,7 +88,13 @@ public function buildForm(array $form, array &$form_state, $entity_type_id = NUL
     parent::buildForm($form, $form_state, $entity_type_id, $bundle);
 
     // Gather bundle information.
-    $instances = field_info_instances($this->entity_type, $this->bundle);
+    $instances = array();
+    $field_definitions = \Drupal::entityManager()->getFieldDefinitions($this->entity_type, $this->bundle);
+    foreach ($field_definitions as $key => $definition) {
+      if ($definition instanceof FieldInstanceConfigInterface) {
+        $instances[$key] = $definition;
+      }
+    }
     $field_types = $this->fieldTypeManager->getDefinitions();

@swentel for that purpose better to add optional argument for getFieldDefinitions() a-la "onlyConfigurable = FALSE"

@Berdir maybe a static method to filter fields? If fields are separated by #2226197: Introduce FieldStorageDefinitionInterface in the Entity Field API

As @swentel pointed out, field_info_instances() needs a subset of EntityManager::getFieldDefinitions(), so the results need to be filtered every time.

I agree with not passing an argument as we shouldn't add new arguments for every type of filtering we might need. A static method to allow filtering of fields might be handy, especially it would allow us to filter fields on properties. In this particular case it would not be very useful though as it would boil down to a callback like this:

  public static function isConfigurable($definition) {
    return $definition instanceof FieldInstanceConfigInterface;
  }

We only have a handful of calls to field_info_instances(), so I would just do the filtering where the calls are made.

Imo there should be no filtering possibility for configurable fields on the EM, as no module using the field API should do any differentiation on that. Given, field module will need to fetch that list of fields repeatedly, I think it makes sense to have a helper somewhere in field module do that. It should be used only internally though, e.g. only by field module - everyone else shouldn't have to bother.

Here's my work in progress so far, I am going to attend a BOF now, will resume work in an hour or so.

As I see there's a lot of usage so better put new function to \Drupal\field\FieldInfo

Alright, will do so!

Just discussed this with @swentel and he informed me that FieldInfo is going away soon, so it is no use putting it in there. I will go ahead with just doing the conversion, we can still provide a convenience method in a followup.

Trying this simple reroll

Let me know if I miss understood what this issue need to be reviewed and RTBC

but it really needed or we keep the filter for some extra reason?

Yes, there is a difference between field_info_instances() and EntityManager::getFieldDefinitions($entity_type, $bundle). The first returns field instances, while the other returns field definitions (which includes instances). So you cannot just replace field_info_instances() with getFieldDefinitions().

Most of the time the definitions need to be filtered so that only the instances remain, but in some cases the original implementation was wrong and should *not* be filtered, as explained in comments #18 and #19 above.

Second try, reroll patch.

So only the content_translation.pages.inc is without the array_filter ... the rest keep the filter. Lets see if the patch pass the test.

26: remove-usage-of-field_info_instances-2225739-26.patch queued for re-testing.

26: remove-usage-of-field_info_instances-2225739-26.patch queued for re-testing.

Replaced all except ones in \Drupal\field\Tests\FieldInfoTest this file should gone soon.
Content translation as always has a wrong test, that was commented out with @todo.

We should use proper replacements not "blind" replace.

Also filed follow-up #2231863: Remove field_views_field_label()

The hunk in ER selection base plugin causes UI change, suppose this needs follow-up (there's @todo already)

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/selection/SelectionBase.php
@@ -109,7 +109,9 @@ public static function settingsForm(FieldDefinitionInterface $field_definition)
-        $bundle_instances = \Drupal::entityManager()->getFieldDefinitions($target_type, $bundle);
+        $bundle_instances = array_filter(\Drupal::entityManager()->getFieldDefinitions($target_type, $bundle), function ($field_definition) {
+          return $field_definition instanceof FieldInstanceConfigInterface;

I think this hunk wrong, ER should not differentiate fields, see filed follow-up issue and related to #2211553: Allow usage of entity_reference field widget and formatter for base fields

I think this one is RTBC.
Inclusion of #2152571: ER: List of things you can sort by on a default reference type method is incomplete makes sense for scope.

Patch looks great, except I don't get why this test is being removed:

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php
@@ -196,26 +196,6 @@ function testTranslateLinkContentAdminPage() {
-  function testFieldTranslationForm() {
-    $this->drupalLogin($this->administrator);
-
-    $article = $this->drupalCreateNode(array('type' => 'article', 'langcode' => 'en'));
-
-    // Visit translation page.
-    $this->drupalGet('node/' . $article->id() . '/translations');
-    $this->assertRaw('Not translated');
-
-    // Delete the only translatable field.
-    field_info_field($this->entityTypeId, 'field_test_et_ui_test')->delete();
-
-    // Visit translation page.
-    $this->drupalGet('node/' . $article->id() . '/translations');
-    $this->assertRaw('No translatable fields');
-  }

#33.1 says "It is not possible to enable translation without making at least one field translatable.", but I don't know if that's referring to this hunk; if it is, then that's not what this hunk is testing; this hunk is testing what happens if you delete the last translatable field after already enabling translation.

If you do a fresh install, enable Content Translation module, add a language to the site, and then create a node, then even if you have not yet enabled content translation for that node type yet (or even for any entity type at all), you still get a "translate" tab when you view the node, and clicking it takes you to a table where the other languages have a "no translatable fields" link that take you to admin/config/regional/content-language. But if you grep HEAD for "no translatable fields", then the test being removed here is the only test for that. So rather than removing the test, can we change it to test that condition when you disable translatability of the node type rather than when you implicitly do that via deleting the last translatable field? Similarly, the other assertion of "Not translated" for when the node type is translatable is also a useful test to keep.

This is already critical and a beta blocker, but this now blocks #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities.

I'd be fine with #33 being committed and a follow up opened to discuss whether to restore that test in some form. I'm tempted to RTBC this now, but would like confirmation from one other reviewer about whether that's ok.

Actually, doing it. Whoever is the core committer for this can be that other reviewer :)

Theoretically I think it could be possible, by disabling JS, to make a bundle translatable without making any of its fields translatable, base fields included. Not sure we need to test this edge-case, but the functionality supporting it is still there...

I'm fine with the current approach too. We can extend with edge-case tests later on if we want.

Committed and pushed to 8.x. Thanks!