This can be considered a followup from #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed.
It has been split out from the patch that I'm going to post to that issue.

Basically, if that issue goes through, there won't be a need for an "active" status anymore, because disabling modules that provide storage or field types will still keep those components present and running in the system. This allows us to remove some code that would no longer be necessary, including the hack we introduced in: #943772: field_delete_field() and others fail for inactive fields.

Files: 
CommentFileSizeAuthor
#41 interdiff.txt5.17 KBamateescu
#41 1503314-41.patch34.33 KBamateescu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1503314-41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#38 interdiff.txt2.28 KBamateescu
#38 1503314-38.patch32.6 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 58,931 pass(es).
[ View ]
#34 interdiff.txt1.38 KBswentel
#34 1503314-34.patch31.59 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 59,106 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#27 1503314-27.patch31.52 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 59,125 pass(es).
[ View ]
#24 1503314-24.patch32.65 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 59,272 pass(es).
[ View ]
#24 interdiff.txt859 bytesswentel
#23 1503314-23.patch32.75 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#23 interdiff.txt3.34 KBswentel
#21 1503314-21.patch32.13 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 59,799 pass(es).
[ View ]
#21 interdiff.txt585 bytesswentel
#19 1503314-19.patch32.08 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#19 interdiff.txt585 bytesswentel
#18 1503314-18.patch31.96 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 59,029 pass(es).
[ View ]
#14 1503314-14.patch32.03 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 58,560 pass(es).
[ View ]
#12 1503314-12.patch30.91 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 58,953 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#10 1503314-10.patch23.26 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#5 1503314-remove-active.patch37.69 KBbojanz
FAILED: [[SimpleTest]]: [MySQL] 35,701 pass(es), 0 fail(s), and 155 exception(s).
[ View ]
#3 1503314-remove-active.patch36.68 KBbojanz
FAILED: [[SimpleTest]]: [MySQL] 35,688 pass(es), 1 fail(s), and 159 exception(s).
[ View ]
#1 1503314-remove-active.patch36.68 KBbojanz
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field/tests/field.test.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new36.68 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field/tests/field.test.
[ View ]

This is basically postponed on the other issue, but let's get some eyes on it.

Status:Needs review» Needs work

The last submitted patch, 1503314-remove-active.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new36.68 KB
FAILED: [[SimpleTest]]: [MySQL] 35,688 pass(es), 1 fail(s), and 159 exception(s).
[ View ]

Let's hope that was the last syntax error.

Status:Needs review» Needs work

The last submitted patch, 1503314-remove-active.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new37.69 KB
FAILED: [[SimpleTest]]: [MySQL] 35,701 pass(es), 0 fail(s), and 155 exception(s).
[ View ]

Removing the testInstanceDisabledEntityType() test as well, since it makes no sense in the post-disabled-modules-fix world (since there won't be any disabled entity types).

Status:Needs review» Needs work

The last submitted patch, 1503314-remove-active.patch, failed testing.

Status:Closed (duplicate)» Active

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

Status:Active» Needs review
StatusFileSize
new23.26 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1503314-10.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new30.91 KB
FAILED: [[SimpleTest]]: [MySQL] 58,953 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Should be better

Status:Needs review» Needs work

The last submitted patch, 1503314-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new32.03 KB
PASSED: [[SimpleTest]]: [MySQL] 58,560 pass(es).
[ View ]

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

StatusFileSize
new31.96 KB
PASSED: [[SimpleTest]]: [MySQL] 59,029 pass(es).
[ View ]

Rerolled because the other issue went in.

Issue tags:+Field API
StatusFileSize
new585 bytes
new32.08 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1503314-19.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new585 bytes
new32.13 KB
PASSED: [[SimpleTest]]: [MySQL] 59,799 pass(es).
[ View ]

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

StatusFileSize
new3.34 KB
new32.75 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

StatusFileSize
new859 bytes
new32.65 KB
PASSED: [[SimpleTest]]: [MySQL] 59,272 pass(es).
[ View ]

Fixing one wrap

Assigned:catch» Unassigned

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 ?

Assigned:Unassigned» catch

Assigning to @catch for #25.

Issue summary:View changes
StatusFileSize
new31.52 KB
PASSED: [[SimpleTest]]: [MySQL] 59,125 pass(es).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 27: 1503314-27.patch, failed testing.

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, 27: 1503314-27.patch, failed testing.

Status:Needs work» Needs review

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() ?

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new31.59 KB
FAILED: [[SimpleTest]]: [MySQL] 59,106 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
new1.38 KB

Status:Reviewed & tested by the community» Needs review

eeuh, woops :)

The last submitted patch, 34: 1503314-34.patch, failed testing.

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

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new32.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,931 pass(es).
[ View ]
new2.28 KB

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 ?

StatusFileSize
new34.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1503314-41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new5.17 KB

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

Status:Needs review» Reviewed & tested by the community

Thanks! Looks good to me.

Title:Remove the concept of active / inactive (field types, storage) from Field APIChange notice: Remove the concept of active / inactive (field types, storage) from Field API
Priority:Normal» Major
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

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.

Status:Active» Needs review

Issue tags:+Missing change record

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.

Status:Needs review» Needs work

The last submitted patch, 41: 1503314-41.patch, failed testing.

Issue tags:+Needs reroll

Issue tags:-Needs reroll

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

Status:Needs work» Needs review

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

Title:Change notice: Remove the concept of active / inactive (field types, storage) from Field APIRemove the concept of active / inactive (field types, storage) from Field API
Priority:Major» Normal
Status:Needs review» Fixed
Issue tags:-Missing change record

Change record looks good! Thank you hairqles!!

Issue tags:-Needs change record

Removed 'Needs change record' tag.