Updated: Comment #10
Problem/Motivation
While #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API concerns the issue of default configuration, this issue is about configuration dependencies when a module is uninstalled. (#2082117: Install default config only when the owner and provider modules are both enabled covers installation.)
During lengthy IRC and discussion at Prague with catch, beejeebus, alexpott, mtift, and others, we came to a general consensus that we should create an API that would allow Module A to declare that it has config entities that depend on Module B. When Module B is being uninstalled this API will be used to inform the user that uninstallation will result in their removal.
Discussion points raised in Prague:
For dual-ownership config entities, what should happen on uninstall? Three options:
- Allow user to back out or delete all your things
- Disallow uninstall with soft dependencies
- Just do nothing and let the site break
If we allow deleting all the things we need:
- A confirmation screen listing everything that will be deleted
- We should have a UI to batch deletions for (e.g.) field deletions
- We should have an API to declare soft dependencies; it should not be in field_system_info()
- Bundles are not formalized, so field API can't implement this dependency management. Can the entity type provider, e.g. node? There can be any number of bundle providers dependent on one entity type provider.
- Also would need to create a robust batching API.
We also need to consider if deleting the forum node type also implies those field instances are being deleted? And do we need to tell the user? (we should provide a summary, e.g. this will delete the node type and N nodes)
Proposed resolution
Store dependencies in the configuration entity files in a new dependencies key. For example, field.instance.node.article.body
dependencies:
entity:
- 'field.field.node.body'
And the views.view.frontpage
dependencies:
module:
- node
Multiple dependencies are possible - for example entity.view_display.node.article.teaser
(in the standard profile)
dependencies:
entity:
- entity.view_mode.node.teaser
- field.instance.node.article.body
- field.instance.node.article.field_image
- field.instance.node.article.field_tags
- node.type.article
module:
- image
- taxonomy
- text
Introduce a ConfigDependencyManager that is able to read configuration files and construct a graph to determine dependencies between configuration entities or determine which entities are dependent on a specific extension (module or theme).
What dependencies do config entities have
Config entity type | Dependencies |
---|---|
block | The module that provides the plugin. The theme. |
breakpoint | if the source is theme or module it depends on that. Currently handled by the module - all this code can be removed (yay!). |
breakpoint_group | the breakpoint entities, if the source is theme or module that too. Currently handled by the module - all this code can be removed (yay!). |
contact_category | n/a |
custom_block_type | n/a |
editor | The filter format it is attached to. The module that supplies the editor plugin. Follow up to convert Editor entity to EntityWithPluginBagInterface and therefore the editor dependency will exist automatically due to ConfigEntityBase::preSave() |
entity_form_display | the form mode entity, the config entity if one provides the bundle, the fields, the widget providers |
form_mode | module that provides the target entity type |
entity_view_display | the view mode entity, the config entity if one provides the bundle, the fields, the formatter providers |
view_mode | module that provides the target entity type |
field_config | module that provides the field type |
field_instance_config | field_config entity, the entity that provides the bundle (eg. node type) |
filter_format | the modules that supply the filter plugins in use |
image_style | the modules that supply the image effect plugins in use |
language_entity | n/a |
node_type | This has a settings key that modules had into but uninstall is handled. Should be converted to the dependency system if these are converted to plugins |
picture_mapping | The breakpoint group |
rdf_mapping | the bundle entity |
search_page | the module that provides the plugin |
shortcut_set | n/a |
action | the modules that supply the action plugins in use |
date_format | n/a |
menu | n/a |
taxonomy_vocabulary | n/a |
tour | The module the tour is assigned to. The Tip plugin providers. Follow up to convert Tour entity to EntityWithPluginBagInterface and therefore the Tip plugin provider dependency will exist automatically due to ConfigEntityBase::preSave() |
user_role | n/a |
view | modules that provide the plugins if not optional, field_instances_config entities? |
Remaining tasks
- Review
User interface changes
- Entities that will be removed by uninstalling a module are list on the confirm form.
- Theme's are never uninstalled so there is nothing to do here. #1067408: Themes do not have an installation status will change this.
API changes
A new API to allow one module to indicate it has soft dependencies through config entities it "owns" (i.e. where it provides the config entity type) on another modules.
Related Issues
#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
#1765936: Invoke ConfigStorageController::delete on module uninstall and add tests
#1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API
#2079121: When a module providing a base table is uninstalled, delete all views built on it
Comment | File | Size | Author |
---|---|---|---|
#129 | 123-129-interdiff.txt | 1023 bytes | alexpott |
#129 | 2080823.129.patch | 219.14 KB | alexpott |
#123 | 2080823.123.patch | 219.27 KB | alexpott |
#123 | 121-123-interdiff.txt | 14.48 KB | alexpott |
#121 | 120-121-interdiff.txt | 15.11 KB | alexpott |
Comments
Comment #1
mtiftblocked on #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
Comment #2
catchComment #3
catchCopying my comment over from that issue:
So the problem with leaving the front page view in when node module is disabled, is the same problem with disabling and re-enabling modules I think. Except it's worse, because we run updates for disabled modules currently, don't do that for uninstalled ones.
Here's what I think will happen, correct me if I'm wrong:
1. Node module is uninstalled, views is left installed.
2. Front page view (still using node entity or other views plugins provided by node) stays in active config.
3. Meanwhile, node module code is updated to rename a plugin, includes an update handler to rename the plugin in views configuration files. Because node module is currently uninstalled, this update never runs.
4. Node module is re-installed, now the view is missing plugins (assuming we don't overwrite it on re-install? But then what would be the point of keeping it...).
So of the options, I prefer this one:
I don't see a way around week/month/year-long race conditions unless we either prevent modules from being uninstalled when configuration depends on them, or uninstall all that configuration. Both of those options are valid. People don't like making modules 'required' dynamically though, so here's an alternative suggestion:
- When uninstall is submitted, validate whether any config entities have a dependency on the module.
- If they do, show a confirmation screen.
- That confirmation screen tells you what's going on, if you want to delete those config entities anyway, you have to explicitly select a checkbox. Otherwise it prevents the uninstall of those modules. For drush and similar it could add an option or its own confirmation.
Comment #3.0
catchremove suggestion to change ModuleHandler::uninstall()
Comment #3.1
mtiftUpdated issue summary.
Comment #3.2
catchUpdated issue summary.
Comment #4
yched CreditAttribution: yched commented#1776830-83: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API had a very important fix, we should make sure it doesn't get lost.
Whatever the process for deleting config or not, when we do delete stuff that is a config entity, we should delete it as a config entity, not just wipe its config file.
[edit: fixed link to the real comment I had in mind :-/]
Comment #5
mtift@yched: Yes, although I think you meant #1776830-75: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API. Yesterday in IRC alexpott talked about moving that over.
Comment #6
yched CreditAttribution: yched commentedDoh, sorry, copied the wrong comment #. No, actually the one I was thinking of was @beejebus #83.
I fixed my comment above.
Comment #7
catchOpened #2090115: Don't install a module when its default configuration has unmet dependencies and marked it postponed on this one.
Comment #8
catchComment #9
catchComment #10
alexpottRetitling issue to focus issue on creating API and providing a use case.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedbump.
Comment #11.0
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdated issue summary.
Comment #11.1
alexpottUpdated issue summary.
Comment #11.2
alexpottlog
Comment #11.3
alexpottlog
Comment #11.4
xjmUpdated issue summary.
Comment #11.5
alexpottUpdated issue summary.
Comment #12
alexpottConsidering this is a key API we should have this done before D8 beta.
Comment #13
ianthomas_ukProviding a clear UI and batching system that can intelligently delete any type of soft dependency seems like an awful lot of work, particularly if there are dependencies of dependencies. Even if such a system could be written, it is making it too easy for site builders to accidentally delete large amounts of content? Maybe you don't want the content to be deleted, but rather converted to some other format.
I think we should be aiming to support a UI that lists the categories of content that are preventing a module from being uninstalled, and allows modules to provide links to admin interfaces to edit/remove that content (similar to the configure links on the Extend page). That way site builders are more likely to understand what they are doing, and each module can provide a suitable UI.
Comment #14
dixon_This is not at all a solution to the problem at hand, but I just want to cross link the Entity Dependency module that would benefit of/extend a generic entity dependency API. That module is currently being used to find content dependencies for content staging solutions built with Deploy.
Comment #15
larowlanAlso, we have sdboyer's graph api in core, I'm using it in github.com/larowlan/default_content to resolve dependency resolution between default content, default config would be very similar.
Comment #16
Gábor HojtsyIs the use case only on module uninstall? Nothing to cover for installation or import here? Then this sounds like something that Drupal did not provide before, did it? So looks like part of the question on how we define CMI's scope. In Drupal 7, if you delete a node type, are its fields deleted? Are the field data storages deleted? In other words is this a regression from Drupal 7 or a want for Drupal 8? :)
Comment #17
catchYes, if you use the API properly:
https://api.drupal.org/api/drupal/modules%21field%21field.attach.inc/fun...
For import/install see #2091615: Installation profiles provide default config that is not only used during installation #2090115: Don't install a module when its default configuration has unmet dependencies both postponed on this issue.
Comment #18
alexpottComment #19
alexpottA patch!!! Too tired to write much of a comment - but just in case I'm hit by a bus.
This patch replaces the above code by having an dependency management system. If you uninstall a module and a view depends on it that view will be removed.
Still have to handle dependencies on other configuration entities.
Comment #22
alexpottFixes some of the test failures.
Comment #23
alexpottComment #25
alexpottSlightly new approach. I've completely removed the dependency on the Config Entity type actually existing so the dependency management system can work for configuration import validation (i.e. before modules are actually enabled).
Additionally this patch adds the ability the make configuration entities dependent on each other and discover this dependencies using the Graph object. Added test for a couple of scenarios.
Furthermore continued working on the implementation for views. Discovered that View::$module is not correct for a couple of the default views. Also started to implement the dependency in the default views configuration.
Comment #26
alexpottFix configuration tests.
Comment #29
alexpottAdded dependency management for fields.
Comment #30
alexpottOops got the direction of dependency discover for entities to wrong way around.
Comment #33
alexpottFixed the test fails by making the dependency a value rather than a key - since config keys can not contain a dot.
Created a sandbox: http://drupalcode.org/sandbox/heyrocker/1145636.git/shortlog/refs/heads/...
Comment #34
alexpottFixing the views dependencies issues pointed out by @swentel. Uninstalling history was prompting uninstallation of views :) http://imgur.com/lAPVwvV
Move views.view.tracker back to views and undid changes. This view is dependent on both history and comment - we have no module that satisfies these conditions - let's do #1853572: Remove the old default tracker view
Comment #35
swentel CreditAttribution: swentel commentedComment #36
swentel CreditAttribution: swentel commentedLet's see how much this blows up with the dependant config entities now removed by the Entity API and the removal of field_system_info_alter(). Uninstalling image worked fine already locally! Haven't committed/pushed this yet, code is not /that/ sweet.
Added an isUninstalling() method so we can use this. Currently use in preDelete of Language config entity.
Comment #38
swentel CreditAttribution: swentel commentedNever thought I would say this, but these failures are fantastic :)
Comment #39
catchIf we implement all dependencies of Views, I think we don't actually need this line. I'm thinking of a feature module that doesn't provide any Views plugins but provides a couple of default views. In that case, whether we delete the view when the module is uninstalled comes down to choice. Can be follow-up either way.
This is great!
Comment #40
alexpottNew patch cut from the sandbox
Comment #41
damiankloip CreditAttribution: damiankloip commentedWe would ideally still want to know where this view came from though?
Comment #42
alexpottThis patch relies on the config entity uuid key being
uuid
- it is hard coded to be but we pretend this is not the case - see #2198377: Enforce UUID key name in configuration entitiesComment #44
damiankloip CreditAttribution: damiankloip commentedComment #45
swentel CreditAttribution: swentel commentedAlex fixed the dependency test. I refactored the reEnableModuleFieldTest. I had to add an extra check in FieldInstance to make sure we don't delete fields there while uninstalling, otherwhise FieldableStorageController goes boom.
This should be green again.
- edit - the comment in the test should be adjusted, the table will not be gone yet.
Comment #46
yched CreditAttribution: yched commentedW00t ! Awesome work !
More general / conceptual remarks for now, saving more specific / nitpicky code remarks for later.
Insert Ewok dance here.
Looks like there's a chance of collisions, two entities of different entity types might very well happen to have the same id.
Favorite method name ever ;-)
More seriously: wow, this reads all config records, and builds & statically persists one ConfigEntityDependency object for each existing config entity.
Next to that, we also build & persist a graph that contains a proportional amount of data.
That's a lot of data to keep in memory :-/
It seems we never actually need the *full* graph for all the config records:
- getDependentEntities($type, $name) uses loadAllTheThingies() to find config entities that depend on the module $name
- then we only need the graph starting from *those* config entities.
So if CDM::getDependentEntities() received an array of $names (instead of currently a single $name, and ConfigManager::findConfigEntityDependents(array $names) iterating seperate calls on each $name), then maybe we could only build a "smaller" graph and then ditch it instead of keeping it in memory ?
I don't see how we can avoid parsing all config entity records.
But maybe there's at least a way to limit the amount of ConfigEntityDependency objects we create and persist ?
The property name (and associated setter and getter names) looks a bit off.
Entity->isSyncing() is fine because it means "the entity is being synced".
But $entity->isUninstalling()... well, the entity is not being uninstalled :-)
No better suggestion for now though.
Actually, I'm not sure I understand why that property / method is needed. Only FieldInstance and Language use it currently. What situation is this preventing ?
Hmm - ConfigEntityBase::getExportProperties() takes care of saving $this->dependencies if it exists, and the patch updates all existing overrides of the method to include 'dependencies' too (without the "if it exists" part, though).
So each config entity will need to explicitly add this to its schema as well ? (patch currently only takes care of field and views schemas)
Wondering if there would be a less painful way ? Well, we already force each schema to re-define 'uuid' & 'langcode'...
OK, so the 'instance' has a dependency on its 'field'. This lets us display accurate info on what will get deleted on module uninstall confirmation form. Uninstalling 'image' deletes all image fields and their instances.
Yet Field::preDelete() still has its custom code to manually delete instances when the field is deleted.
Or, put another way : shouldn't it be automatic that all dependents of a config entity get deleted whenever that entity gets deleted - not just at uninstall time ?
What's the meaning of "having a dependency on another entity" if you can still exist when that entity got deleted ?
I'm afraid getLabel() is not enough - there will be entities with the same label (e.g. most of the time, instances of the same field just reuse the same label). We'd need the id() too IMO.
It feels potentially confusing that some EntityTypes allow you to manually add $entity->addDependency() at runtime, while others will wipe and rebuild them themselves.
Shouldn't it be the same behavior for all entity types ?
(Also, feels weird that we have wrapper methods to add and remove dependencies, but that emptying is done by directly working on the ->dependencies property)
Comment #47
alexpott1. Need video evidence
2. Nice spot - pushed a patch to address and test this to the sandbox.
3. We need all the config entities unfortunately as things depends on config entities which themselves have no dependencies. Without these entities we can not build a graph. We only persist the graph and config entities in the ConfigDependencyManager for as long as it exists (rather than the whole request)
4. We trying to ensure that we don't raise exceptions during a delete. The language entity prevents deleting the default language since this would seriously mess up you site - unless you are uninstalling the language module.
5. Some call the parent function some don't - I think we we do is ok.
6. Yes we should use this system elsewhere.
7. Yep - I would actually like this to be a link to where the user can manage the thing.
8. I'm not sure I agree. Views is wiping the dependencies because it know it can recalculate them. Other entities can choose to do this differently - it should be up to the module that creates the entity type to implement this how they like.
Also we have to worry about secondary deletes :)
Comment #48
catchIMO it would be fine to enforce this in the UI and not do anything at the API level - would save adding the odd uninstallation check.
We don't do things like preventing the deletion of nodes set as the front page or deleting users that authored thousands of nodes, and this isn't really that much different.
Comment #49
swentel CreditAttribution: swentel commentedThe UI already prevents you deleting it, I'm guessing it's there to prevent it when it would be triggered programmatically.
Comment #50
catchRight but is that really necessary? If you delete it programmatically you should be able to understand the consequences of doing so / avoid doing it in the first place.
Comment #51
alexpott@catch this is different - if you do this you site is totally kaput.
Comment #52
catchWhat if during uninstall we delete the default language configuration value before deleting the default language?
Also I think it's OK to let people do things via the API that completely kaput their site. We don't lock down db_truncate() either.
Comment #53
Wim LeersNot sure if it's relevant, but I see mention of "graph" here. If you need more graph capabilities, you may want to look at the Gliph graph library that Drupal includes — written by sdboyer :)
Comment #54
xjmAs per #2198429-10: Make deleted fields work with config synch, this issue can proceed without #2198429: Make deleted fields work with config synch by retaining field_system_info_alter() for now.
Comment #55
xjmHowever, @alexpott stated in IRC that #2199483: Provide a default config_prefix based on entity type ID and provider is a hard blocker for this issue, so should this issue be postponed? If so, it would also be good to document exactly why here.
Comment #56
alexpottComment #57
catchComment #58
alexpottComment #59
xjmAwesome list in the issue summary.
I rescoped #2198429: Make deleted fields work with config synch based on our call earlier this week; is that the right way to go? I don't think it blocks this but adding that "deleted stuff" step should probably be beta-blocking as part of making config synch work.
Comment #60
alexpottNew patch adds dependency management for
The config installer now installs default configuration in the order to guarantee dependencies exist.
Unfortunately an interdiff was not possible.
Comment #62
XanoThe array structure must be documented.
Same here.
The return value must have a description.
The return value must have a description.
It may be useful to link to the entities, so site admins can quickly inspect the data they are about to remove.
The method name is missing brackets.
Comment #63
XanoAlso, why doesn't the patch add PHPUnit tests for the newly added code to entities? When #2134857: PHPUnit test the entity base classes is fixed, that won't even be hard to do anymore.
Comment #64
alexpott@Xano this patch is no way near ready for that kind of review.
We have issues with secondary writes during module install now everything is being installed in order of dependencies!
Comment #66
xjmLooks like this issue is going forward without #2198429: Make deleted fields work with config synch; should that updated issue happen alongside this one? After we do #1808248: Add a separate module install/uninstall step to the config import process?
Comment #67
alexpottFixed failing tests - loads of work still needed
Comment #68
alexpottPatch attached to show where I'm at and to display something interesting. The search module creates entity view modes for node - but the search module does not depend on node. This is a problem - since we can not work out the node entity's provider if the node entity does not exist! To test this - install minimal, disable node, enable search.. KABOOOM! Also
Drupal\system\Tests\Entity\ConfigEntityImportTest
breaks because of this - and probably any test that enables search but node is going to have a problem.Comment #70
alexpottOkay for now I've moved the node search display modes from the search module to the node module since this is where the plugin is. Discussed with @Gabor, @catch - potentially the only solution will be to have a node_search module.
The patch attached implements all of the dependencies in the table in the issue summary including theme dependencies and the removal of the all the breakpoint code to manage dependencies (see the lovely diff of breakpoint.module)
Comment #71
alexpottAttaching missing interdiff
Comment #72
alexpott#71 is a lie
Comment #73
alexpottRemoved the work in ConfigImporter as this should be done in #2030073: Config cannot be imported in order for dependencies.
One point to note:
We need to remove theme config on disable since we can get into config mismatch issues if a theme is upgraded and it's configuration is left lying around. Plus this allows us to keep active configuration nice and clean.
Comment #74
alexpottKnown outstanding issues:
Comment #77
alexpottPatch fixes
node_add_body_field()
- the teaser mode is created by Standard profileIt also dependencies on plugin providers to entity view/form displays based on the formatter/widget they are using.
Comment #78
alexpottHa I thought there must be a theme disable confirm form - but nope there is not. Maybe adding one should be a critical follow up.
Comment #79
alexpottThe current patch ensures that theme configuration and configuration entities dependent on the theme is removed when the theme is disabled. However there is no confirmation form to inform the user that data is going to be deleted. So I think this needs to be removed and left to #1067408: Themes do not have an installation status. In HEAD breakpoint currently removes config on theme disabling - this is incorrect. This patch will remove this and create dependencies on the theme.
Comment #83
alexpottShould be back to green. Removed too much of the CustomBlock entity so restored that... and entity_test_install() was creating a component on a form display for a field type that does not exist :)
Comment #84
alexpottComment #86
alexpottThis time green for real - some more non existent plugins being used!!! This patch is going to keep everyone honest. No more assumptions allowed - if you have an unmet dependency you'll know :)
Comment #87
Gábor HojtsyThis patch looks amazing. I summarized the changes in this visual. Green things are new, blue things are changed old. The "paper slip" is about actual config changes. Actual document at https://docs.google.com/drawings/d/1MZcID-iTsQaDhYe_caBNU4-qbmAS2mS8zy3i... Added to the issue summary.
Comment #88
Gábor HojtsyComment #89
alexpottThe current @todo list:
Comment #90
swentel CreditAttribution: swentel commentedRegarding the todo: +1 on a separate method
Here's a first review, I'm not able to commit the small nitpicks in the sandbox today, the earliest on monday.
I'm surprised by this change, text_default exists, or did you just do this because of the RebuildTest to make them consistent ?
Needs a comma after 'however' ?
We can probably remove the array_reverse now, since the introduction of setUninstalling()? Also, the comments makes it feel like we're only doing this for fields and instances only ;)
'the a non' - ditch the 'the'
exceeds 80 chars
Same here
'declare'
80 chars
Can't we just call drupalCreateContentType() which is added in a lot of other places ?
Hmm, so this only adds them for field instances ? We should probably account to for 'extra fields' which also have provider.
'and' instead of 'are'
Other than that, having code relying or 'listening' to a profile seems just completely wrong to me.
Comment #91
swentel CreditAttribution: swentel commentedAdditionally on this part of the review, short brain dump of an idea that both me and Alex had separately and very briefly talked about in IRC a couple of days ago. I'm not 100% sure anymore whether it was because of this part of the patch, but it's surely related.
The idea is simple: default configuration entities (and maybe also just simple settings) simply don't come with a module that is also the provider, or in another module that would need the other as a dependency.
As a consequence,
node_add_body_field
in the current patch obsolete, and imho also simply not done.To me this feels almost like a natural evolution in the CMI process, not sure how others feel about that idea. Definitely not something to fix in this patch, but for a follow up, but this is the idea. so .. shoot :)
Comment #92
larowlanSo that would make forum a feature module? It needs default fields, vocabularies etc. If I keep going with the views conversions, it would make even more sense.
Comment #93
larowlanIn our new OO Paradigm, isn't a concrete new bad karma? We're tied to this concrete implementation. Playing devil's advocate - could any of these use a factory/service? We already have new Graph() in core, so probably ignore that.
There is no need to explicitly declare the dependency. (declare not declared)
would it be faster to key these by name instead? Would make unsetting faster.
out of scope, but ouch
oh yeah! awesome cleanup
Can these be injected?
Comment #94
tim.plunkett#1915272: Move _breakpoint_delete_breakpoints() into BreakpointGroupStorageController was refactoring that breakpoint delete code, since you're removing it wholesale should we just close that issue?
Comment #95
alexpott#90 @swentel
1. This is form widget not a field formatter afaics there is no text_default.
2. Fixed by moving comments around.
3. Nope - it's important that entities are uninstalled in the order of reverse dependency. Fixed comment.
4. Fixed
5. Fixed
6. Fixed
7. Fixed
8. Fixed
9. Sure - fixed.
10. We don't store a type key for extra fields - if we did then this would work. However this did make me realise that we need to add dependencies for fields that are hidden. Discussed in irc with @swentel - he said "yeah, we'll need to store the module/provider in the definitions (of the extra fields) so instead of invokeAll, we'll have to get the implementations and then store the module" - feels like a followup to me.
11. Fixed
#91, #92 @swentel, @larowlan
Yes I think we should have as few default config entities as possible in core modules and just have everything in standard profile or feature modules. But that's a separate issue for this :)
#93 @larowlan
1. These are value objects I don't think there is any need to add additional complexity here by making it swappable / injected / or anything else :)
2. Fixed
3. This was how I did it originally but entity ids have dots in them which is extremely problematic for our Config::get()
4. Yep the moduleHandler is a mess
5. :)
6. Yep - fixed.
#94 @timplunkett
Agreed - I'll close that as a dupe of this.
Comment #97
alexpottMoved dependency calculation out of
preSave()
into a newcalculateDependencies()
method that is called duringpreSave()
. Whilst doing this I realised that the only usages of the public methodscreateDependency()
andremoveDependency()
are in tests. Additionally sincecalculateDependencies()
ensures that the dependencies are completely recalcuated on save there is little point to having a public method to add a dependency andremoveDependency()
makes no sense at all.@swentel prefers
addDependency()
overcreateDependency()
and so do I.This refactor highlighted a problem with previous patches whereby setting $this->dependencies to an empty array would not actually reset the dependencies :)
Comment #99
alexpottMy custom htaccess snuck in
Comment #100
jessebeach CreditAttribution: jessebeach commentedRetitling. It's missing a possessive apostrophe.
Comment #101
damiankloip CreditAttribution: damiankloip commentedComment #102
alexpottRerolled and prepared the ground for unit testing of all the
calculateDependencies()
implementations - which is what I'm working on atm.Comment #103
alexpottAdded unit tests for all the calculateDependencies implementations except View, Tour, EntityDisplayBase and PictureMapping configuration entities because those classes are more than a bit difficult to unit test.
todo list
Comment #105
alexpottFix fails in the CKEditorTest - a static caching issue.
Comment #106
dixon_Just had a quick read through the code...
Should we not provide an interface for this, even though it might not be strictly pluggable?
Doesn't seem to be much difference between the two, at least not from looking straight at the definitions.
Is it meant to be just a semantical difference, even through internally it happen to be the same property? Maybe this could use an explanation in the doc blocks there.
which...? :)
Further, I think it would be good design to provide a set of higher interfaces starting from something like
interface EntityDependencyInterface
andabstract EntityDependencyBase
thatclass ConfigEntityDependency
could implement and inherit from. Because there's definitely some code in there that could be nicely reused by the contrib modules like Entity Dependency and Deploy.But, that should not at all hold up this patch from moving forward as-is. I'd be happy to file that patch as a backward compatible feature request for Drupal 8.1 or 8.2 or so...
Comment #108
alexpottThe fail in #105 for a segfault in
Drupal\update\Tests\UpdateContribTest
The patch attached also:
\Drupal\tour\Tests\TourTest
Comment #109
alexpotttodo list
Comment #111
Bojhan CreditAttribution: Bojhan commentedThis looks great, I am always happy when we can inform users better when doing destructive actions!
My only comment would be that the fieldset has no useful label, keep in mind that fieldsets should when possible always describe their contents. There are a couple solutions; "To be deleted configuration", "Configuration deletions", "Configuration that will be removed" etc.
Comment #112
alexpottUpdated uninstall confirm form with suggestion from @Bojhan and added tests - nothing left @todo here!
Module uninstall confirmation form (closed)
Module uninstall confirmation form (open)
Comment #113
alexpottComment #115
alexpott112: 2080823.112.patch queued for re-testing.
Comment #116
alexpottTrackerTest failed failing to write a configuration file - so it looks like testbot diskspace.
Comment #117
alexpottRerolled due to #2124377: Rename "Picture" module to "Responsive Image" module
Comment #118
catchOnly got part way through, but posting what I have so far:
Should system.site.uuid seems like it shouldn't be in config but rather $settings (with a comment never to change it). Or rename to site_uuid? I'm wondering whether we want to allow this at all or should just reserve the key. Problem isn't introduced here so just follow-up if we want to revisit that I think.
I got a bit thrown by 'module', 'theme', 'entity' as the three types of dependency. All dependencies boil down to these, but some are via plugins or similar that in turn are provided by a module (if we solve #2212151: Document/test updating active configuration based on API changes (i.e. plugins) then it'd be possible for a plugin provider to move from one module to another - dependency would still be there, but for a different module).
This is fine, but it feels like it's missing some high level documentation on how we got to those three kinds of dependency.
This means all the other methods have to call getGraph() instead of relying on the property. Why not move the logic to setData()?
Either way, shouldn't setData() reset $this->graph (whether to null or the new graph)?.
Comment #119
alexpottComment #120
Wim LeersWow, wow, wow! Impressive work! I got through about 60% of the patch. I only fixed the things where I was certain my proposed change was correct.
"use load multiple" sounds very strange. Changing it to "use load_multiple" would already help, but is still not ideal. Not worth spending more time on though probably.
I think "entity" is confusing, since it could be a content entity. We only allow dependencies on *config* entities. I understand that "entity" is shorter than "config_entity", but I think the latter would be better because it leaves no room for ambiguity.
s/their fields/fields are assigned to them/
?
What does it mean to have "dependencies written to the configuration entity"? Does that mean that configuration entity YML files are dynamically modified whenever a new module/theme/config entity is added to the system that depends on it?
If so, I think that could be worded more clearly.
It says just "entity", so this could be either a config or content entity? If so, I think it'd be better if this were mentioned specifically.
Why not
Drupal\Component\Graph\Graph
?This block of code could be moved into the "else" block a few lines down, that would make the code much clearer. Or is there a particular reason for it to be this way?
The description here of what the specific alphabetical ordering is, is … special :P Can we make this less ambiguous?
s/is deleted/is being deleted/
?
Not >1, but >0 or >=1?
Description missing.
s/is deleted/is being deleted/
?
s/import process/uninstall process/?
Why make the key singular?
Because this is a test block, it happens to be that we control that it indeed can only be used in the Stark theme. But I think this generally shouldn't be hardcoded, right?
Why disable this and another similar assertion further down?
I think one thing is still missing here: dependencies on modules that provide formatters and widgets, which could be contrib modules.
"Core" is a module?
Good catch :)
Comment #121
alexpottThank @Wim Leers!
entity_type:UUID
but using the configuration entity configuration name means that this would no longer support content entities. Hmmm what to do? I guess we could swap toconfig_entity
as a key - let's see how others feel.Dependencies are stored to the configuration entity's configuration object
. If a installing a module or theme would add dependencies to existing configuration entities just be being installed then it would need to re-save the configuration entities itself - this should be very rare indeed.Drupal\Component\Graph\Graph
:)Core
in the addDependency code and say it's silly to add a dependency on it.Comment #122
BerdirWent through this, quite a patch ;) Make sure to read everything first, I found answers to my own questions myself in some cases.
I also didn't read anything in the issue and only skimmed the issue summary. Overall, this looks good, only wondering about the performance impact of all the entity loading that's going on. The abstraction of he dependency ID is nice, but it's also kind of stupid that we have to load all these entities just to get a string with the config prefix and id back? Could this maybe be a static method, or something somewhere else? Is there a reason to change that ID?
Also, thinking a bit more about performance, it shouldn't be necessary to calculate dependencies during a config sync, should it? That means we could wrap it in a !isSyncing() and avoiding to recalculate in that case?
At least the system.site.yml has a uuid too, is that a problem?
This is an entity type, not an entity, correct? Maybe use $entity_type_ids or something instead?
return $this.
Wouldn't that mean that we're then checking for a specific implementation and not an "interface"? Why not do a method_exists() instead?
Is this something that modules need to check?
If yes, it should explain what that means, just like isSyncing() should (we spent a lot of time on that in https://drupal.org/node/2157467). If not here, then somewhere else.
Should use the interface when changing it anyway..
Not sure why we are doing this here? Seems like the method is unnecessary anyway?
We are setting it here, so it shouldn't be necessary?
Would using the interface here make this easier?
Minor: Should we move the addDefaultField() call to here as well, so that those two things are together just like in CommentTestBase?
This also needs an @inheritdoc now..
Leading \ missing. Sorry ;)
Ah, I guess that answers my own question, this is the expected behavior then.
Is there anything we can assert here, or are we simply relying on it throwing exceptions when it would do the wrong thing?
It's not *strictly* required that bundles are an entity type (entity_test uses simple state variables). Maybe reword the comment a bit so that it says something like "... uses entities to manage its bundles" or so?
We now have formatters and widgets in Core, e-mail for example. This would then add module: Core I think?
So far, this was explicitly supported as the formatter/widgets was considered a non-required dependency, there was a recent comment from someone (swentel?) about that somewhere, but can't find it right now.
Not saying that we can't change that or shouldn't, just pointing out that this explicitly like this, unlike field type providers.
k, there are cases where it needs to be checked :)
Any chance to unify those two flags/methods? $entity->iKnowWhatImDoing() ;)
Huh.... ah, the default fallback. Nice.
This is the default storage controller for @ConfigEntityType, so you can remove the explicit definition.
Seems like this test is now pretty useless, why not just remove the assertion?
Wasn't sure why we bother to actually test this, but it's just following isSyncing. Not really necessary, you're adding other empty methods too
Comment #123
alexpottPerformance
With regard to loading everything - we have to load everything to get the dependencies array too - I think this is unavoidable. You are right we don't have to recalculate during an import - nice catch. Implemented this in the patch attached.
Numbered review
Comment #124
BerdirReviewed the change notice, added the missing reference to this issue, looks great! (just made the code snippets use php tags to have code highlighting).
Did some performance tests to be able to understand the performance cost of this change. I used view displays as we have a few of them and they have lots of dependencies:
$ drush scr config_save.php
HEAD:
Resaved 9 displays in 22.74ms
Resaved 9 displays in 23.79ms
Resaved 9 displays in 21.89ms
Resaved 9 displays in 21.2ms
Resaved 9 displays in 23.16ms
Resaved 9 displays in 24.01ms
Resaved 9 displays in 20.94ms
With Patch:
Resaved 9 displays in 41.39ms
Resaved 9 displays in 39.76ms
Resaved 9 displays in 42.12ms
Resaved 9 displays in 40.16ms
Resaved 9 displays in 41.91ms
Resaved 9 displays in 37.86ms
So this does make it twice as slow on my laptop (someone wants to try this on a different system?)
I did have a look at xhprof, most of the time is spent loading additional config entities, I also had some cache writes due to list calls, the problem there is that after every write, we reset the whole list cache, so the next display entity has to update that again. This is actually caused by the existing UUID check in ConfigEntityBase::presave(), but EntityDisplayBase previously didn't call parent::preSave(). And that call is more expensive than the new calculateDependencies() method. So EntityViewDisplay was a bit an unfair example but on the other side, that is the actual regression for them.
Anyway, I guess there isn't really anything to optimize, other maybe looking into the config list cache and performance of loading config by UUID but those are pre-existing issues in HEAD that we just happen to run into here.
+1 to RTBC from my side, have nothing else to complain about. This will need an OK from other reviews as well, I don't have an overall grasp of the whole thing here.
One last thing here: The migration system currently has it's own dependency system, which does work differently. Should we refactor that based on this later? It has advanced concepts like optional dependencies (run first when existing, but ignore otherwise)? It applies to running migrations, not copying them around, so probably should stay separate? Either way, it will result in a conflict because it also uses "dependencies" right now as the key.
Comment #125
alexpottThis is blocked on #2221767: Migration configuration entities should not use the dependencies key
Comment #126
alexpott123: 2080823.123.patch queued for re-testing.
Comment #127
alexpottSo the current us of
dependencies
actually does not block this patch since the Migrate API and migrations in 8.x do not use it. @sun has suggested changing the key we add on this issue torequires
. I'm not a fan of this at the moment since we still havedependencies
in.info.yml
.It would be fantastic to proceed with this patch asap.
Comment #128
Gábor HojtsyIt looks good from my side as well. Based on the above reviews as well, it look all good to go now :)
(Note I reviewed the whole architecture above already and created the figure explaining what it does in the issue summary :) All concerns are resolved and this has a verrrry long dependency chain as well.)
Comment #129
alexpottTiny patch context conflict with #2219925: Rename ConfigEntityInterface::getExportProperties() to toArray() in ConfigEntityInterface.
Comment #130
catchI've looked at this several times now and I think it's good as well. We might want to open a separate issue to discuss isSyncing vs. isUninstalling (and I still feel like the language presave preventing delete is heavy-handed), but none of these are really new issues, and this unblocks lots and lots and lots of other issues.
Committed/pushed to 8x, thanks!
Comment #132
Wim LeersHurray! :) Awesome work, Alex!
#2007248: When CKEditor module is uninstalled, the right text editor config entities are affected but their labels are missing was solved automatically thanks to this issue. However, I did find one small problem there: text editor entities slated for deletion are found correctly, but their labels are missing. I will fix it in that issue though, and that of course doesn't warrant a rollback. It's just an FYI, because others might encounter the same problem elsewhere.
Comment #134
XanoComment #135
andypostThere's 3 @todo left so filed follow-up #2343517: Cleanup @todo referring to the config dependencies API issue