Follow up from #1735118: Convert Field API to CMI

Changes to happen

Talked this through a little with alexpott on IRC and moving the logic to the storage controllers is a good idea. He suggested making the methods also static and renaming the method to findByProperties() but that's more of a config overall DX issue that needs to created.

So for now, just moving the logic and not yet deprecating the old functions is enough for this patch.

Old summary

Move to EFQ ? are they even still needed ?.

Probably involves a separate API to retrieve deleted fields, which very little code actually needs to deal with.

Files: 
CommentFileSizeAuthor
#23 field-read-x-1953414-23.patch17.62 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 55,517 pass(es).
[ View ]
#23 interdiff.txt1.09 KBswentel
#22 interdiff-19vs22.patch7.03 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-19vs22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 field-read-x-1953414-19.patch17.62 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,297 pass(es).
[ View ]
#19 interdiff.txt1.25 KBandypost
#19 field-read-x-1953414-19.patch17.62 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,663 pass(es).
[ View ]
#17 interdiff.txt2.63 KBandypost
#17 field-read-x-1953414-16.patch17.63 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found.
[ View ]
#15 interdiff.txt5.86 KBandypost
#15 field-read-x-1953414-14.patch18.36 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found.
[ View ]
#13 interdiff.txt674 bytesandypost
#13 field-read-x-1953414-12_.patch17.62 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#12 interdiff.txt9.27 KBandypost
#12 field-read-x-1953414-12.patch17.61 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#8 field-read-x-1953414-8.patch12.66 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 54,964 pass(es).
[ View ]
#8 interdiff.txt4.17 KBswentel
#4 field-read-x-1953414-4.patch13.66 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 54,096 pass(es), 76 fail(s), and 70,742 exception(s).
[ View ]

Comments

Issue tags:+Field API

Tagging

Status:Postponed» Active

Un-postponing.

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

Status:Active» Needs review
StatusFileSize
new13.66 KB
FAILED: [[SimpleTest]]: [MySQL] 54,096 pass(es), 76 fail(s), and 70,742 exception(s).
[ View ]

Let's see what happens when moving this to the storage controllers - marked the functions deprecated already too as they are still there.

Title:Figure out what to do with field_read_(field|instance)() Move logic of field_read_fields() and field_read_instances() to the storage controller.

Talked this through a little with alexpott on IRC and moving the logic to the storage controllers is a good idea. He suggested making the methods also static and renaming the method to findByProperties() but that's more of a config overall DX issue that needs to created.

So for now, just moving the logic and not yet deprecating the old functions is enough for this patch.

Yep that sums up the discussion nicely.

Status:Needs review» Needs work

The last submitted patch, field-read-x-1953414-4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.17 KB
new12.66 KB
PASSED: [[SimpleTest]]: [MySQL] 54,964 pass(es).
[ View ]

This should be a little bit better :)

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

The last submitted patch, field-read-x-1953414-8.patch, failed testing.

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

#8: field-read-x-1953414-8.patch queued for re-testing.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new17.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new9.27 KB

So just override contructor

StatusFileSize
new17.62 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new674 bytes

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?

StatusFileSize
new18.36 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found.
[ View ]
new5.86 KB

state should be injected too //cc Crel

Status:Needs review» Needs work

The last submitted patch, field-read-x-1953414-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new17.63 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found.
[ View ]
new2.63 KB

@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

Status:Needs review» Needs work

The last submitted patch, field-read-x-1953414-16.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new17.62 KB
PASSED: [[SimpleTest]]: [MySQL] 55,663 pass(es).
[ View ]
new1.25 KB

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.

StatusFileSize
new17.62 KB
PASSED: [[SimpleTest]]: [MySQL] 55,297 pass(es).
[ View ]
new7.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-19vs22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Other fixed

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new1.09 KB
new17.62 KB
PASSED: [[SimpleTest]]: [MySQL] 55,517 pass(es).
[ View ]

Good to go, two extreme nitpicks in the patch.

+1 to rtbc

Status:Reviewed & tested by the community» Fixed

Committed e416a5c and pushed to 8.x. Thanks!

Sweet - @yched, now drop field_read_field/instance - field_read_fields/instances completely and depend on entity_load_multiple_by_properties() ?

- edit - although that's against my comment earlier of course.
- edit 2 - probably just keep them for now - alex suggested that too

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

Well, just like we're killing field_create_field etc, kill field_read_field - field_read_fields() - field_read_instance() - field_read_instances().

People will instead have todo something like this:

<?php
 
// For field(s).
 
$conditions = array();
  return
entity_load_multiple_by_properties('field_entity', $conditions);
 
// For instance(s).
 
$conditions = array();
  return
entity_load_multiple_by_properties('field_instance', $conditions);
?>

We have some special keys for $conditions, like 'include_inactive' and 'include_deleted' which don't actually map to properties and are now handled within field_read_fields() and field_read_instances(). This might make it a bit more confusing.

Also, a lot of search and replaces of course in core if we do this. Not sure, it feels consistent, but I have no strong opinion.

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

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

Issue summary:View changes

Updated issue summary.