Second issue from the DAT initiative :)

Found this stray @todo while working on #1503314: Remove the concept of active / inactive (field types, storage) from Field API:

function field_info_cache_clear() {
...

  // @todo: Remove this when field_attach_*_bundle() bundle management
  // functions are moved to the entity API.
  entity_info_cache_clear();

...
}

Seems like this should've been handled in #1374116: Move bundle CRUD API out of Field API

CommentFileSizeAuthor
#7 2148723-7.patch1.55 KBamateescu
#2 2148723.patch673 bytesamateescu
#1 2148723.patch0 bytesamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Active » Needs review
FileSize
2148723.patch0 bytes

Let's see what breaks.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
FileSize
2148723.patch673 bytes

Ahem.

Status: Needs review » Needs work

The last submitted patch, 2: 2148723.patch, failed testing.

yched’s picture

yched CreditAttribution: yched commented

We currently still need that entity_info_cache_clear() call, not for the reason mentioned in the @todo :-/, but because configurable field/instance definitions are currently cached in both field.module's FieldInfo and EntityManager::getFieldDefinitions()...

That's for #2116363: Unified repository of field definitions (cache + API) to sort out.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Right, so.. should we change this patch to only add a reference to that issue or just post a comment there and close this one?

yched’s picture

yched CreditAttribution: yched commented

Hmm...

What would make sense to cleanup in field_info_cache_clear() right now would be:
- the three drupal_static_reset() refer to old/obsolete stuff, they could be removed (not fully sure about 'field_available_languages', but worth a try
- typedData()->clearCachedDefinitions() & 'plugin.manager.field.field_type'->clearCachedDefinitions() should move to entity_info_cache_clear(), now that the notion of "field types" belongs to the Core entity/field system.
- the @todo above entity_info_cache_clear() should indeed point to #2116363: Unified repository of field definitions (cache + API)
- FieldInfo()->flush() stays

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Title: field_info_cache_clear() shouldn't call entity_info_cache_clear() anymore? » Clean up field_info_cache_clear()
Status: Needs work » Needs review
Issue tags: -DAT
FileSize
2148723-7.patch1.55 KB

field_available_languages() seems to be called only in two tests which should probably be refactored at some point. So yeah, let's try.

Since we're not fixing the @todo anymore, I'll remove the tag :(

Status: Needs review » Needs work

The last submitted patch, 7: 2148723-7.patch, failed testing.

yched’s picture

yched CreditAttribution: yched commented 
+++ b/core/includes/entity.inc
@@ -45,6 +45,13 @@ function entity_info_cache_clear() {
+  // Clear the config static cache.
+  \Drupal::service('config.factory')->reset();

+++ b/core/modules/field/field.info.inc
@@ -27,19 +27,9 @@
-  \Drupal::service('config.factory')->reset();

Probably doesn't explain the fails, but shouldn't this stay in field_info_cache_clear() for now ? (field_info is about the things stored in config)

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs work » Postponed

I looked a bit at the test failures and it seems that typed_data and field_type cache clears are not yet ready to be moved, and 'field_available_languages' cannot to be removed as well.

Since a patch that just removes the other two static resets doesn't bring too much value at this point, I'll just postpone this cleanup.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented

Since we are at Beta1 is it a good time for the cleanup?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Postponed » Closed (duplicate)

No, too late unfortunately. Because this function no longer exists :)

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented

Thanks for closing the issue.