Remove field_read_field(s)() and field_read_instance(s) in favor of entity_load() and entity_load_multiple_by_properties()

+++ b/core/modules/field/field.attach.incundefined
@@ -1553,7 +1553,7 @@ function field_entity_bundle_create($entity_type, $bundle) {
-  $instances = field_read_instances();
+  $instances = entity_load_multiple('field_instance');
   foreach ($instances as $instance) {
     if ($instance->entity_type == $entity_type && $instance->bundle == $bundle_old) {

Doesn't this mean that we could restrict with params in _by_properties() instead of loading all ?

+++ b/core/modules/field/field.info.incundefined
@@ -304,13 +304,13 @@ function field_info_fields() {
- *   The field array, as returned by field_read_fields(), with an
- *   additional element 'bundles', whose value is an array of all the bundles
- *   this field belongs to keyed by entity type. NULL if the field was not
- *   found.
+ *   The field array, as returned by entity_load_multiple_by_properties(), with
+ *   an additional element 'bundles', whose value is an array of all the
+ *   bundles this field belongs to keyed by entity type. NULL if the field was
+ *   not found.

The "array" and "with an additional 'bundles' entry" parts are not really true anymore. We could be more drastic here and just say "the field definition" with a type hint on FieldInterface
(same for the next two functions)

#11: 2018319-drop-field-read-x-11.patch queued for re-testing.

Tagging

While working on this issue I noticed that it takes more than 6 lines to replace a "field_read_field" call...
So this feels like a serious dx regression.

I think it's better to keep these functions for now....

Updated issue summary.

Here's a reroll to keep this ball rolling after *a lot* of changes around, some of the changes have been made in other parts or the files/calls simply don't exist anymore.

Would be good to get rid of those before beta...
Anyone up for a reroll ?

@aspilicious: does your comment #15 still apply ? do you have specific examples ?

"beta target" -> bumping priority accordingly

We talked about in Prague and it should be less than 6 lines now. We will see if someone gets the patch green without the 'overhead'.

Reroll, plus adjusetd the code in ImageStyle::replaceImageStyle() - we should be iterating over EntityDisplays now, not configurable fields.

Conflicts, so no easy interdiff though :-/

No more field.active

Moar "active/inactive" fixes
+ removes invocation of hook_field_read_*(), that hook is long gone from our api.

Ok, this looks good. The patch applies and fixes the issue described. The comments look good too, so RTBC.

1. +++ b/core/modules/field/field.deprecated.inc
   @@ -244,8 +241,7 @@ function field_info_field_by_id($field_id) {
     *   An array, each key is a field ID and the values are field arrays as
   - *   returned by field_read_fields(), with an additional element 'bundles',
   - *   whose value is an array of all the bundle this field belongs to.
   + *   returned by entity_load_multiple_by_properties().
   

   Missed a spot :)

2. +++ b/core/modules/field/field.module
   @@ -222,10 +223,10 @@ function field_entity_bundle_create($entity_type, $bundle) {
   -      $id_new = $instance->entity_type . '.' . $bundle_new . '.' . $instance->getName();
   +      $id_new = $instance->entity_type . '.' . $bundle_new . '.' . $instance->field_name;
   

   Is this right?

3. +++ b/core/modules/field/field.module
   @@ -248,10 +249,10 @@ function field_entity_bundle_rename($entity_type, $bundle_old, $bundle_new) {
   +  // Get the instances on the bundle. entity_load_multiple_by_properties() must be
   +  // used here since field_info_instances() does not return instances for disabled
      // entity types or bundles.
   

   Needs a small rewrap.

And I see that we remove field_read_field[s](), but not field_read_instance[s](), any reason for that?

1. Reworded that area a bit.
2. Yeah, it itches me to see those ->getName() calls on code that knows it deals with FieldInstance entities, especially when there's a direct ->entity_type or ->bundle access on the same line :-) So I guess I couldn't resist here.
But right, this change has nothing to do here. Reverted.
3. Fixed.

And indeed, patch was not removong the actual field_read_instance*() functions... Fixed.

Purrfect, the interdiff looks good so I'm keeping the RTBC status :)

Actually... could we do the conversion here, then move the actual removal to a separate issue that just removes the @deprecated functions?

We don't have a standard way to deal with @deprecated yet, but for things that should obviously be removed, I might want to try to schedule those patches to land around the same time.

Also that guarantees zero patch conflicts from other patches in the queue that might possibly use these functions still.

@catch: sad panda - see #2155697: Remove a couple simple deprecated functions.

July was too late for @deprecated removals, now is too early, at some point it's going to be too late again - slightly disconcerting ;-)

Given the amount of issues still on the table, I'd sure appreciate to call done "done", get those out of my mental plate and avoid the overhead of keeping track of "@deprecated removal" followups while we wait for an appropriate moment to actually remove them hoping we don't miss the moment when it becomes too late again...

But well. Rather get the conversions in while it applies and is green.

Rerolled, keeping the @deprecated functions around.
Not sure what to do with the actual removal then - merge it with #2155697: Remove a couple simple deprecated functions ?

Forgot the interdiff.
(looking at it is weird :-p)

Yes it's a mess with @deprecated, but that's why I'd rather get the conversions in without the mess holding it up.

Putting them in the other issue is fine. For functions like this which are not used that massively (compared to node_load() or something), we could probably do a commit just before/after one of the alphas.

See also #2158871: [Policy, no patch] Clearly denote when @deprecated code is slated to be removed.