Remove the concept of active / inactive (field types, storage) from Field API

This still needs a clean-up and follow-up work for figuring out what to do instead of the ugly stuff that is happening in field_system_info_alter() etc.

I just posted the patch over at #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed but that does not fully cover the problem we have with field, field status, field purge, etc. and especially not the scenario of 'soft-dependencies' that field module tries to solve in field_system_info_alter() (which we simply don't have an API for, hence the hack).

field_system_info_alter() could go away if we do #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall.

Let's see if we can remove active/inactive - field_system_info_alter() is a different beast I guess.

Should be better

Should be ready for review, and almost good to go too I think.

Here's a small review :)

1. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstance.php
   @@ -245,9 +245,8 @@ public function __construct(array $values, $entity_type = 'field_instance') {
   -      // field_info_field_by_id() will not find the field if it is inactive.
          if (!$field) {
   -        $field = current(field_read_fields(array('uuid' => $values['field_uuid']), array('include_inactive' => TRUE, 'include_deleted' => TRUE)));
   +        $field = current(field_read_fields(array('uuid' => $values['field_uuid']), array('include_deleted' => TRUE)));
   

   I think this 'if' block should be removed completely as we cannot (should not?) create an instance of a deleted field..

2. +++ b/core/modules/field/lib/Drupal/field/FieldInfo.php
   @@ -190,19 +190,15 @@ public function getFieldMap() {
   -      // Filter out instances of inactive fields, and instances on unknown
   -      // entity types.
   

   Does this comment really need to be removed entirely?

1. We need this to construct the field object when we are purging field data - edit - I think, I'll have to double check.
2. this is the same as in #2059965: Should FieldInfo::getFieldMap() filter data about unknown entity types actually which you RTBC'd :)

Doh, I knew it looked familiar. I'm getting old :)

Rerolled because the other issue went in.

rerolled + testing whether hook_modules_(un)installed() hook can be removed or not.

I also noticed that field_sync_status provided the 'module' relying on provider, however that didn't fail before, so it looks fine, but not sure if we're missing something.

So apparently not :)

YEAHHHROCK'NROLL!!!!!!!!

1. +++ b/core/modules/field/field.deprecated.inc
   @@ -159,7 +159,7 @@ function field_info_formatter_settings($type) {
   - * The function only returns active, non deleted fields.
   + * The function only returns non deleted fields.
   

   Nitpick, but we could s/non deleted/non-deleted/ while we're here.

2. +++ b/core/modules/field/field.deprecated.inc
   @@ -194,9 +194,8 @@ function field_info_field_map() {
   + *   non-deleted field. For deleted fields, use
   + *   field_info_field_by_id().
   

   We could put that back on one line.

3. +++ b/core/modules/field/field.deprecated.inc
   @@ -364,8 +360,7 @@ function field_read_field($entity_type, $field_name, $include_additional = array
     *   The default behavior of this function is to not return fields that are
   - *   inactive or have been deleted. Setting
   - *   $include_additional['include_inactive'] or
   + *   have been deleted. Setting
     *   $include_additional['include_deleted'] to TRUE will override this behavior.
   

   --> "that are have been deleted"
   + the last line could also be rewrapped

4. +++ b/core/modules/field/field.deprecated.inc
   @@ -405,8 +397,7 @@ function field_read_fields($conditions = array(), $include_additional = array())
   + *   been deleted. Setting
     *   $include_additional['include_deleted'] to TRUE will override this behavior.
   

   Rewrap

5. +++ b/core/modules/field/field.deprecated.inc
   @@ -429,8 +420,7 @@ function field_read_instance($entity_type, $field_name, $bundle, $include_additi
   + *   have been marked deleted. Setting
     *   $include_additional['include_deleted'] to TRUE will override this behavior.
   

   Rewrap

6. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstance.php
   @@ -242,9 +242,8 @@ public function __construct(array $values, $entity_type = 'field_instance') {
   -      // field_info_field_by_id() will not find the field if it is inactive.
          if (!$field) {
   -        $field = current(field_read_fields(array('uuid' => $values['field_uuid']), array('include_inactive' => TRUE, 'include_deleted' => TRUE)));
   +        $field = current(field_read_fields(array('uuid' => $values['field_uuid']), array('include_deleted' => TRUE)));
   

   Then it looks like we could completely remove the if () {field_read_fields())} code branch, since field_info_field_by_id() can return deleted fields.

+ we should also remove 'active' in the list of properties saved in \Drupal\field\Entity\Field::getExportProperties()

Fixed remarks. Active was already removed from the exportProperties. Wondering whether we still need status ?

Fixing one wrap

Yay. This looks good.

'active' / getExportProperties() : indeed, sorry, I guess I was looking at the wrong place.

The one thing that still leaves me thinking is sites coming from D7. The current field upgrade path creates CMI records for every record in the field_config table, including those about field types that are unknown on the D8 site - e.g field type module hasn't been ported yet.
With the current patch, this would lead to straight fatal errors about unknown class or something (the error you get when trying to instantiate a plugin for an unknown plugin_id).

I guess we could still preserve a layer of safety by having the FieldInfo methods (which are the ones that provide the information used for actual work on entities) read from config, but only take into account and cache and return the fields for which the field type is known.

In this case, what the patch would end up doing is:
- remove the API around the notion of "disabled fields (field_read_fields(include_inactive)...)
- remove the tracking of the 'active' status into config records, and replace it by runtime filtering on cache rebuilds - means overhead on cache rebuild, but that should be negligible (one trivial "if" per field / instance).
Which seems fine by me ?

@catch, @alexpott, what do you think ?

Assigning to @catch for #25.

reroll after the upgrade path tests were removed.

@yched - maybe we'll get a faster core committer response if set this to RTBC ? :)

@swentel: yes, sounds like a reasonable assumption ;-)
Will come back at this after I'm done with the long overdue review of #2047229: Make use of classes for entity field and data definitions

27: 1503314-27.patch queued for re-testing.

27: 1503314-27.patch queued for re-testing.

1. +++ b/core/modules/comment/comment.install
   @@ -49,13 +48,11 @@ function comment_install() {
      $fields = entity_load_multiple_by_properties('field_entity', array(
        'type' => 'comment',
   -    'include_inactive' => TRUE,
        'include_deleted' => FALSE,
      ));
      foreach ($fields as &$field) {
        $instances = entity_load_multiple_by_properties('field_instance', array(
          'field_uuid' => $field->uuid,
   -      'include_inactive' => TRUE,
          'include_deleted' => FALSE,
        ));
   

   Minor: this is now using the default value for 'include_deleted', meaning we could remove that param in the call.
   More importantly, the phpdoc for _comment_get_comment_fields() mentions something a bout "we need this because of inactive fields", meaning it looks like we are removing the reason why the function exists to begin with...

   Well, that function is really weird anyway:-/. Opened #2143589: _comment_get_comment_fields() is weird, costly and unused ?

2. +++ b/core/modules/forum/forum.install
   @@ -21,7 +21,7 @@ function forum_install() {
   +  if (!field_read_field('node', 'taxonomy_forums')) {
   

   Then this could switch to field_info_field() ?

eeuh, woops :)

From the interdiff :

+++ b/core/modules/comment/comment.install
@@ -48,11 +48,13 @@ function comment_install() {
     'type' => 'comment',
+    'include_inactive' => FALSE,
     'include_deleted' => FALSE,
   ));
   foreach ($fields as &$field) {
     $instances = entity_load_multiple_by_properties('field_instance', array(
       'field_uuid' => $field->uuid,
+      'include_inactive' => FALSE,
       'include_deleted' => FALSE,
     ));

Heh, I rather meant keep include_inactive away and remove include_deleted too ;-)

I've reverted all changes to _comment_get_comment_fields() because that function is going away in #2143589: _comment_get_comment_fields() is weird, costly and unused ? so let's avoid any unnecessary conflict. The test fail above was coming from CommentManager::getFields() trying to get fields for config entity types, which is a little weird to say the least, fixed that as well.

Not sure about the changes to CommentManager, smells like something is wrong somewhere else.

It seems weird that we now have to patch CommentManager::getAllFields() while the previous patches didn't have to ?

Also, even though running CommentManager::getAllFields() on a config entity does look like something that shouldn't happen in the first place, the fact that it now triggers warnings in Fieldinfo::getFieldMap() looks like something is wrong in there when we build the field map for an entity type that has no field ?

Will need a reroll after #2115291: Field types must use as provider its own module instead of Core when are defined in hook_field_info_alter()...
Looks like the test added there becomes completely moot after this ?

Not sure about the changes to CommentManager, smells like something is wrong somewhere else.

Turns out it was a stray config static cache. Added a reset call in field_info_cache_clear() and reverted the changes to CommentManager.

Will need a reroll after #2115291: Field types must use as provider its own module instead of Core when are defined in hook_field_info_alter() ...
Looks like the test added there becomes completely moot after this ?

I think so too, reverted the test changes from that patch.

Also opened a separate issue for CommentManager::getFields(): #2148709: CommentManager::getFields() should not try to get fields for config entity types

Thanks! Looks good to me.

Committed/pushed to 8.x, thanks! Needs a short change notification.

HI.

You will find bellow a short change record draft. sorry if my english is not perfect.
I hope this help.

Summary :

Remove the concept of active / inactive (field types, storage) from Field API.
There won't be a need of active status anymore.

Impacts for module developers :

The fields YML files no more declare an 'active:1' property.

Function field_info_formatter_settings() will now return non-deleted fields instead of active and non deleted fields.

Function field_read_field() and field_read_instance(), will now return all fields who are not deleted by default.
To override this behavior the $include_additional parameter will no more use $include_additional['include_inactive'] argument but only $include_additional['include_deleted'] set to true.

function field_rebuild() and field_modules_installed() will clear field cache.

The field_sync_field_status() functions is no more useful and has been removed.

The field class has no more an $active public property.

The function loadByProperties(), in FieldInstanceStorageController, will no more use the property 'field.active' in $conditions argument.

The function loadByProperties(), in FieldStorageController, will now return all fields who are not deleted by default.
To override this behavior the $conditions parameter will no more use $conditions['include_inactive'] argument but only $conditions['include_deleted'] set to true.

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

#44 looks like a good start. It'd be great if someone knowledgeable about the D8 field API could give it a review and help get it published. We can even use the new change record draft feature to iterate on it. :)

41: 1503314-41.patch queued for re-testing.

Doesn't need reroll, code has been committed already. The issue is left open for the missing change record.

Did some dig around the issue and started to prepare a change record for this issue.
https://drupal.org/node/2224569

@hairqles - could you look at comment #44 - it has more info

Marking needs work for the Change Notice based on swentel's comment from #53.

Updated the Change record according to @swentel comment.
http://drupal.org/node/2224569

Change record looks good! Thank you hairqles!!

Removed 'Needs change record' tag.