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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz’s picture

Status: Active » Needs review
FileSize
36.68 KB

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.

bojanz’s picture

Status: Needs work » Needs review
FileSize
36.68 KB

Let's hope that was the last syntax error.

Status: Needs review » Needs work

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

bojanz’s picture

Status: Needs work » Needs review
FileSize
37.69 KB

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.

bojanz’s picture

fubhy’s picture

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

catch’s picture

swentel’s picture

Status: Active » Needs review
FileSize
23.26 KB

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
30.91 KB

Should be better

Status: Needs review » Needs work

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

swentel’s picture

Status: Needs work » Needs review
FileSize
32.03 KB

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

amateescu’s picture

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?

swentel’s picture

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

amateescu’s picture

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

amateescu’s picture

FileSize
31.96 KB

Rerolled because the other issue went in.

swentel’s picture

Issue tags: +Field API
FileSize
585 bytes
32.08 KB

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
585 bytes
32.13 KB

So apparently not :)

yched’s picture

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

swentel’s picture

FileSize
3.34 KB
32.75 KB

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

swentel’s picture

FileSize
859 bytes
32.65 KB

Fixing one wrap

yched’s picture

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 ?

yched’s picture

Assigned: Unassigned » catch

Assigning to @catch for #25.

swentel’s picture

Issue summary: View changes
FileSize
31.52 KB

reroll after the upgrade path tests were removed.

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

yched’s picture

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

swentel’s picture

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.

swentel’s picture

Status: Needs work » Needs review

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

yched’s picture

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

swentel’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
31.59 KB
1.38 KB
swentel’s picture

Status: Reviewed & tested by the community » Needs review

eeuh, woops :)

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

yched’s picture

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

amateescu’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
32.6 KB
2.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.

yched’s picture

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 ?

yched’s picture

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 ?

amateescu’s picture

FileSize
34.33 KB
5.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

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Looks good to me.

catch’s picture

Title: Remove the concept of active / inactive (field types, storage) from Field API » Change 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.

jetmartin’s picture

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.

jetmartin’s picture

Status: Active » Needs review
xjm’s picture

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

martin107’s picture

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

Status: Needs review » Needs work

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

martin107’s picture

Issue tags: +Needs reroll
yched’s picture

Issue tags: -Needs reroll

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

hairqles’s picture

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

hairqles’s picture

Status: Needs work » Needs review
swentel’s picture

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

jessebeach’s picture

Status: Needs review » Needs work

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

hairqles’s picture

Status: Needs work » Needs review

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

jessebeach’s picture

Title: Change notice: Remove the concept of active / inactive (field types, storage) from Field API » Remove 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!!

jessebeach’s picture

Issue tags: -Needs change record

Removed 'Needs change record' tag.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.