Remove field_info_*()

Given that #2167267: Remove deprecated field_attach_*_view() and #2095195: Remove deprecated field_attach_form_*() have been rescoped to remove the functions in the same patch as ensuring their replacements are fully functional, why do we want to do this one as a followup to #2116363: Unified repository of field definitions (cache + API) rather than in that same issue? Wouldn't doing this as part of that issue help ensure that what's done in that issue actually works?

So I was asking for deprecated function removal in dedicated patches, because we had several cross-commits in the same couple of weeks, where a patch was committed using something not yet even or quite recently deprecated, while another patch refactored then removed the thing it was using.

Marking stuff, then removing it separately, meant there was at least a fighting chance to avoid breaking HEAD.

On the other hand, it's a bit unwieldy doing it like that, and I don't really care that much either way as long as it works.

We'll see when we get there, but in that specific case but I suspect it might be a bit of work for #2116363: Unified repository of field definitions (cache + API) to introduce a new registry *and* convert all current uses of field_info_*() / Fieldinfo::getXxx(). Sounds like it easily could bump the patch size over the "painfull to reroll & review" line.

It's great if patches like #2167267: Remove deprecated field_attach_*_view() can get a "permission" to convert calls + remove old functions in one fell swoop, but it's greater if patch authors can still choose to go with "provide new API + BC layer" & "convert existing & remove BC" in separate patches ?

but it's greater if patch authors can still choose to go with "provide new API + BC layer" & "convert existing & remove BC" in separate patches ?

Yeah I don't see why we'd stop doing that, it's the only way to make patches reviewable really. I think that's fine if we sort out the @removebeforerelease tag and apply that to stuff that's going to be removed.

Cool. Just thought I'd ask, but #5 is a good answer, thanks.

What about field_info_fields() and field_info_cache_clear()?

Unpostponed!

Bye, bye.

field_info_cache_clear() is still there, wanted to see if this is passing first, looking into that next...

This removes field_info_cache_clear() and either updates or removes the calls to it. Many field tests now directly load the field entity with the new loadByName(), so we don't need to clear any additional caches. Let's see how well this works, installer works...

Missed a few calls that were using service('field.info')...

Ups, we still need to clear the field definitions cache in field_cache_clear() :)

Also managed to debug FieldInstanceCrudTest first, which seems like an obvious choice but ended up debugging a completey different problem, that it created fields on non-existing bundles, with the new FieldMap() approach, the second bundle was never added to the field map, leading it to delete the field when there were still instances of it.

Fixing some nasty caching issues. We now actually rely on the entity bundles and the field map in some cases, so we need to make sure that they're properly rebuilt/updated

This should fix the last two :)

Great to see this go away :-)

Do we still have a use for field_cache_clear() ?
It makes little sense in field.module, that is only supposed to be about managing configurable fields.
It now only clears stuff related to the EntityManager, so maybe it should be merged with entity_info_cache_clear() ?

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -605,6 +605,7 @@ public function clearCachedFieldDefinitions() {
     $this->fieldDefinitions = array();
     $this->fieldStorageDefinitions = array();
     $this->fieldMap = array();
+    $this->bundleInfo = NULL;

Minor, but can't we initialize bundleInfo to array() like the other "static cache" members above ?

I actually want to remove entity_info_cache_clear() too :)

We can possibly merge it into clearCachedFieldDefinitions(), the typed data manager part should be there anyway and the field cache is not in Core\Entity, even though it's still a "configurable field" cache until #597236: Add entity caching to core is resolved (working on that one right now).

NULL vs. array, yeah, maybe, it had that initially and it didn't work because getAllBundleInfo() uses !isset(), I can probably change that to !empty() because we don't care about caching if you don't have any entity types, which is going to be pretty hard to achieve outside of unit tests ;)

Killed them both :)

Guessed a few times which cache clear we need, let's see if this works.

Fixing the unit test fails and refactoring the cache clearing a bit, let's see if we can do the cached value caching through a cache tag.

I tested the installer every time.. except when it mattered :)

And now I uploaded the wrong patch. Oh my.

Yay at all the cleanups and removals !

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -605,7 +612,13 @@ public function clearCachedFieldDefinitions() {
     $this->fieldDefinitions = array();
     $this->fieldStorageDefinitions = array();
     $this->fieldMap = array();
+    $this->bundleInfo = array();
+    $this->displayModeInfo = array();
+    $this->extraFields = array();
     Cache::deleteTags(array('entity_field_info' => TRUE));
+    // The typed data manager statically caches prototype objects with injected
+    // definitions, clear those as well.
+    $this->typedDataManager->clearCachedDefinitions();
   }

This now does much more than just clearing field definitions.
--> clearCaches() ? clearCachedData() ?

Also, looking at the rest of the diff for stuff (mostly tests ?) that directly calls the clearCachedDdefinitions() method inherited from TypedData (another unfortunate TypedData name steal :-)), it's a bit obscure in which cases it's enough to simply call that one.
Not sure what we can do about this here though.

Not sure about the name, agreed it's more than field definitions now, so maybe just clearCaches() might work, it's also more than data. On the other side, could be confusing that clearCaches doesn't clear the definitions cache?

Not sure I understand the second part, but you can't blame typed data for clearCachedDefinitions(), that's plugin API, not typed data :)

Oops, I shouldn't post reviews when I can't fire my phpstorm. Apologies to TypedData - yeah, Plugin API is guilty of the same lack of namespacing in its method names. And IIRC it's even my bad for that specific method :-/.

OK, never mind my second point then. Once you figure clearCachedDefinitions() is about *plugin* definitions, then I guess it's clearer when to call that one vs clearCaches() :-)

So yeah, regarding the name clearCaches() - right, it clears evrything except plugin definitions.
Then maybe :
- add a clearCachedData() for the (yet to come) entity cache + the render cache
- rename the current clearCachedFieldDefinitions() to clearCachedInfo(), and have it call clearCachedData() at the end - you need to clear the data when you clear the metadata...

?

@yched: You are too used to blaming TypedData ;-)
@berdir: Great work as always, here a review:

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -605,7 +612,13 @@ public function clearCachedFieldDefinitions() {
   +    $this->bundleInfo = array();
   

   Why do we have to clear the bundle cache when the field cache is cleared? That does not seem logical. I guess it's due to comment module, but then this seems to be comment module's business.
   What opens an interesting question, but I think it's the right question to ask: How can a module react on field caches being cleared and clear depending caches?
   cache tags solve the issue for permanent caches, but how is it solved for static caches? Does that justify a hook for that? I mean there will probably be quite some caches that need to be rebuild once field caches are changed.

   I think the method name would be fine if bundleinfo could be cleared elsewhere, as then it's really about clearing field definitions + everything that depends on it. So for now we could just comment why bundleinfo is cleared here and keep the method name imho.

2. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Tests/ContentTranslationSettingsTest.php
   @@ -171,8 +169,8 @@ protected function assertSettings($entity_type, $bundle, $enabled, $edit) {
   +    \Drupal::entityManager()->clearCachedFieldDefinitions();
   +    \Drupal::entityManager()->clearCachedDefinitions();
   

   clearCachedDefinitions() encapsulates the first one already.

1. Hm, if we move bundle reset out, then where are we going to do that? New method?

Right now, bundle info cache clear happens as part of EntityManager::clearCachedDefinitions(), which is called through entity_invoke_bundle_hook(), which calls entity_info_cache_clear() which is called in NodeType::postSave(). (yeah, fun)

Which means that currently, we clear entity type definitions every time we actually want to clear the bundle cache... so maybe a separate method for that would make sense?

You're right that bundles are the only thing that is not somehow related to field definitions, so then it would actually make sense again...

What about clearCachedBundles() ?

Then we have:
- clearCachedDefinitions() (clears everything)
- ClearCachedBundles() (just bundles)
- clearCachedFieldDefinitions() (clears everything that is or depends on field definitions. Right now, that includes all field/entity caches, but that might go away together with PrepareCacheInterface, if we manage to pull that off...)

Depending static caches is an interesting problem, #2269025: CommentManager::getAllFields() should have a static cache is an example, I'm surprised that just worked, possibly because our tests don't cover the cases that could make it fail (like adding more than one comment field in the same request) but we never had a solution for that so far, so they probably just need to listen to events that are relevant for them and clear their caches manually (for that issue, it would be new/changes comment fields).

2. Yeah, it didn't when I added that, will update.

As discussed on IRC, yeah - ClearCachedBundles() (just bundles) makes a lot of sense to separate out.

Ok, added that.

@yched: You OK with this?

Yup, feels right.

+++ b/core/modules/field/field.module
@@ -249,13 +249,6 @@ function field_entity_bundle_delete($entity_type, $bundle) {
- * Implements hook_rebuild().
- */
-function field_rebuild() {
-  \Drupal::entityManager()->clearCachedFieldDefinitions();
-}

Yay, was wondering about that one :-)

Patch works for me if green.

Interesting test fails.

- Bundle cache clear needs to clear typed data definitions as well.
- Display modes need to have the entity_field_info cache tag
- Added a new cache tag for bundles.
- comment never called entity_invoke_bundle_hook() for new bundles, so we didn't clear the bundle cache for new comment fields.

Ah, I forgot to explain one change. I had to add a manual cache clear in one case, but I think that is valid and consistent with other cases.

We add a new field through the UI and then create a new node with that field. That only worked because previously, we cleared all entity caches in entity_invoke_bundle_hook(), which was called *after* the node body field was added, and nothing happened to fill the cache again in the meantime. This is a test-only scenario.

We add a new field through the UI and then create a new node with that field. That only worked because previously, we cleared all entity caches in entity_invoke_bundle_hook(), which was called *after* the node body field was added, and nothing happened to fill the cache again in the meantime. This is a test-only scenario.

But doesn't that mean that the cache clear logic is flawed, i.e. it does not clear all static caches as needed?

No, it is not flawed. This is a test problem, adding a new field obviously clears the caches. But it can't affect the static cache in the test parent.

The difference to before is that the old cache clear resulted in an empty field cache *before* we added the field in the test, so there was no static cache. This was only the case because creating a node type cleared the field definitions cache 2-4 times, now we only do it once (after adding the body field) and the entity display then loads it again.

I see, thanks for explaining. Patch looks good to me then and yched +1ed on it already, so setting RTBC.

Awesome.

Chatted with @berdir in IRC -

FieldInfoTest ::testSettingsInfo() and FieldInfoTest::testWidgetDefinition() are imho the only two that have no replacement, not sure what to do with them?M

Well we need to find them a new home.

+++ b/core/modules/comment/comment.module
@@ -257,6 +258,16 @@ function comment_field_config_delete(FieldConfigInterface $field) {
+ * Implements hook_ENTITY_TYPE_delete() for 'field_config'.
+ */
+function comment_field_config_insert(FieldConfigInterface $field) {

Docblock / function mismatch

Ok, re-added those tests. Also didn't apply anymore, @class_resolver conflicted.

1. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldTypePluginManagerTest.php
   @@ -0,0 +1,65 @@
   +
   +  public static function getInfo() {
   

   Missing inheritdoc.

2. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldTypePluginManagerTest.php
   @@ -0,0 +1,65 @@
   +   * Returns field info definition.
   +   */
   +  protected function getExpectedFieldTypeDefinition() {
   

   hm, that method name does not make any sense more now. Maybe, we could just add some coverage to check that those field types are found as well?

3. +++ b/core/modules/field/lib/Drupal/field/Tests/WidgetManagerTest.php
   @@ -0,0 +1,33 @@
   +  public static function getInfo() {
   

   inheritdoc?

We actually changed that recently and getInfo() should have @inheritdoc too, just copied that from the existing class.

2. Not sure what doesn't match, the description is wrong but the method name seems to be fine?

ad 2: The method says "getExpectedFieldTypeDefinition", but it does not use the definition to compare it with something expected. Instead of that it makes use of those definitions to define which field types are tested.

Ok, added @inheritdoc and removed that method, just kept a list of field types.

Thanks, back to RTBC then.

Committed 00339b3 and pushed to 8.x. Thanks!

Weeehaa ! This kind of marks the point where the old Field API is "mostly" merged into the new one. Congrats, folks.
(well, arguably #2144263: Decouple entity field storage from configurable fields is about another remnant)

+ field.module is getting skinnier and skinnier, as it should.