Problem/Motivation

Right now, with two field info systems: The one for entity fields maintained by the EntityManager and the one for configurable fields (FieldInfo).

Almost everything in FieldInfo now has corresponding methods in the entity manager, with the exception of the field map.

Additionally, there are a lot of calls left to the old functions/methods that need to be updated.

Proposed resolution

The patch does a number of different things

  1. Add a new getFIeldMap() method to the EntityManager to replace FieldInfo::getFieldMap(). The new field map has the same structure but also contains non-configurable fields. That makes it slower to build as we need to loop over all content entity types and their bundles and collect the field definitions of those. @todo: Profile cpu/memory usage, try to optimize and open a follow-up to add locking if necessary.
  2. Most remaining usages of field info functions can be classified into two groups. The first use them to directly interact with specific configurable fields, like the helper methods that add default body fields, tests and so on. For those, the patch adds FieldConfig::loadByName($entity_type, $field_name) (replaces field_info_field()) and FieldInstanceConfig::loadByName($entity_type, $bundle, $field_name) (replaced field_info_instance()) that directly load the entity. Two special cases (in FieldInstanceConfig::__construct() and Tables.php) need to load deleted fields, there we need to go through loadByProperties() as those fields don't really exist anymore and can only be loaded with that strange API. This is an existing problem that used to be internal to FieldInfo and now becomes a bit more visible. But only very few places need that.
  3. Most other usages can be replaced with getFieldDefinitions()/getFieldStorageDefinitions() on the entity manager. This also involves replacing the injected field info service with the entity manager if not already present.

How that is done exactly often looks a bit different, a few notable special cases:

  1. Entity Query Tables.php: This has a lot of hacks that go back to times when we didn't have the API's that we have now, so a lot of code that be simplified there. This patch does as much as necessary, further improvements are certainly possible but go beyond what we need to do achieve the goals of this issue.
  2. In ContentEntityDatabaseStorage, we still need to check for configurable field objects as those methods only care about those, #2144263: Decouple entity field storage from configurable fields will address that further.
  3. comment_entity_bundle_info() can not call getFieldMap() on the entity manager as that results in a loop. This means that for the time being, comments will only support configurable fields, this is not a regression as this is already the case in HEAD, #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form *might* address this.
  4. In field_entity_bundle_field_info() we now use an entity query to load all matching field instances
  5. The content entity migration destination has also been simplified a bit for the entity bundle handling and as a side effect, the (currently unused field-type specific handling) would now also work for base fields.
  6. The same is true for the Term autocomplete controller, it is now possible to define a term reference field as a base field in your entity type and use the autocomplete widget for it.

Remaining tasks

1. Get reviews.
2. Implement reviews (including existing reviews from @fago and @yched)
3. Some non-beta blocking follow-ups need to be opened as mentioned above.
4. Profile the new getFieldMap() implementation on a site with a large number of bundles and fields.

User interface changes

None.

API changes

FieldInfo::getFieldMap() -> EntityManager::getFieldMap()
field_info_field() -> FieldConfig::loadByName() (only for cases where the code is explicitly working with configurable fields, see node_add_body_field() as an example)
field_info_instance() -> FieldInstanceConfig::loadByName() (same as above)

Everything else is just converting to existing API's. Only exception are changed constructor arguments for things like entity storage implementations, in case subclasses extend those, they would need to be updated (like user and comment in core), but so far, we never made change records for such changes AFAIK.

Original report by @fago

Right now, with two field info systems: The one for entity fields maintained by the EntityManager and the one for configurable fields. Ideally, we've only one and avoid duplicated field definitions in memory for the same thing (as we have it now).

As discussed in Prague the plan is to move all over to an entity field definition service (wherever that will resides). However, for that being possible we need to complete the following issues first:
#2047229: Make use of classes for entity field and data definitions
#2114707: Allow per-bundle overrides of field definitions

Once we've that issue fixed, I think we've finally managed to unify both field APIs nicely.

Postponed until

#2047229: Make use of classes for entity field and data definitions
#2114707: Allow per-bundle overrides of field definitions
#2226197: Introduce FieldStorageDefinitionInterface in the Entity Field API
#2225629: Move hook_field_extra_fields() to the entity API

Blocks

#2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form
#2167167: Remove field_info_*()

Files: 
CommentFileSizeAuthor
#118 cleanup-field-info-stuff-2116363-118-interdiff.txt2.4 KBBerdir
#118 cleanup-field-info-stuff-2116363-118.patch170.86 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,187 pass(es).
[ View ]
#115 cleanup-field-info-stuff-2116363-115-interdiff.txt1.05 KBBerdir
#115 cleanup-field-info-stuff-2116363-115.patch169.14 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,225 pass(es).
[ View ]
#113 cleanup-field-info-stuff-2116363-113-interdiff.txt1.5 KBBerdir
#113 cleanup-field-info-stuff-2116363-113.patch169.14 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,273 pass(es).
[ View ]
#112 cleanup-field-info-stuff-2116363-112-interdiff.txt8.77 KBBerdir
#112 cleanup-field-info-stuff-2116363-112.patch167.64 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,242 pass(es).
[ View ]
#102 cleanup-field-info-stuff-2116363-102.patch166.46 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,225 pass(es).
[ View ]
#102 interdiff.txt18.13 KBjessebeach
#101 interdiff.txt1004 bytesjessebeach
#101 cleanup-field-info-stuff-2116363-101.patch167.47 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,830 pass(es).
[ View ]
#99 cleanup-field-info-stuff-2116363-99.patch167.56 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-field-info-stuff-2116363-99.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#99 interdiff.txt14.73 KBjessebeach
#95 cleanup-field-info-stuff-2116363-95.patch165.39 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,511 pass(es).
[ View ]
#95 interdiff.txt693 bytesjessebeach
#92 interdiff.txt761 bytesjessebeach
#92 cleanup-field-info-stuff-2116363-92.patch165.45 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-field-info-stuff-2116363-92_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#85 cleanup-field-info-stuff-2116363-85.patch165.44 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,512 pass(es).
[ View ]
#85 interdiff.txt2.98 KBjessebeach
#82 cleanup-field-info-stuff-2116363-82.patch165.28 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,397 pass(es).
[ View ]
#74 cleanup-field-info-stuff-2116363-74.patch165.29 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-field-info-stuff-2116363-74.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#72 cleanup-field-info-stuff-2116363-72.patch165.31 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-field-info-stuff-2116363-72.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#71 cleanup-field-info-stuff-2116363-71-interdiff.txt7.87 KBBerdir
#71 cleanup-field-info-stuff-2116363-71.patch165.18 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,678 pass(es).
[ View ]
#69 cleanup-field-info-stuff-2116363-69.patch162.65 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,717 pass(es), 1 fail(s), and 9 exception(s).
[ View ]
#65 cleanup-field-info-stuff-2116363-65-interdiff.txt8.52 KBBerdir
#65 cleanup-field-info-stuff-2116363-65.patch164.56 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-field-info-stuff-2116363-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#60 cleanup-field-info-stuff-2116363-60-interdiff.txt7.4 KBBerdir
#60 cleanup-field-info-stuff-2116363-60.patch164.95 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,053 pass(es).
[ View ]
#58 cleanup-field-info-stuff-2116363-58.patch157.55 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,040 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#56 cleanup-field-info-stuff-2116363-56-interdiff.txt15.79 KBBerdir
#56 cleanup-field-info-stuff-2116363-56.patch157.86 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-field-info-stuff-2116363-56.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#49 cleanup-field-info-stuff-2116363-49-relevant-change.txt1.61 KBBerdir
#49 cleanup-field-info-stuff-2116363-49-interdiff.txt2.1 KBBerdir
#49 cleanup-field-info-stuff-2116363-49.patch147.46 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,841 pass(es).
[ View ]
#44 cleanup-field-info-stuff-2116363-44-interdiff.txt867 bytesBerdir
#44 cleanup-field-info-stuff-2116363-44.patch146.4 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,727 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#42 cleanup-field-info-stuff-2116363-41-interdiff.txt54.02 KBBerdir
#41 cleanup-field-info-stuff-2116363-41-interdiff.txt1.17 KBBerdir
#41 cleanup-field-info-stuff-2116363-41.patch146.29 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,348 pass(es), 64 fail(s), and 8 exception(s).
[ View ]
#39 cleanup-field-info-stuff-2116363-39-interdiff.txt16.18 KBBerdir
#39 cleanup-field-info-stuff-2116363-39.patch104.03 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,639 pass(es), 139 fail(s), and 135 exception(s).
[ View ]
#37 cleanup-field-info-stuff-2116363-37-interdiff.txt17 KBBerdir
#37 cleanup-field-info-stuff-2116363-37.patch103.33 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,504 pass(es), 210 fail(s), and 148 exception(s).
[ View ]
#34 cleanup-field-info-stuff-2116363-34-interdiff.txt904 bytesBerdir
#34 cleanup-field-info-stuff-2116363-34.patch92.25 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#32 cleanup-field-info-stuff-2116363-32-interdiff.txt37.17 KBBerdir
#32 cleanup-field-info-stuff-2116363-32.patch92.25 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/content_translation/content_translation.module.
[ View ]
#31 cleanup-field-info-stuff-2116363-31-interdiff.txt37.63 KBBerdir
#31 cleanup-field-info-stuff-2116363-31.patch59.92 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,735 pass(es), 773 fail(s), and 104 exception(s).
[ View ]
#23 cleanup-field-info-stuff-2116363-23.patch23.17 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,624 pass(es), 1,258 fail(s), and 651 exception(s).
[ View ]

Comments

yched’s picture

Title:Merge field info services» Unified repository of field definitions (cache + API)
Issue summary:View changes

Retitling a bit

fago’s picture

Status:Postponed» Active
yched’s picture

I'm not sure to which extent this can be decoupled from #2114707: Allow per-bundle overrides of field definitions... Seems hard to do one without the other ?

effulgentsia’s picture

Priority:Major» Critical
Issue tags:+Approved API change, +beta blocker

Per discussion with catch, this is a beta blocker.

fago’s picture

Assigned:Unassigned» fago

I'll come up with a proposal of how the API could look like, so we have a discussion start.

fago’s picture

afaics, FieldInfo right now takes care of three things:
1. getInstances(), $entity type optional
2. getBundleInstances()
3. getFields
4. getBundleExtraFields
5. getFieldMap
6. getField, getInstance()

On the entity field API side we've got

<?php
 
public function getFieldDefinitions($entity_type, $bundle = NULL);
?>

on the entity manager so far. So what does that duplicate and what's missing compared to the field API repository?
Let's see:

1) is covered, however $entity_type is required. Imo that should be acceptable as getting all instances across all entity types will be very resource intensive and we should not encourage doing that - if you really need to do that, loop over the entity types yourself.

2) is covered.

3) is missing - entity field API has no notion of "storage fields", i.e. the part of a field definition that is shared across all bundles.

4) Extra fields is no concept that exists at the entity field API level so far. I think it shouldn't be part of the regular entity field API as extra fields are something else. But anyway it's not clear to me whether we need to take them into account or they might go away anyway? If they won't go away, I suppose it would make most sense to give them their own repository/function (and possibly a rename?)

5. getFieldMap() seems to be hardly used, but mostly comment module seems to need it to find a list of its fields and do something with it. We have nothing comparable for entity fields yet.

6. There is no helper to get a single field definition. We might want to add one, or wait for the PHP 5.4 issue to be fixed and go with getFieldDefinitions()[$name]. Howsoever, we might want to add a helper that eases filtering the returned field definitions, e.g. to only include configurable, translatable ones, or the fields of a certain type. Maybe we could support passing in an optional closure used for filtering, or we leave it up to the caller?

Summarizing, we mostly have to take care of 3. and 5., and do some work on 4. and 6 in order to be able to use entity-field's repository for configurable fields as well. I don't see 4-6 as so problematic, but for 3) we miss a respective concept as pointed out by yched in #3. (also see #2114707-26: Allow per-bundle overrides of field definitions.

More concrete, I could see us doing:

  • define something along the lines a FieldStorageDefintion + interface and add a respective method getFieldStorageDefinitions($entity_type), which includes full Field definitions (which extend the interface) + field storage definitions not being full field definitions also.
  • add getExtraFields($entity_type, $bundle = NULL), working analogously to 4. How we want to cache those information is another question, I'd prefer to postpone discussing until we've figured out the interface.
  • add a getFieldDefinitionMap() working analogously to the config field equivalent, but for all field definitions.
  • optionally, improve all methods with a consistent way to filter and/or obtain a specific field definition

As we probably don't have an agreement on the FieldStorageDefinition issue yet, I'd suggest leaving 3) out for now and taking care of the rest first. Once we figured out #2114707: Allow per-bundle overrides of field definitions, we can add adapt the repository API accordingly. Thoughts?

yched’s picture

Thanks a lot for this @fago.
Overall, agreed with the plan.

A couple precisions below:

1) Fully agreed on getting rid of getInstances("all entity types"). API-wise and performance-wise this doesn't make sense to support it. We might need to check the actual uses though.

Not fully sure getInstances($entity_type) is covered though - it returns lists of $instances ("varying by bundle"), keyed by bundle name. IIRC that's not what you intended for getFieldDefinitions($entity_type) (flat list, no by-bundle variants) ?

I'd happily remove support for getInstances($entity_type) too, for that matter. But would need checking the actual uses as well...

3) Yes, if base fields have no equivalent for "stuff that define a storage", the registry of $field is orphaned and left alone in field.module's FieldInfo. Could be an issue conceptually / DX-wise, but agreed with leaving that aside for now.

4) Extra fields have always been the odd duck :-/.
Conceptually they have no business in field.module whatsoever. They are here simply because there was nowhere else to put them in CCK D6 / Field API D7, and because Field UI introduced the need.

Up until D7 they were "stuff that are *not* fields ;-) but appear in entity forms / views and thus need to be part of Field UI's drag-n-drop screens".
In current D8 they are "stuff that are not rendered through widgets / formatters but still need to be reorderable". That includes base fields still rendered through custom #render code, or arbitrary 3rd party additions like https://drupal.org/project/eva. They're not going away in D8...

They don't belong in "D8 Field API" (Core/Field) either, but rather they belong to the entity system itself. #1954188: Revaluate the concept of 'extra fields' was titled "move extra fields to the entity system" at some point, but it got rejected as an API change last summer. IMO we should reconsider if we're gutting FieldInfo, makes really no sense to leave them there.

Probably doesn't have to be done in this issue here, though...

5) getFieldMap() is used to keep getBundleInstances() performant, but there possibly might be other ways now.

API wise it's used for cleanup / housekeeping tasks (find all file fields, all comment fields). In early 7.x releases the only alternative was the perf-awful field_info_instances("all, everywhere"). That API tool is still needed IMO, and agreed, it should apply at the level of "all fields" rather than just configurable fields.

6) A "filtering" method will possibly be needed at some point, but meanwhile a simple EntityManager::getFieldDefinition($name) would still be very handy to have IMO ?

yched’s picture

FieldInfo::getInstances("all entity types") / field_info_instances(NULL) :
no use case in core (other that testing it)

FieldInfo::getInstances($entity_type) / field_info_instances($entity_type) :

- field_entity_field_info() ;-)

- field_views_field_label():
Finds the "label" most frequently used for a field across its instances. There has to be other ways... Worst case, it doesn't seem unreasonable to include the label in the FieldMap.

- entity_reference\Plugin\entity_reference\selection\SelectionBase::settingsForm()
Collects possible fields to sort by on the target entity type (has a "@todo Use Entity::getPropertyDefinitions()").
Could use baseFieldDefinitions() + configurable $fields. There's probably also a need for "human label" like for field_views_field_label() above, but the current code just uses the label of the last instance iterated on.

yched’s picture

While playing with the Entity REST (HAL) API, found out that field_info_field_map() is also used to parse "_embedded" relations in incoming entity POST / PATCH data.

Thus it currently only accounts for configurable fields, and causes weirdness on idempotence / re-entrance of GET+POST or GET+PATCH:
All entity_ref fields currently output their data in "_embedded" on GET, but only the "_embedded" entries of configurable fields are parsed on PATCH/POST.

So yes, we definitely need a field_info_field_map() for [base + configurable] fields...

xjm’s picture

xjm’s picture

Status:Active» Postponed
Related issues:+#2114707: Allow per-bundle overrides of field definitions

@berdir pointed out that we should get #2114707: Allow per-bundle overrides of field definitions in first here, so postponing on that issue.

Berdir’s picture

ianthomas_uk’s picture

Have I understood correctly that we are proposing to remove \Drupal\field\FieldInfo? If so, we should mark it @deprecated, and update the replacement instructions on the field_info_*() functions. Are there any other classes that we plan to remove?

YesCT’s picture

While working on #2152571-30: ER: List of things you can sort by on a default reference type method is incomplete

it had in SelectionBase

    if ($target_type->isSubclassOf('\Drupal\Core\Entity\ContentEntityInterface')) {
      $sort_fields = array();
      // Add base fields.
      foreach (\Drupal::entityManager()->getFieldDefinitions($target_type_id) as $field_definition) {
        $sort_fields += static::getFieldOption($field_definition);
      }
      // Add configurable fields.
      foreach (Field::fieldInfo()->getInstances($target_type_id) as $bundle_instances) {
        foreach ($bundle_instances as $instance_definition) {
          $sort_fields += static::getFieldOption($instance_definition);
         }
       }

the line:
foreach (\Drupal::entityManager()->getFieldDefinitions($target_type_id) as $field_definition) {
phpstorm warned me that:
getFieldDefinitions has a second required argument of $bundle

@Berdir notes that is getBaseFieldDefinitions() now because of #2114707: Allow per-bundle overrides of field definitions and asked me to make a note here as it might effect thinking on this issue.

Berdir’s picture

What I meant is the use case "give me all fields of an entity type", something that entity.module in 7.x used to call entity_get_all_property_info() or something. Not sure if that's something we want to provide too or not.

Berdir’s picture

Xano’s picture

fago’s picture

jessebeach’s picture

Issue summary:View changes
ianthomas_uk’s picture

Title:Unified repository of field definitions (cache + API)» [PP-1] Unified repository of field definitions (cache + API)
Related issues:+#2226197: Introduce FieldStorageDefinitionInterface in the Entity Field API
xjm’s picture

Title:[PP-1] Unified repository of field definitions (cache + API)» Unified repository of field definitions (cache + API)

Unpostponed!

xjm’s picture

Status:Postponed» Active

no really

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new23.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,624 pass(es), 1,258 fail(s), and 651 exception(s).
[ View ]

Starting to do more clean-up to remove unnecessary usages of the field info service and convert it to using the entity manager instead.

There are two types of usages of the field info methods/functions:

- Actual runtime usages to get settings and so on, those can use field definitions
- But there are also a lot of cases that continue to interact with field config (instances) and need to continue to do so, because they create them or change settings and need to save them, the new API does not offer that, unless you do an instanceof at least.

There are also a few usages that have existing issues to deal with them, translation sync and taxonomy term autocomplete, not toching them for now..

Berdir’s picture

Ah, one thing I forgot is that for the cases that need to use instances, using the entity API would be very ugly because it would look like this:

- field_info_instance('node', 'article', 'body')
+ entity_load('field_instance_config', 'node.article.body')

=> You need to know about the internal structure of field config ID's, so maybe we can keep some wrapper methods/functions around that no longer use any caching but just call entity_load() directly?

Status:Needs review» Needs work

The last submitted patch, 23: cleanup-field-info-stuff-2116363-23.patch, failed testing.

fago’s picture

You need to know about the internal structure of field config ID's, so maybe we can keep some wrapper methods/functions around that no longer use any caching but just call entity_load() directly

That seems, reasonable. e.g. field_config_load() & field_instance_config_load() - as part of field.module?

andypost’s picture

There's own cache for Config no need for another layer of cache.
Also #1880766: Preload configuration objects could be helpful to load some data

EDIT this wrappers are remains of hook_menu() loaders for field_ui

Berdir’s picture

@andypost we're not talking about existing functions, we're talking about adding new ones that have the same arguments as field_info_field() and field_info_instance() today. And the problem with the field info cache is that he's designed for specific use cases like, loading all field instances and automatically preloading all field config objects for that, which is something that is currently broken but that's not the problem there anyway

Not sure about those specific function names though because a), they're too alike to normal entity load functions like node_load() and b) adding new functions is frowned upon these days.. static methods somewhere? a service seems overkill? We could add something like FieldConfig::loadByName() or something like that, once we have ::load() but I know you don't like that issue very much...

Berdir’s picture

Actually, I like FieldConfig::loadByName(), almost all use cases except CommentManager are functions and tests right now anyway, so not much of a problem, and as those methods are custom anyway, we can hardcode the entity_type and avoid many problems the load() issue has.

Working on a updated patch...

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new59.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,735 pass(es), 773 fail(s), and 104 exception(s).
[ View ]
new37.63 KB

Replaced field_info_field() calls outside of FieldInfoTest (which will be removed).

Berdir’s picture

StatusFileSize
new92.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/content_translation/content_translation.module.
[ View ]
new37.17 KB

More changes. Didn't touch the content entity storage yet and comment manager als still injects the FieldInfo, and there are two calls to field map to which we have no replacement yet, but other than that, there shouldn't be much left.

Not sure about the changes to FieldInstanceConfig but without the field map and the pre-load logic based on the UUID, all the uuid logic there seems useless.. there's a separte issue about that being way slower right now, although it's still not as optimized as it was before.

Status:Needs review» Needs work

The last submitted patch, 32: cleanup-field-info-stuff-2116363-32.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new92.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new904 bytes

Ups.

The last submitted patch, 31: cleanup-field-info-stuff-2116363-31.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 34: cleanup-field-info-stuff-2116363-34.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new103.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,504 pass(es), 210 fail(s), and 148 exception(s).
[ View ]
new17 KB

Fixing tests.

Status:Needs review» Needs work

The last submitted patch, 37: cleanup-field-info-stuff-2116363-37.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new104.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,639 pass(es), 139 fail(s), and 135 exception(s).
[ View ]
new16.18 KB

Ouch deleted fields, also noticed that I had the order of entity type, bundle and field name wrong in many cases for loadByName(), this should hopefully fix lots of the remaining test fails.

Status:Needs review» Needs work

The last submitted patch, 39: cleanup-field-info-stuff-2116363-39.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new146.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,348 pass(es), 64 fail(s), and 8 exception(s).
[ View ]
new1.17 KB

More fixes, converted migration content entity, infamous Tables.php converted, ContentEntityDatabaseStorage converted as well.

I believe we're just left with just the field map stuff, no idea about that :(

Berdir’s picture

That was obviously the wrong interdiff.

Status:Needs review» Needs work

The last submitted patch, 41: cleanup-field-info-stuff-2116363-41.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new146.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,727 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new867 bytes

Fixing deleted fields and entity queries..

Status:Needs review» Needs work

The last submitted patch, 44: cleanup-field-info-stuff-2116363-44.patch, failed testing.

yched’s picture

Yay !
+1 on the overall approach, I'll try (ahem) to review more closely soonish :-/

Just skimming through the recent interdiffs :

+++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListBuilder.php
@@ -90,13 +89,12 @@ public function setMapperDefinition($mapper_definition) {
-    foreach (Field::fieldInfo()->getInstances($this->baseEntityType) as $fields) {
-      $entities = array_merge($entities, array_values($fields));
-    }
-    return $entities;
+    $ids = \Drupal::entityQuery('field_instance_config')
+      ->condition('entity_type', $this->baseEntityType)
+      ->execute();
+    return $this->storage->loadMultiple($ids);

Couldn't this be done with getAllByPrefix() ? (or whatever the name of the "give me all config records starting with field.instance.[entity_type].*" CMI method, can't look at code right now)

Berdir’s picture

Yes, we could do that, but I'm not sure if we should have knowledge about how field instance config ID's are built and the internal outside of field.module. That said, this is just as internal and it's config translation, so it knows about config :)

What we/I did in field_entity_base_field/storage_info() is to use an entity query + a STARTS_WITH condition on the ID, we should optimize that in our config entity query implementation, I think at least that is a good abstraction to have and should be reasonably fast then.

Berdir’s picture

Also, @yched/@fago/anyone, do you of you have any ideas what we should do about the field map?

With Tables.php refactored to not use it, we have two cases left in core that need it, both are "give me all fields of a specific type", they are a bit different however:

- Vocabulary::postSave(): This updates field config configuration to remove itself from the vocabularies setting of term reference fields. We *could* replace that with an EFQ as we can only care/update field_config entities anyway, we can't save base fields in case someone would add a term reference base field.

- CommentManager::getallFields(): That returns a field map array of all comment fields, which is then used to display the comment bundle overview page and it's also used all over the place to check if a specific entity type has any fields. There's the issue about fixing the bundle length problem for comment fields, and one of the options there is about introducing a new config thing so we could possibly build on that.

Either way, contrib will have more use cases for "give me all fields of my type" and probably even more generic thing that you need a field map for...

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new147.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,841 pass(es).
[ View ]
new2.1 KB
new1.61 KB

Ah fun, I fixed an actual bug in FieldOverview.php which didn't consider the now configurable field prefix, now we have tests that used to verify the exception that is thrown in FieldConfig and instead runs into the form validation, which is better but now we no longer test the code in FieldConfig...

Adding the relevant changes in FieldOverview for context.

Gábor Hojtsy’s picture

Was asked to review this :) Yay! Looking at the patch there are very numerous changes and the issue summary is far from up to date as to what is going on in the patch at all. Would be great to get an overview of what is going on to make reviewing possible for someone coming in like me.

Berdir’s picture

Will try to update it, but the goal is simple.

Kill all usage of field_info_* functions and the FieldInfo service.

There are a few repeating patterns on how I do that, will try to group and summarize them, but there are many special one-off changes necessary which will need reviews from people who know the respective code but any review helps :)

fago’s picture

- Vocabulary::postSave(): This updates field config configuration to remove itself from the vocabularies setting of term reference fields. We *could* replace that with an EFQ as we can only care/update field_config entities anyway, we can't save base fields in case someone would add a term reference base field.

That's weird actually, as usually we do not enforce integrity of references, but silently dismiss stale references. Not sure this would be an option here as well, but I agree that we need to keep supporting the field map use case anyway as it's going to be needed by contrib.

I started reviewing this. I was not able to finish yet, but here a first part:

  1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
    @@ -95,25 +95,27 @@ public function addField($field, $type, $langcode) {
    +        if ($fields = entity_load_multiple_by_properties('field_config', array('uuid' => substr($specifier, 3), 'include_deleted' => TRUE))) {

    include_deleted is not a property by which we should be able to load here - so that's confusing. I'd rather add a "deleted => 0" property by default if no conditions are passed, and use "deleted" => 1 else to fit the API - but that's a pre-existing issue.
    But moreover, it's interesting that we query be field-config-id here what is not valid according to the interface. That's a separate issue though as well.

  2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
    @@ -133,39 +135,16 @@ public function addField($field, $type, $langcode) {
    +            $propertyDefinitions = $field->getPropertyDefinitions();

    -            // Get the field definitions form a mocked entity.
    -            $values = array();
    -            $field_name = $field->getName();
    ...
    -            if ($bundle_key = $entity_type->getKey('bundle')) {
    -              $values[$bundle_key] = reset($field_map[$entity_type_id][$field_name]['bundles']);
    -            }

    Awesome cleanup.

  3. +++ b/core/modules/block/custom_block/custom_block.module
    @@ -114,8 +116,8 @@ function custom_block_entity_bundle_info() {
    +  $field = FieldConfig::loadByName('custom_block', 'body');

    Good DX + it makes it clear it's about config fields only very well!

  4. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.php
    @@ -105,24 +94,29 @@ public function overviewBundles() {
         foreach ($fields as $entity_type => $data) {
    +      $field_storages = $this->entityManager()->getFieldStorageDefinitions($entity_type);
    ...
    +        $label = $field_storage->getLabel();
    +        if ($field_storage instanceof FieldConfigInterface) {

    I think $field_storage is a confusing variable name, as it's not the field's storage. I'd rather use $storage_definition or just $definition if that's not ambiguous?

  5. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -770,4 +770,18 @@ protected function getFieldItemClass() {
    +   * @return static

    Minor: @return static|null ?

Berdir’s picture

1. & 6. include_deleted exists in HEAD, I'm just using it, this is the code that getFieldById() would fall back to if it is not defined, which is the case for deleted fields. It's all fake, because deleted fields exist in state only, but not something we can deal with here...

4. Yeah, have to clean that up in Tables.php as well.

Berdir’s picture

Ouch, started to implement the approach that we discussed but run into a blocker with comment.module. We need the bundle list to build the field map but comment.module needs the field map to provide the bundles.

Not sure how to resolve this, we might need to switch to a separate config file for comment bundles as discussed before or only support configurable fields and instead of using the map, use an entity query?

fago’s picture

comment_entity_bundle_info() seems to work already with the first field only, i.e. couldn't we rely on field storage definitions here? That would resolve the loop but require us to have a map for storage definitions as well :/

In general, yeah switching to a separate config item that results in respective fields to be added would make more sense to me. Not sure, this might happen anytime soon? Do you know the issue # of it?

So yeah, it seems requiring configurable fields + going with efq would be reasonable to move one here now. But then, would the follow-up to fix it have to be beta blocking also? :/

Berdir’s picture

StatusFileSize
new157.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-field-info-stuff-2116363-56.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new15.79 KB

Ok, this should work and comment fields should at least work as well as they do now (meaning, no support for base fields yet in the bundles).

Status:Needs review» Needs work

The last submitted patch, 56: cleanup-field-info-stuff-2116363-56.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new157.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,040 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-roll.

Status:Needs review» Needs work

The last submitted patch, 58: cleanup-field-info-stuff-2116363-58.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new164.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,053 pass(es).
[ View ]
new7.4 KB

Fixed that unit test and added new unit tests for getFieldMap(), also added some missing @covers.

fago’s picture

Status:Needs review» Needs work

Thanks! I've finished reviewing now - looks already great. Here a few remarks:

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -514,6 +524,35 @@ public function getFieldStorageDefinitions($entity_type_id) {
    +        // Rebuild the definitions and put it into the cache.
    +        foreach ($this->getDefinitions() as $entity_type_id => $entity_type) {

    How long does the field map rebuilding take? Should we use locking to protect from cache stampeding or would that be premature optimization?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -514,6 +524,35 @@ public function getFieldStorageDefinitions($entity_type_id) {
    +          if ($entity_type->isFieldable()) {

    That check is deprecated now as non-fieldable entities have fields as well. (Yes, that flag needs to be renamed ;)

  3. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
    @@ -75,11 +74,12 @@ public function addField($field, $type, $langcode) {
    +    // @todo Needed for menu links.

    What should/can we do about it here?

  4. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.php
    @@ -200,7 +194,12 @@ public function bundleInfo($commented_entity_type, $field_name) {
    +    $field_storage = $this->entityManager()->getFieldStorageDefinitions($commented_entity_type)[$field_name];

    field_storage left-over -> storage definition

  5. +++ b/core/modules/file/file.module
    @@ -1857,9 +1861,10 @@ function file_icon_map(File $file) {
    + * @param \Drupal\field\FieldConfigInterface $field
    + *   (optional) A field definition to be used for this check. If given, limits the
      *   reference check to the given field.
    + *   @todo This should use FieldDefinitionInterface instead.

    Should this be solved as part of this issue? If not, it needs to be fixed such that description + type hint maches and the comment does not exceed 80chars. Also best, create the follow-up and link it here.

The unit test looks good to me, but do we miss test coverage for the configurable field integration or is that covered as part of an existing test? Should we do an integration test for that else?

Berdir’s picture

Thanks.

1. Not sure, hard to say without looking at it on a real site with lots of entity types, bundles and fields. I'll see if I can generate some data for that. I wouldn't add it there, but maybe a @todo and follow-up.

2. True, isFieldable() is wrong, but we do need a check for content entity.

3. Nothing, we need to complete the menu link conversion, I'll add a reference to that issue.

I don't think we need an integration test for configurable fields, this builds on getFieldDefinitions(), if that returns the fields then the field map will work as well.

fago’s picture

I don't think we need an integration test for configurable fields, this builds on getFieldDefinitions(), if that returns the fields then the field map will work as well.

True.

ad 1) Sounds good to me.

Berdir’s picture

Issue summary:View changes

Updated issue summary.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new164.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-field-info-stuff-2116363-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new8.52 KB

Re-rolled, opened follow-up to improve id based query performance: #2247379: Optimize config entity query conditions on ID.

#52.4/#62.4: Yeah, renamed.
#52.5: Improved documentation.

#62.2: Looking for ContentEntityTypeInterface now.
#62.3: Added issue reference.
#62.5: Tried to address it, the code is unused and broken in HEAD (the $field variable is overwritten in the loop), so we can't really make it worse :)

Berdir’s picture

Some first benchmarks, will do some more profiling on this:

Generated a bunch of node types and fields and then run my test script:

Number of node types: 16
Number of fields: 137
FieldInfo::getFieldMap(): 65.97ms
EntityManager::getFieldMap(): 246.86ms

FieldInfo::getBundleInstances(): 78.32ms
FieldInfo::getBundleInstances() different type: 4.55ms
FieldInfo::getBundleInstances() from cache: 9.14ms

EntityManager::getFieldDefinitions(): 53.24ms
EntityManager::getFieldDefinitions() different bundle: 7.11ms
EntityManager::getFieldDefinitions(): 8.4ms

What this shows is that the new field map is obviously much slower but the nice thing is that we don't need the field map at runtime (well, with the exception of comment.module), so building the field definitions of a single node type is actually faster (the numbers vary quite a bit by run), I thought that building the second bundle would be much faster for the field info implementation, but because all config objects have been loaded into memory already, it doesn't make much of a difference. What's strange is that loading from an empty static cache seems be slower with the new method, that might be related to the changes I made to FieldInstanceConfig::construct().

Will do some profiling to find out what exactly is slower and if there's something to optimize.

The node type generation is a devel generate plugin implementation, will post that too :)

The test script:

<?php
<?php

use Drupal\Component\Utility\Timer;
use
Drupal\field\Field;

echo
"Number of node types: " . count(\Drupal::entityManager()->getBundleInfo('node')) . "\n";
echo
"Number of fields: " . count(Field::fieldInfo()->getFields()) . "\n";


drupal_flush_all_caches();
Timer::start('field_info_field_map');
Field::fieldInfo()->getFieldMap();
$time = Timer::stop('field_info_field_map');

echo
"FieldInfo::getFieldMap(): " . $time['time'] . "ms\n";

drupal_flush_all_caches();
Timer::start('entity_manager_field_map');
\
Drupal::entityManager()->getFieldMap();
$time = Timer::stop('entity_manager_field_map');

echo
"EntityManager::getFieldMap(): " . $time['time'] . "ms\n\n";

drupal_flush_all_caches();
Timer::start('field_info_bundle_instances');
Field::fieldInfo()->getBundleInstances('node', 'thi');
$time = Timer::stop('field_info_bundle_instances');

echo
"FieldInfo::getBundleInstances(): " . $time['time'] . "ms\n";

Timer::start('field_info_bundle_instances2');
Field::fieldInfo()->getBundleInstances('node', 'wewrebebr');
$time = Timer::stop('field_info_bundle_instances2');

echo
"FieldInfo::getBundleInstances() different type: " . $time['time'] . "ms\n";

Timer::start('field_info_bundle_instances_cache');
\
Drupal::configFactory()->reset();
\
Drupal::getContainer()->set('field.info', NULL);
Field::fieldInfo()->getBundleInstances('node', 'thi');

$time = Timer::stop('field_info_bundle_instances_cache');

echo
"FieldInfo::getBundleInstances() from cache: " . $time['time'] . "ms\n\n";

drupal_flush_all_caches();
Timer::start('entity_manager_field_definitions');
\
Drupal::entityManager()->getFieldDefinitions('node', 'thi');
$time = Timer::stop('entity_manager_field_definitions');

echo
"EntityManager::getFieldDefinitions(): " . $time['time'] . "ms\n";

Timer::start('entity_manager_field_definitions2');
\
Drupal::entityManager()->getFieldDefinitions('node', 'wewrebebr');
$time = Timer::stop('entity_manager_field_definitions2');

echo
"EntityManager::getFieldDefinitions() different bundle: " . $time['time'] . "ms\n";

Timer::start('entity_manager_field_definitions_cache');
\
Drupal::configFactory()->reset();
\
Drupal::getContainer()->set('entity.manager', NULL);
\
Drupal::entityManager()->getFieldDefinitions('node', 'thi');
$time = Timer::stop('entity_manager_field_definitions_cache');

echo
"EntityManager::getFieldDefinitions(): " . $time['time'] . "ms\n";
?>
Berdir’s picture

Status:Needs review» Needs work

The last submitted patch, 65: cleanup-field-info-stuff-2116363-65.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new162.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,717 pass(es), 1 fail(s), and 9 exception(s).
[ View ]

Re-roll, minor conflict in EntityDatabaseStorage and some code in comment AdminController that I changed has been deleted.

Status:Needs review» Needs work

The last submitted patch, 69: cleanup-field-info-stuff-2116363-69.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new165.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,678 pass(es).
[ View ]
new7.87 KB

Fixing the migrate test fails...

Berdir’s picture

StatusFileSize
new165.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-field-info-stuff-2116363-72.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll.

Status:Needs review» Needs work

The last submitted patch, 72: cleanup-field-info-stuff-2116363-72.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new165.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-field-info-stuff-2116363-74.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll.

fago’s picture

I had a look at the recent improvements + looked over the whole patch again, I think it's ready to go. However, I think we should remove the field info service + accessor functions now as well.

comment_entity_bundle_info() can not call getFieldMap() on the entity manager as that results in a loop. This means that for the time being, comments will only support configurable fields, this is not a regression as this is already the case in HEAD, #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form *might* address this.

Should we create a separate issue for making comment field work with base fields also? Just to ensure we do not miss the problem if #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form for some reason does not happen.

Also, the summary could need an update.

fago’s picture

Status:Needs review» Reviewed & tested by the community

We already have #2167167: Remove field_info_*(), so let's do it in a second step then.

fago’s picture

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 74: cleanup-field-info-stuff-2116363-74.patch, failed testing.

jessebeach’s picture

I'll start on the Change Notice for this issue.

Berdir’s picture

Thanks! See the issue summary, most of this issue is just applying changes for things we already have the API's for, the only exceptions being the stuff listed in API changes I think.. Not sure if that should be a new change notice or adding it to existing field related ones...

Looks like it also needs a re-roll, will do that tonight.

jessebeach’s picture

Good point, I'll check existing change notices as well.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new165.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,397 pass(es).
[ View ]

Minor conflict in CustomBlockTypeTest because we renamed Block body to Body.

fago’s picture

Re-roll looks good, thanks. Let's wait for the change record before we re-RTBC then.

yched’s picture

Yeah, really sorry, started to review last week, and got stalled :-/

The main remarks I had were pertaining to DX :

- in lots of places we have to stuff like :

<?php
foreach ($this->entityManager->getFieldDefinitions($this->entityTypeId, $bundle) as $field_name => $instance) {
    if (
$instance instanceof FieldInstanceConfigInterface) {
?>

The recurrence of this hints that there should be more direct way ?

- Similarly, lots of places do something like :

<?php
  $field
= \Drupal::entityManager()->getFieldStorageDefinitions($this->definition['entity_type'])[$this->definition['field_name']];
?>

PHP 5.4 does allow this, I'm not conviced that's great legibility though, expecially on such long lines where the final array de-referencing is after the fold and what you see before the fold is a misleading "$field = $EM::getFieldStorageDefinitions(...)"
--> Could that be a new "get single field" method in EntityManager ?

Those two tasks could easily be followups if needed - we'd just need to go through the places that this patch modifies and adjust them back.

jessebeach’s picture

StatusFileSize
new2.98 KB
new165.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,512 pass(es).
[ View ]

Rerolled, conflicts with HEAD. The rejected hunk corrections are in the interdiff.

jessebeach’s picture

Ok, as someone with at best passing knowledge of the Entity and Field systems, let me just say that EntityManager::getFieldMap() makes sense! To me! This is awesome. I'm walking through the patch with a debugger and everything's clicking into place as I would expect. The stack trace follows a grokkable progression of steps as fields are requested from an entity manager class, the field map is requested from the entity manager and the base and bundle (non-configurable) fields and field_instance_config (configurable) fields from field_entity_bundle_field_info() are assembled and combined.

And any time you see code that works through mocking get replaced with a simple one-liner, well, that's good :)

+ $propertyDefinitions = $field->getPropertyDefinitions();

- // Get the field definitions from a mocked entity.
- $values = array();
- $field_name = $field->getName();
- // If there are bundles, pick one.
- if ($bundle_key = $entity_type->getKey('bundle')) {
-   $values[$bundle_key] = reset($field_map[$entity_type_id][$field_name]['bundles']);
- }
- $entity = $entity_manager
-   ->getStorage($entity_type_id)
-   ->create($values);
- $propertyDefinitions = $entity->$field_name->getFieldDefinition()->getPropertyDefinitions();
-
- // If the column is not yet known, ie. the
- // $node->field_image->entity case then use first property as
- // column, i.e. target_id or fid.
- // Otherwise, the code executing the relationship will throw an
- // exception anyways so no need to do it here.
- if (!$column && isset($propertyDefinitions[$relationship_specifier]) && $entity->{$field->getName()}->first()->get('entity') instanceof EntityReference) {
-   $column = current(array_keys($propertyDefinitions));
- }

We're now making good use of the "FieldStorage" code introduced in #2226197: Introduce FieldStorageDefinitionInterface in the Entity Field API.

Would it be prudent to completely remove usage of field_info_field_map() in this patch as well? Such as in FieldConfig?

Berdir’s picture

Yeah, getFieldMap() is easy to understand, our worry is that it might not perform well when you have many entity types, bundles and fields. See #66. We agreed to move forward, it's very hard to estimate how this will be on real, large sites, so we will have to profile it there and then find ways to optimize for example memory usage if we need to (by clearing the static cache during building up the map, for example).

I guess I missed to replace all calls to field_info_field_map(), yes, we should do that here too.

We even discussed to remove the old code completely, which is currently supposed to happen in #2167167: Remove field_info_*(), but that would make the patch quite a bit bigger (we'd remove ~1k lines of code) and it's large already...

@yched: Yeah, I was wondering about single-field versions of those methods too, would mean 3 more methods I guess (base field, field, storage field). Would give us a central place to e.g. throw an exception instead of an unpleasant fatal error if something doesn't exist. Could be done as a follow-up as you said, but we would have to touch quite a bit of code twice.. @fago?

jessebeach’s picture

@todo: Open follow-up to improve config entity conditions on the ID.

I don't 100% understand what this follow-up would require, so I must lean on the devs in this issue to get this issue logged before can move to commit the patch in this issue.

jessebeach’s picture

Yeah, getFieldMap() is easy to understand, our worry is that it might not perform well when you have many entity types, bundles and fields.

I'll do some profiling.

jessebeach’s picture

jessebeach’s picture

Issue summary:View changes
jessebeach’s picture

StatusFileSize
new165.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-field-info-stuff-2116363-92_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new761 bytes

Small comment cleanup.

Status:Needs review» Needs work

The last submitted patch, 92: cleanup-field-info-stuff-2116363-92.patch, failed testing.

jessebeach’s picture

That patch in 92 applied just fine locally. grumble grumble. I'll reroll it.

For the moment, I want to lay out some performance data. The scenario here is the front page with 10 article nodes. I took the fastest of 6 page reloads (cached) on 8.x and then with the patch applied. Comparing the two, we see a 16ms degradation in page performance with this patch in #92.

Run #536bdfd6ad8aa Run #536be0d2793c1 Diff Diff%
Number of Function Calls 67,835 67,107 -728 -1.1%
Incl. Wall Time (microsec) 528,060 544,571 16,511 3.1%
Incl. CPU (microsecs) 495,581 510,117 14,536 2.9%
Incl. MemUse (bytes) 15,844,056 15,825,112 -18,944 -0.1%
Incl. PeakMemUse (bytes) 15,889,152 15,870,024 -19,128 -0.1%

Digging into the comparison, this patch introduces performance degradations for renderables that aren't cached correctly, like the Administrative Toolbar (#2254865: toolbar_pre_render() runs on every page and is responsible for ~15ms/17000 function calls).

I uninstalled the Toolbar module for the second round of testing using the same scenario as the first test. The results are encouraging:

Run #536bebb66e5f6 Run #536bec4548829 Diff Diff%
Number of Function Calls 56,106 55,406 -700 -1.2%
Incl. Wall Time (microsec) 477,441 456,808 -20,633 -4.3%
Incl. CPU (microsecs) 445,567 428,588 -16,979 -3.8%
Incl. MemUse (bytes) 14,935,688 14,916,640 -19,048 -0.1%
Incl. PeakMemUse (bytes) 14,920,920 14,901,544 -19,376 -0.1%

We see an improvement of 20ms with this patch. I think the lesson here is that these changes exacerbate performance issues in modules in how they assemble renderables.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new693 bytes
new165.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,511 pass(es).
[ View ]

Reroll because of a conflict in SearchCommentTest. See the interdiff.

xjm’s picture

jessebeach’s picture

fago, Berdir, I'm feeling good about this patch. What think you two? Let's get this in!

Berdir’s picture

I think the only thing left is #84, the question is if we want to add those methods here or create a follow-up.

Thanks for the change record, looks good, I think we shouldn't mentioon the field methods on entity classes there, it's not really related to that (as they are about base fields, which were never covered by field info), just reference to the existing change records that we have for that topic, like https://drupal.org/node/1806650 and https://drupal.org/node/2205235 and https://drupal.org/node/2236285.

jessebeach’s picture

StatusFileSize
new14.73 KB
new167.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-field-info-stuff-2116363-99.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
  1. in lots of places we have to stuff like :

    <?php
    foreach ($this->entityManager->getFieldDefinitions($this->entityTypeId, $bundle) as $field_name => $instance) {
        if (
    $instance instanceof FieldInstanceConfigInterface) {
    ?>

    The recurrence of this hints that there should be more direct way ?

    I thought about introducing a method like eachFieldDefintion to EntityManagerInterface. We could pass an interator callback function to the method as an argument. But we'd still need to specify the entity type and bundle, so in the balance, it does not end up being less code; we might save on the repetition of the for loop at the cost of complicating the entity manager interface. And then we set up a pattern that might lead to a method like eachFieldStorageDefinition. I can't convince myself that methods on the interface are any better her than a few raw for loops.

  2. Similarly, lots of places do something like :

    <?php
      $field
    = \Drupal::entityManager()->getFieldStorageDefinitions($this->definition['entity_type'])[$this->definition['field_name']];
    ?>

    PHP 5.4 does allow this, I'm not conviced that's great legibility though, expecially on such long lines where the final array de-referencing is after the fold and what you see before the fold is a misleading "$field = $EM::getFieldStorageDefinitions(...)"
    --> Could that be a new "get single field" method in EntityManager ?

    I agree these one-liners are hard to parse while scanning the code. I'd rather just pull the one-liners apart into two and clean up variations on "field storage" variables, standardizing to $fieldStorageDefinitions. I've done this in the updated patch.

    If we introduce a method like singleFieldStorageDefinition($entity_type, $field_name), that will suggest a method like singleFieldDefinition($entity_type, $bundle, $field_name) which can be just as complex as a one-liner depending on how the arguments are supplied. Plus we'll end up complicating EntityManagerInterface with these highly-specific methods, just like the iterating methods I mentioned in the bullet point above.

    Personally I'm fine with the code if we just pull apart the one-liners like I've done.

  3. I updated the change notice with links to the related notices.

Status:Needs review» Needs work

The last submitted patch, 99: cleanup-field-info-stuff-2116363-99.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new167.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,830 pass(es).
[ View ]
new1004 bytes

The Drop moves fast today.

jessebeach’s picture

StatusFileSize
new18.13 KB
new166.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,225 pass(es).
[ View ]

Refactored instances of $fieldStorageDefinitions to $field_storage_definitions to comply with coding standards: https://drupal.org/coding-standards#naming

fago’s picture

Status:Needs review» Reviewed & tested by the community

@yched: Yeah, I was wondering about single-field versions of those methods too, would mean 3 more methods I guess (base field, field, storage field). Would give us a central place to e.g. throw an exception instead of an unpleasant fatal error if something doesn't exist. Could be done as a follow-up as you said, but we would have to touch quite a bit of code twice.. @fago?

As #99 shows this needs some discussion + quite some issues depend on this one. Thus I think we should move on here now, and cover that in a separate issue.

Changes are good and the change record is there -> RTBC again.

tstoeckler’s picture

I think we will get more used to seeing method()['foo'] over time. I personally think it is much more preferable over introducing a separate method or an optional parameter to the existing method. Such a method needs to be documented, unit tested and it needs to copy paste a trivial implementation for each implementation of the respective interface. And while the corresponding PHP construct might still seem unusual it is 100% clear what is happening.

A typed exception would certainly be preferable, I agree on that part, but a

Undefined index 'foo' on line 123

is quite easily debuggable.

Berdir’s picture

Well, yes and no, the thing is that we're talking about objects, so you wouldn't see that notice.. you'd see a fatal error method call on non-object on the next line, which is already a bit harder to debug, especially when you combine it with dynamic field names and so on.

I've opened #2266003: Add methods to get a single field/base field/field storage definition to the entity manager, we can continue to discuss there.

yched’s picture

Fine with disussing single-field methods in a followup.

Re :

- (myself)

<?php
foreach ($this->entityManager->getFieldDefinitions($this->entityTypeId, $bundle) as $field_name => $instance) {
    if (
$instance instanceof FieldInstanceConfigInterface) {
?>

The recurrence of this hints that there should be more direct way ?
- (jessebeach) I thought about introducing a method like eachFieldDefintion to EntityManagerInterface

Yeah, generic iterators are hard. I was thinking of something much much simpler, related to the very specific use case above, that is duplicated in various places : only get configurable fields.

Berdir’s picture

Yeah, we've already discussed that when field_info_instances() usages were converted but most use cases that still only check for configurable fields are often left-overs/bugs that will prevent using base fields/other defined fields in that case. There are only two cases in this patch:

- ContentEntityStorageBase: Will instead check on whatever it will need to do see if it has separate storage, see https://drupal.org/node/2144263. So if we'd want a method for that, it should probably be a helper method in that class, not on the entity manager?
- comment/AdminController: That will change to a separate config entity for comment bundles in #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form and it's currently a bug that it's limited to configurable fields only.

yched’s picture

re @Berdir #107 : fine, good enough for me. Thanks for the clarification !

Still not losing hope of reviewing this fully, here's what I have for now.

  1. +++ b/core/modules/comment/comment.module
    @@ -98,15 +99,18 @@ function comment_help($route_name, Request $request) {
    -  foreach (\Drupal::service('comment.manager')->getAllFields() as $entity_type => $fields) {
    ...
    +  $ids = \Drupal::entityQuery('field_config')
    +    ->condition('type', 'comment')
    +    ->execute();

    I guess this cannot use comment.manager->getAllFields() because EM->getFieldMap() requires the bnudle info and that would result in infinite recursion ?

    If so, we should keep/adapt the existing comment about recursion.
    E.g: "Find all existing comment fields. We cannot rely on getAllFields() / getFieldMap(), since those require the list of bundles and would result in an infinite recursion."

  2. +++ b/core/modules/comment/comment.module
    @@ -98,15 +99,18 @@ function comment_help($route_name, Request $request) {
    +    list($entity_type_id, $field_name) = explode('.', $id);
    +    $instance_ids = $config_factory->listAll('field.instance.' . $id . '.');

    I might be missing something, but I don't get how this works :

    $id is a FieldConfig ID, i.e [entity_type].[field_name].
    So this code looks for FieldInstanceConfigs with name field.instance.[entity_type].[field_name].* - that won't work, FieldInstanceConfig are field.instance.[entity_type].[BUNDLE].[field_name] ?

  3. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -116,7 +105,7 @@ public function getAllFields() {
    -    if (!$this->fieldInfo->getField($entity_type, $field_name)) {
    +    if (!$this->entityManager->getStorage('field_config')->load($entity_type . '.' . $field_name)) {

    @@ -131,7 +120,7 @@ public function addDefaultField($entity_type, $bundle, $field_name = 'comment',
    -    if (!$this->fieldInfo->getInstance($entity_type, $bundle, $field_name)) {
    +    if (!$this->entityManager->getStorage('field_instance_config')->load($entity_type . '.' . $bundle . '.' . $field_name)) {

    Shouldn't this use the newly introduced loadByName() ?

  4. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.php
    @@ -407,13 +409,13 @@ function testDisallowedFieldNames() {
    -    $this->assertText(t('There was a problem creating field Disallowed field: Attempt to create field title which is reserved by entity type node.', array('%label' => $label)), 'Field was not saved.');
    +    $this->assertText(t('The machine-readable name is already in use. It must be unique.'));

    Oh ? why ?

Berdir’s picture

Status:Reviewed & tested by the community» Needs work

1. Yep exactly. It's temporary, with #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form, it will work just like any other implementation and with #2261401: Automatically provide bundle info/list based on bundle_of annotation., it will be provided by default based on the bundle_of annotation key. So not worried much about documentation there but I can add a sentence...

2. Yeah, that should be the entity type ID I think.. confused, will check that, it apparently doesn't cause any errors?

3. Well, the entity manager is injected and I think some of that stuff there has unit tests...

4. See #49, I updated the validation function to use the field definitions, so it already checks base fields there instead of passing the validation and then resulting in an exception when trying to save it. Changes the scope of this test a bit and it's checking something else, but the resulting UI is way better :)

yched’s picture

Thanks for the explanations, make sense :-)

yched’s picture

Finished reviewing - at last, sorry for the delay :-/

  1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
    @@ -75,11 +74,13 @@ public function addField($field, $type, $langcode) {
    +    // @todo Needed for menu links, make this implementation content entity
    +    //   specific after https://drupal.org/node/1842858.

    Looks like https://drupal.org/node/1842858 has been deprecated in favor of https://drupal.org/node/2256521 ?

    Other than that, not sure I follow everything that's being made in Tables.php, but that code will probably need an overhaul after "generate schemas for entities", so, leaving it alone here as long as its green.

  2. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.php
    @@ -105,24 +94,29 @@ public function overviewBundles() {
    +        $label = $storage_definition->getLabel();
    +        if ($storage_definition instanceof FieldConfigInterface) {
    +          $bundles = $storage_definition->getBundles();
    +          $sample_bundle = reset($bundles);
    +          $field_definitions = $this->entityManager()->getFieldDefinitions($entity_type, $sample_bundle);
    +          $label = $field_definitions[$field_name]->getLabel();
    +        }

    getBundles() currently only exists on FieldConfigInterface, not on FieldStorageDefinitionInterface, that's probably an overlook.
    However, looks like this code could use the field map instead, and then wouldn't need to differenciate configurable / non-configurable fields ?

  3. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.php
    @@ -105,24 +94,29 @@ public function overviewBundles() {
    -        $row['data']['field_name']['data'] = $field_info->get('locked') ? $this->t('@label (@field_name) (Locked)', $tokens) : $this->t('@label (@field_name)', $tokens);

    This will fatal if $storage_definition isn't a FieldConfig ?

  4. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
    @@ -118,15 +106,15 @@ public function commentApprove(CommentInterface $comment) {
    -    if ($entity = $this->entityManager()->getStorage($comment->getCommentedEntityTypeId())->load($comment->getCommentedEntityId())) {
    +    if ($entity = $comment->getCommentedEntity()) {

    Lol - yeah, slightly better :-)

  5. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListBuilder.php
    @@ -90,13 +89,12 @@ public function setMapperDefinition($mapper_definition) {
    +    $ids = \Drupal::entityQuery('field_instance_config')
    +      ->condition('entity_type', $this->baseEntityType)
    +      ->execute();

    That could be optimized with a STARTS_WITH condition on the id ?
    (once the relevant patch is in)

  6. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -798,4 +798,20 @@ protected function getFieldItemClass() {
    +  public static function loadByName($entity_type_id, $field_name) {

    method name is not too accurate (it's actually "load by parts of the ID string rather than by the concatenated ID string itself"), but no better suggestion for now, so never mind.

    I guess if EntityType::load($id) is coming, we could override it by accepting either one arg (the $id) or two args (the $entity_type & $field_name), but that would probably be frowned upon :-p

  7. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php
    @@ -232,24 +231,20 @@ class FieldInstanceConfig extends ConfigEntityBase implements FieldInstanceConfi
       public function __construct(array $values, $entity_type = 'field_instance_config') {

    Hmm - the changes here don't look right.

    - The old code does :
    "load the field using field_uuid if present, else use entity_type/field_name"

    - The new code does :
    "load the field using entity_type/field_name, and if no field was found, use field_uuid if present"
    But that will break in the case of : a field+instances got deleted and a new field+instances with the same name is created while the old ones are still pending purge. There will be code that tries to instanciate a FieldInstanceConfig for the "old" instance.
    With HEAD, field_uuid prevails, so the correct (deleted) FieldConfig will be fetched.
    With patch, loading the field with entity_type/field_name will work and load "some" field, but the wrong one (the non-deleted one)

    field_uuid has to prevail. The glitch is that the previous (misnamed, yay for removal) FieldInfo::getFieldById($field_uuid) encapsulated the fact that deleted and non-deleted fields are stored in separate locations - it looked in both (optimizing for the most current runtime case of non-deleted), and returned whatever it found.

    That logic (fetch a field given its uuid, whether deleted or non-deleted, but optimized for non-deleted) would need to be kept somewhere else.
    - We could inline it directly in FieldInstanceConfig, but it seems a shame to have FieldInstanceConfig know about the storage of deleted fields. So maybe a static loadByUUID() method in FieldConfigStorage ?
    - More importantly, "optimize for non-deleted" means we should have a place where non-deleted fields are statically present in memory keyed by UUID. FieldInfo::$fields used to do that, but that's gone with the unified repository now - UUIDs are specific to configurable fields, the unified repo doesn't cater for them.

    Alternative :
    Only bother for the case of deleted fields in FieldInstanceConfig::__construct() if (!empty($values['deleted']). Because : the field we want *might* be deleted, but *only* if the instance itself is deleted. If the instance is deleted, we still need to look for the field in both deleted and non-deleted (the instance might be deleted and not the field), but then no need to optimize, this only happens at purge time, entity_load_multiple_by_properties('field_config', array('uuid' => $uuid, 'include_deleted' => TRUE)) will do.
    (this is basically what the patch is doing for the other remaining use of getFieldById() in Tables.php)

  8. +++ b/core/modules/file/file.module
    @@ -648,8 +651,9 @@ function file_file_download($uri, $field_type = 'file') {
    +      $field_storage_definitions = \Drupal::entityManager()->getFieldStorageDefinitions($entity_type);
           foreach ($entities as $entity) {
    -        $field = field_info_field($entity_type, $field_name);
    +        $field = $field_storage_definitions[$field_name];

    That "$field = " line could move up one loop.

  9. +++ b/core/modules/forum/forum.module
    @@ -115,7 +115,7 @@ function forum_menu_local_tasks(&$data, $route_name) {
    -    $field = Field::fieldInfo()->getField('node', 'taxonomy_forums');
    +    $field = FieldConfig::loadByName('node', 'taxonomy_forums');

    We don't really need to load the FieldConfig here, this could use the field map directly (that's what FieldConfig::getBundles() does internally). No biggie.

  10. +++ b/core/modules/migrate_drupal/config/install/migrate.migration.d6_term_node_revision.yml
    @@ -9,6 +9,7 @@ source:
    +  type: type

    Not sure how those changes in migrate_drupal are related - just checking, don't bother explaining if they are ;-)

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new167.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,242 pass(es).
[ View ]
new8.77 KB

Fixed the comment bundles, that was really broken and we didn't notice it as it just loaded a bogus new config object and resulted in a empty title. Also added a simple test.

#111

1: Yeah, true, updated the reference, @todo is still the same.
2. This will also go away in the referenced issue, and we know that right now, comment only supports configurable fields, for example because of the entity bundle hook implementation, I'd prefer to not make any more changes in code that I know will be removed completely in a beta blocker :)
3. Same.
5. Correct. I wasn't sure if it implies too much internal knowledge about how the id is built, but this very specific and field instance config specific anyway. Updated.
6. that issue is RTBC. But yeah, overriding it seems like a hack, because we'd change the definition of the interface and the first argument wouldn't have the same argument anymore. I like the name :)
7. Wow, wall of text. Fun use case that we obviously have no test coverage for :p. Yeah, I was trying to get rid of the load by uuid because that is super slow (the optimization in loadById() is broken as we found out in #2211723: FieldInstance::__construct() loads all field_config entities). I like the deleted check idea, let's try that. We have to avoid loading by UUID *somehow*. field bulk delete test passed, let's see what happens...

8. Updated.
9. Updated.
10. They're no longer strictly required but there's an API to do field type specific massaging of migrated field values in EntityContentBase::import(), but it only runs if it can detect the correct bundle. I tried to make that required, but that turned out to be too complicated, so I reverted that but kept the easy fixes.

While working on 7., I noticed that PHPStorm failed to parse static|null, so I reverted that..

Thanks for the review, feeling more comfortable now to move forward with this, I understand you're OK with the big picture, as there's nothing negative about the issue as a whole here.

Berdir’s picture

StatusFileSize
new169.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,273 pass(es).
[ View ]
new1.5 KB

Ah, #2249113: Use an entity save instead of db_insert() in user_install() landed, but we can revert 50% of the changes now because ContentEntityDatabaseStorage no longer depends on the field.module service :)

fago’s picture

Status:Needs review» Needs work

The instance deleted check makes sense, as well as the other fixes.

+++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php
@@ -231,20 +231,19 @@ class FieldInstanceConfig extends ConfigEntityBase implements FieldInstanceConfi
+    if (!empty($values['deleted'])) {      if ($fields = entity_load_multiple_by_properties('field_config', array('uuid' => $values['field_uuid'], 'include_deleted' => TRUE))) {

oopsie.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new169.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,225 pass(es).
[ View ]
new1.05 KB

Uhm, yeah. fixed.

fago’s picture

Status:Needs review» Reviewed & tested by the community

Thanks. yched's remarks have been addressed and he did not have any general concerns, so I think this is good to go then! If there are other small remarks we can fix them in follow-ups also.

yched’s picture

Yeah, RTBC +1, aside from the minor comment typo below ;-)

This patch is full of yay. Thanks a ton @Berdir !

+++ b/core/modules/comment/comment.module
@@ -104,13 +104,21 @@ function comment_entity_bundle_info() {
+    // @todo: We can not rely on the field map here, so we need tomanually look

tomanually :-)

Berdir’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new170.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,187 pass(es).
[ View ]
new2.4 KB

Found one more call and fixed the comment.

fago’s picture

Status:Needs review» Reviewed & tested by the community

Thanks, changes are good. Back to RTBC then.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed da026cf and pushed to 8.x. Thanks!

diff --git a/core/modules/comment/lib/Drupal/comment/CommentManagerInterface.php b/core/modules/comment/lib/Drupal/comment/CommentManagerInterface.php
index da93586..f302e18 100644
--- a/core/modules/comment/lib/Drupal/comment/CommentManagerInterface.php
+++ b/core/modules/comment/lib/Drupal/comment/CommentManagerInterface.php
@@ -28,7 +28,7 @@
    *   - bundles: The bundles in which the field appears, as an array with entity
    *     types as keys and the array of bundle names as values.
    *
-   * @see field_info_field_map()
+   * @see \Drupal\Core\Entity\EntityManagerInterface::getFieldMap()
    */
   public function getFields($entity_type_id);

diff --git a/core/modules/file/file.module b/core/modules/file/file.module
index 339eac6..7580d9b 100644
--- a/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -7,8 +7,6 @@

use Drupal\Core\Field\FieldDefinitionInterface;
use Drupal\Core\Render\Element;
-use Drupal\field\Entity\FieldConfig;
-use Drupal\field\FieldConfigInterface;
use Drupal\file\Entity\File;
use Drupal\Component\Utility\NestedArray;
use Drupal\Component\Utility\Unicode;
diff --git a/core/modules/forum/forum.module b/core/modules/forum/forum.module
index e8c8a12..1b9b8f8 100644
--- a/core/modules/forum/forum.module
+++ b/core/modules/forum/forum.module
@@ -9,7 +9,6 @@
use Drupal\Component\Utility\Xss;
use Drupal\Core\Entity\EntityInterface;
use Drupal\Component\Utility\String;
-use Drupal\field\Entity\FieldConfig;
use Symfony\Component\HttpFoundation\Request;

/**
diff --git a/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/EntityComment.php b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/EntityComment.php
index dd64262..42d3723 100644
--- a/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/EntityComment.php
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/EntityComment.php
@@ -10,7 +10,6 @@
use Drupal\Core\Entity\EntityManagerInterface;
use Drupal\Core\Entity\EntityStorageInterface;
use Drupal\Core\State\StateInterface;
-use Drupal\field\FieldInfo;
use Drupal\migrate\Entity\MigrationInterface;
use Drupal\migrate\Plugin\MigratePluginManager;
use Drupal\migrate\Row;
diff --git a/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Vocabulary.php b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Vocabulary.php
index fea64bb..d492d7a 100644
--- a/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Vocabulary.php
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Vocabulary.php
@@ -9,7 +9,6 @@

use Drupal\Core\Config\Entity\ConfigEntityBundleBase;
use Drupal\Core\Entity\EntityStorageInterface;
-use Drupal\field\Field;
use Drupal\taxonomy\VocabularyInterface;

/**

Minor clean up on commit

  • Commit da026cf on 8.x by alexpott:
    Issue #2116363 by Berdir, jessebeach | fago: Unified repository of field...
jessebeach’s picture

I published the Change Notice.

xjm’s picture

The change record for https://drupal.org/node/2260037 is a bit confusing; I think it's written as if the issue is still in progress, rather than complete? And it doesn't really help me understand what actually changed. Edit: https://drupal.org/node/2260037/revisions/view/7250591/7250689 helps clear things up, thanks @Berdir!

Status:Fixed» Closed (fixed)

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