Move logic of field_read_fields() and field_read_instances() to the storage controller.

Un-postponing.

Suppose it's much easy to mark them as deprecated in favor of entity_load_multiple_by_properties()

Yep that sums up the discussion nicely.

+++ b/core/modules/field/field.crud.incundefined
@@ -154,64 +154,11 @@ function field_read_fields($conditions = array(), $include_additional = array())
+  return entity_load_multiple_by_properties('field_entity', $conditions);

@@ -362,80 +309,11 @@ function field_read_instances($conditions = array(), $include_additional = array
+  return entity_load_multiple_by_properties('field_instance', $conditions);

Why not mark them @deprecated?

+++ b/core/modules/field/lib/Drupal/field/FieldInstanceStorageController.phpundefined
@@ -33,4 +33,92 @@ public function importDelete($name, Config $new_config, Config $old_config) {
+  public function loadByProperties(array $conditions = array()) {

Parent implementation already have this method... so probably could be inherited

+++ b/core/modules/field/lib/Drupal/field/FieldInstanceStorageController.phpundefined
@@ -33,4 +33,92 @@ public function importDelete($name, Config $new_config, Config $old_config) {
+      $instances = entity_load_multiple('field_instance', array($conditions['entity_type'] . '.' . $conditions['bundle'] . '.' . $conditions['field_name']));
...
+      $instances = entity_load_multiple('field_instance');
...
+      $state = \Drupal::state();
+      $deleted_fields = $state->get('field.field.deleted');
...
+        $instances[$id] = entity_create('field_instance', $config);
...
+      if (!$include_inactive && !entity_get_info($instance->entity_type)) {
...
+      module_invoke_all('field_read_instance', $instance);

+++ b/core/modules/field/lib/Drupal/field/FieldStorageController.phpundefined
@@ -0,0 +1,90 @@
+      $fields = entity_load_multiple('field_entity', array($conditions['field_name']));
...
+      $fields = entity_load_multiple('field_entity');
...
+      $deleted_fields = \Drupal::state()->get('field.field.deleted') ?: array();
...
+        $fields[$id] = entity_create('field_entity', $config);
...
+      module_invoke_all('field_read_field', $field);

ControllerInterface should inject em

So just override contructor

a small copy/paste fix

+++ b/core/modules/field/lib/Drupal/field/FieldInstanceStorageController.phpundefined
@@ -21,6 +26,69 @@
+    $this->entityType = $entity_type;
+    $this->entityInfo = $entity_info;
+    $this->hookLoadArguments = array();
+    $this->idKey = $this->entityInfo['entity_keys']['id'];
...
+    if (isset($this->entityInfo['entity_keys']['status'])) {
+      $this->statusKey = $this->entityInfo['entity_keys']['status'];
+    }
+    else {
+      $this->statusKey = FALSE;
+    }
+
+    $this->configFactory = $config_factory;
+    $this->configStorage = $config_storage;

Why not simply call parent::__construct($entity_type, ...); as it will set all this values as well.

+++ b/core/modules/field/lib/Drupal/field/FieldStorageController.phpundefined
@@ -0,0 +1,158 @@
+    $this->entityType = $entity_type;

Here as well.

In general I'm wondering whether the really similar logic in both FieldStoragecontroller and FieldInstanceStorageController could be moved to a helper object?

state should be injected too //cc Crel

@dawehner nice catch! I think a Base class is out of scope but makes sense and leads to bikesheding

Let's address @deprecated in other issue

Should be fine now

+++ b/core/modules/field/lib/Drupal/field/FieldStorageController.php
@@ -0,0 +1,158 @@
+      $deleted_fields = \Drupal::state()->get('field.field.deleted') ?: array();

This should be injected, too.

+++ b/core/modules/field/field.crud.incundefined
@@ -154,64 +154,11 @@ function field_read_fields($conditions = array(), $include_additional = array())
+  return entity_load_multiple_by_properties('field_entity', $conditions);

@@ -362,80 +309,11 @@ function field_read_instances($conditions = array(), $include_additional = array
+  return entity_load_multiple_by_properties('field_instance', $conditions);

Why not use Drupal::entityManager()->getStorageController('field_entity')->loadByProperties($conditions) directly?

+++ b/core/modules/field/lib/Drupal/field/FieldInstanceStorageController.phpundefined
@@ -21,6 +27,67 @@
+   * @var \Drupal\Core\Extension\ModuleHandler
...
+   * @param \Drupal\Core\Extension\ModuleHandler $module_handler
...
+  public function __construct($entity_type, array $entity_info, ConfigFactory $config_factory, StorageInterface $config_storage, EntityManager $entity_manager, ModuleHandler $module_handler, KeyValueStoreInterface $state) {

There should be always the ModuleHandlerInterface type hinted as it exists.

+++ b/core/modules/field/lib/Drupal/field/FieldStorageController.phpundefined
@@ -0,0 +1,157 @@
+      $fields = $this->entityManager->getStorageController($this->entityType)->load(array($conditions['field_name']));
...
+      $fields = $this->entityManager->getStorageController($this->entityType)->load();
...
+        $fields[$id] = $this->entityManager->getStorageController($this->entityType)->create($config);

So the storage controller is used that often ... why not store it in the constructor on the object.

Let's leave entity_load_multiple_by_properties() as is (procedural wrapper)

Other fixed

+1 to rtbc

Committed e416a5c and pushed to 8.x. Thanks!

@swentel: hmm, I'm a bit lost here actually. Can you summarize the options ?

Ideally, yes, we'd want to get rid of field_read_*() IMO.

- The notion of inactive fields should theoretically go away with "modules can't be disabled anymore"
- I think I remember a discussion in Portland where there was an agreement to remove the support for querying deleted & non-deleted fields in the same API function. Like "if you need a deleted field / instance, fetch it yourself from state()".
Or maybe it was in the issue queue. I do remember a heavy "yes" from @alexpott on this point :-)