Unified repository of field definitions (cache + API)

Retitling a bit

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 ?

Per discussion with catch, this is a beta blocker.

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 ?

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.

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

Filed followup #2167167: Remove field_info_*().

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

#2114707: Allow per-bundle overrides of field definitions is in!

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?

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.

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.

Notes from the hangout today: https://docs.google.com/document/d/1F3vFhI6b20ilc0wxB1D8Akxq2tkjtkRwXwb2...

Now just postponed on #2226197: Introduce FieldStorageDefinitionInterface in the Entity Field API

Unpostponed!

no really

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

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?

Adding related tasks.

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

@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...

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

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

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.

Ups.

Fixing tests.

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.

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

That was obviously the wrong interdiff.

Fixing deleted fields and entity queries..

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)

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.

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

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.

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.

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

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.

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?

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

Re-roll.

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

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.

Updated issue summary.

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

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

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";

65: cleanup-field-info-stuff-2116363-65.patch queued for re-testing.

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

Fixing the migrate test fails...

Re-roll.

Re-roll.

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

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.

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

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

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 :

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 :

  $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.

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

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?

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?

@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.

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.

Small comment cleanup.

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.

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:

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.

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

Another beta blocker is now postponed on this: #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form

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

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.

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.

The Drop moves fast today.

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

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.

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 field manager, we can continue to discuss there.

Fine with disussing single-field methods in a followup.

Re :

- (myself)

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.

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.

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 ?

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

Thanks for the explanations, make sense :-)

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

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.

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

Uhm, yeah. fixed.

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

Found one more call and fixed the comment.

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

I published the Change Notice.

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!