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.
Comment | File | Size | Author |
---|---|---|---|
#41 | interdiff.txt | 5.17 KB | amateescu |
#41 | 1503314-41.patch | 34.33 KB | amateescu |
#38 | interdiff.txt | 2.28 KB | amateescu |
#38 | 1503314-38.patch | 32.6 KB | amateescu |
#34 | interdiff.txt | 1.38 KB | swentel |
Comments
Comment #1
bojanz CreditAttribution: bojanz commentedThis is basically postponed on the other issue, but let's get some eyes on it.
Comment #3
bojanz CreditAttribution: bojanz commentedLet's hope that was the last syntax error.
Comment #5
bojanz CreditAttribution: bojanz commentedRemoving 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).
Comment #7
bojanz CreditAttribution: bojanz commentedThis will happen in #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed, active patch in #2003966: Helper issue for #1199946 : Disabled modules are broken beyond repair.
Comment #8
fubhy CreditAttribution: fubhy commentedThis 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).
Comment #9
catchfield_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.
Comment #10
swentel CreditAttribution: swentel commentedLet's see if we can remove active/inactive - field_system_info_alter() is a different beast I guess.
Comment #12
swentel CreditAttribution: swentel commentedShould be better
Comment #14
swentel CreditAttribution: swentel commentedShould be ready for review, and almost good to go too I think.
Comment #15
amateescu CreditAttribution: amateescu commentedHere's a small review :)
I think this 'if' block should be removed completely as we cannot (should not?) create an instance of a deleted field..
Does this comment really need to be removed entirely?
Comment #16
swentel CreditAttribution: swentel commented1. 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 :)
Comment #17
amateescu CreditAttribution: amateescu commentedDoh, I knew it looked familiar. I'm getting old :)
Comment #18
amateescu CreditAttribution: amateescu commentedRerolled because the other issue went in.
Comment #19
swentel CreditAttribution: swentel commentedrerolled + 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.
Comment #21
swentel CreditAttribution: swentel commentedSo apparently not :)
Comment #22
yched CreditAttribution: yched commentedYEAHHHROCK'NROLL!!!!!!!!
Nitpick, but we could s/non deleted/non-deleted/ while we're here.
We could put that back on one line.
--> "that are have been deleted"
+ the last line could also be rewrapped
Rewrap
Rewrap
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()
Comment #23
swentel CreditAttribution: swentel commentedFixed remarks. Active was already removed from the exportProperties. Wondering whether we still need status ?
Comment #24
swentel CreditAttribution: swentel commentedFixing one wrap
Comment #25
yched CreditAttribution: yched commentedYay. 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 ?
Comment #26
yched CreditAttribution: yched commentedAssigning to @catch for #25.
Comment #27
swentel CreditAttribution: swentel commentedreroll after the upgrade path tests were removed.
@yched - maybe we'll get a faster core committer response if set this to RTBC ? :)
Comment #28
yched CreditAttribution: yched commented@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
Comment #30
swentel CreditAttribution: swentel commented27: 1503314-27.patch queued for re-testing.
Comment #32
swentel CreditAttribution: swentel commented27: 1503314-27.patch queued for re-testing.
Comment #33
yched CreditAttribution: yched commentedMinor: 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 ?
Then this could switch to field_info_field() ?
Comment #34
swentel CreditAttribution: swentel commentedComment #35
swentel CreditAttribution: swentel commentedeeuh, woops :)
Comment #37
yched CreditAttribution: yched commentedFrom the interdiff :
Heh, I rather meant keep include_inactive away and remove include_deleted too ;-)
Comment #38
amateescu CreditAttribution: amateescu commentedI'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 fromCommentManager::getFields()
trying to get fields for config entity types, which is a little weird to say the least, fixed that as well.Comment #39
yched CreditAttribution: yched commentedNot 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 ?
Comment #40
yched CreditAttribution: yched commentedWill 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 ?
Comment #41
amateescu CreditAttribution: amateescu commentedTurns out it was a stray config static cache. Added a reset call in field_info_cache_clear() and reverted the changes to CommentManager.
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
Comment #42
yched CreditAttribution: yched commentedThanks! Looks good to me.
Comment #43
catchCommitted/pushed to 8.x, thanks! Needs a short change notification.
Comment #44
jetmartin CreditAttribution: jetmartin commentedHI.
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.
Comment #45
jetmartin CreditAttribution: jetmartin commentedComment #46
xjmTagging "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. :)
Comment #47
martin107 CreditAttribution: martin107 commented41: 1503314-41.patch queued for re-testing.
Comment #49
martin107 CreditAttribution: martin107 commentedComment #50
yched CreditAttribution: yched commentedDoesn't need reroll, code has been committed already. The issue is left open for the missing change record.
Comment #51
hairqles CreditAttribution: hairqles commentedDid some dig around the issue and started to prepare a change record for this issue.
https://drupal.org/node/2224569
Comment #52
hairqles CreditAttribution: hairqles commentedComment #53
swentel CreditAttribution: swentel commented@hairqles - could you look at comment #44 - it has more info
Comment #54
jessebeach CreditAttribution: jessebeach commentedMarking needs work for the Change Notice based on swentel's comment from #53.
Comment #55
hairqles CreditAttribution: hairqles commentedUpdated the Change record according to @swentel comment.
http://drupal.org/node/2224569
Comment #56
jessebeach CreditAttribution: jessebeach commentedChange record looks good! Thank you hairqles!!
Comment #57
jessebeach CreditAttribution: jessebeach commentedRemoved 'Needs change record' tag.