Problem/Motivation
Entities with missing modules will result in fatal errors everywhere.
Proposed resolution
Add an uninstall validate method to the module_installer service added in #2324055: Split up the module manager into runtime information and extension information.. Also make uninstall call it if it wasn't called before having a uninstall_validated array on the class listing the modules already validated for uninstall. This method should call a helper on entity manager that checks for any entities provided by that module (handwaving a bit here -- who discovers how an entity provides which module types).
The uninstall method should validate the whole list of modules after all dependencies are resolved.
Then the ModulesUninstallForm can call this validate handler to inform the user why a module can not be uninstalled.
Make the uninstall form handle this. Much better than running (number of entity types) queries on _system_rebuild_module_data...
Remaining tasks
Implement the module_installer service so that EntityManager can be injected into it without having a circular dependency between entity_manager and module_handler.#2324055: Split up the module manager into runtime information and extension information.
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Add automated tests | Instructions | ||
when this is fixed, unpostpone #2338873: Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data |
User interface changes
- As of #53, modules that are uninstallable because content entities exist are not shown on the uninstall form. That may or may not be the behavior we want.
- As of #104, modules that are uninstallable because content entities exist are disabled on the uninstall form and provide a message with a reason.
API changes
- ModuleInstallerInterface->validateUninstall() has been added.
- Module uninstall validators can register themselves with the module_install.uninstall_validator tag and implement \Drupal\Core\Extension\UninstallValidatorInterface
Follow-ups:
Comment | File | Size | Author |
---|---|---|---|
#116 | interdiff-111-115.txt | 1.2 KB | bircher |
#116 | 2278017-115.patch | 20.3 KB | bircher |
#111 | 2278017-111.patch | 20.21 KB | bircher |
#111 | interdiff-107-111.txt | 11.63 KB | bircher |
#107 | 2278017-107.patch | 18.01 KB | bircher |
Comments
Comment #1
xjmComment #2
effulgentsia CreditAttribution: effulgentsia commentedComment #3
catchProposed solution works for me.
Comment #4
xjmComment #5
dawehnerThis is certainly a big issue, though similar problems existed back in D7, didn't they? So at least peopled dealt with that problem earlier as well.
Comment #6
BerdirYes, this already existed in 7.x, although in 8.x, it's going to be a bit.. rougher. Due to interface and methods, trying to display or edit, maybe even delete nodes with a missing node type will likely end in fatal errors and not some notices and warnings.
Comment #7
catchComment #8
BeyersdorferTJ CreditAttribution: BeyersdorferTJ commentedFor a clean enviroment all entities should be deleted when their entity type gets deleted. To make a module required as soon as it defines an entity type sound like a really dirty quick fix to me (and it might be difficult to categorize it as this).
Comment #9
naxoc CreditAttribution: naxoc commentedComment #10
BerdirSo it looks like #2224581: Delete forum data on uninstall added a custom hack to do this for the forum node type. And #1002164: The Book module can be uninstalled with nodes with a book node type still existing is about to do the same custom hack for the book node type.
Should we try to make that into a general solution here, possibly without the making-modules-required hack ?
Comment #11
cilefen CreditAttribution: cilefen commentedComment #12
chx CreditAttribution: chx commentedComment #13
cilefen CreditAttribution: cilefen commentedComment #14
cilefen CreditAttribution: cilefen commentedThis is a sketch of how this might work. It is rolled against #2324055: Split up the module manager into runtime information and extension information..
Comment #15
chx CreditAttribution: chx commentedThanks much for working on this! protected function validateUninstall should be public so that the form can call it ahead of the time and offer only uninstallable modules.
Comment #16
cilefen CreditAttribution: cilefen commentedThis is rolled against #2324055: Split up the module manager into runtime information and extension information. #40 and sets the method visibility to public and puts it in the interface. It still does not contain the real entity work.
Comment #17
cilefen CreditAttribution: cilefen commentedComment #18
cilefen CreditAttribution: cilefen commentedIt is not obvious to me how to differentiate a content entity from a configuration entity.
Comment #19
chx CreditAttribution: chx commentedThank you SO MUCH for working on this.
$content = $factory->get($module)
should be$content = $factory->get($entity_type->id())
. Not all entity types are the same as their module (although often they are). For example,taxonomy
vstaxonomy_term
andtaxonomy_vocabulary
.You hardly need to check for SqlContentEntityStorage: the entity query is storage agnostic and so is the whole issue. But I guess you are well aware of that and only used this in place of checking for content entities.
So to check whether an entity is a content entity or not, the standard way is
$entity_type->isSubclassOf('Drupal\Core\Entity\ContentEntityInterface')
. At least this exists 4 times already but of course it's impossible to find if you don't know what to look for specifically. This should be documented somewhere but where? Perhaps we should provide wrappers, only three interfaces are ever used... (Drupal\Core\Config\Entity\ConfigEntityInterface
(6 times),Drupal\Core\Entity\FieldableEntityInterface
(8 times) are the other two). How nice it'd be$entity_type->is('content')
or$entity_type->isContent()
even. This is a normal task followup, I feel.Comment #20
tim.plunkettSee #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements()
Comment #21
znerol CreditAttribution: znerol commentedThis should not be the responsibility of the
ModuleInstaller
. Rather theModuleHandler
is responsible for making runtime information available to any interested service (including to the module installer).As the module installer performs the rather disruptive task of adding/removing modules, it only should be injected into/used by code which intends to perform such a task. Validation should also be possible without having to depend on module installer.
Comment #22
chx CreditAttribution: chx commented> Validation should also be possible without having to depend on module installer.
Could you please tell us how?
Comment #23
znerol CreditAttribution: znerol commentedvalidateUninstall()
has no dependencies on the installer, it can easily live onModuleHandler
.This then becomes to
$this->moduleHandler->validateUninstall($module_list)
. For other examples see the cross branch interdiff linked in #2324055-50: Split up the module manager into runtime information and extension information..Comment #24
chx CreditAttribution: chx commented> validateUninstall() has no dependencies on the installer
But it has an entity manager dependency which in turn depends on the module handler like any other plugin manager. Also it is not a runtime operation but an uninstall time only.
Comment #25
znerol CreditAttribution: znerol commentedSo in your opinion
validateUninstall()
needs to be onModuleInstaller
because if it were onModuleHandler
, then there would be a dependency loop? Note that in factModuleInstaller
also has a dependency onModuleHandler
, so I fear this would not help either.Comment #26
chx CreditAttribution: chx commentedIt would: you can inject module_handler into module_installer and entity.manager both without problems. The other direction would be problematic but it isn't so.
Comment #27
cilefen CreditAttribution: cilefen commentedThis is a reroll, again against #2324055: Split up the module manager into runtime information and extension information.. It needs the work suggested in #19 after, if there is a consensus on who injects who.
Comment #28
cilefen CreditAttribution: cilefen commentedChanges from #19. This patch doesn't break Drupal but it does not yet work.
Comment #29
chx CreditAttribution: chx commentedI think the only thing we had was misunderstanding not disagreement: there are no choices in what gets injected into what so it's kind of hard to disagree with it :)
Comment #30
xjmComment #31
xjmClosley related issue: #2338873: Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data.
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedOut of curiosity, what's the benefit of baking logic about content entities into
ModuleInstaller
, as the above does, as opposed to usinghook_system_info_alter()
(as forum.module does in HEAD per #10 and as #31 is proposing to do for fields)? Attaching a patch as a POC of thehook_system_info_alter()
approach. Setting to "needs review" for testbot, but will re-postpone because I'm not yet suggesting we deviate from #28, only inquiring about what's wrong with thehook_system_info_alter()
approach.Comment #34
effulgentsia CreditAttribution: effulgentsia commentedComment #35
effulgentsia CreditAttribution: effulgentsia commentedReuploading #28 and repostponing the issue. I'd love feedback on #32 / #34 though in order to understand why this issue is proposing the direction it is.
Comment #36
chx CreditAttribution: chx commentedYou are running 1 query per entity type on every cache rebuild (at least). How's that a good idea? Not to mention of firing the entity system while the module system is being rebuilt. That's a recipe for brittleness.
Comment #37
plachWhy don't we add an
hasData()
method to the storage class? (Actually I thought it was already there)That way it can be implemented in a storage-agnostic way.
Comment #38
chx CreditAttribution: chx commentedThat won't help the number of queries...
Comment #39
plachActually I was referring to the previous approach too...
Comment #40
chx CreditAttribution: chx commented@plach we have countFieldData() for fields but not an actual hasData exactly because that can be done with the query we are using here.
Comment #41
plachThe fact that we can do that with an entity query does not mean the ideal DX implies using an entity query. We could certainly use the EQ as the default method implementation, we use the same approach to implement entity loading by property (field) values IIRC. Anyway, let's not waste time on this, it was just a small remark I had after reviewing the last patches here, definitely not something that should block this issue :)
Comment #42
cilefen CreditAttribution: cilefen commentedComment #43
Wim LeersInitial review.
Incomplete docblock.
Missing newline.
$module
is a string,$this->uninstallValidated
is an array. This can't be correct.Should be injected?
array
typehint misssingComment #44
xjmComment #45
cilefen CreditAttribution: cilefen commentedComment #47
cilefen CreditAttribution: cilefen commentedI am not sure why this fails to install, Drupal\Core\Entity\Query\QueryFactory implements Drupal\Core\Entity\Query\QueryFactoryInterface.
Comment #48
cilefen CreditAttribution: cilefen commentedComment #49
Wim Leers#47 + #48: is that because
\Drupal\Core\Entity\Query\QueryFactory
doesn't implementQueryFactoryInterface
? I wonder why? Seems wrong indeed.Nit: needs newline between the single-line comment and the
@var
declaration.This should check if
\Drupal\Core\Entity\ContentEntityInterface
is implemented, notSqlContentEntityStorage
. There are several such examples to be found if you grep D8 forContentEntityInterface'
(yes, with that trailing single quote).Needs to be resolved before this can be RTBC :)
Comment #50
cilefen CreditAttribution: cilefen commentedThank you @Wim Leers. For #3, I think we could do an array_intersect_key() on the results of system_rebuild_module_data().
Comment #51
Wim LeersYep, #49.3 is the only remaining problem… besides this still needing tests, of course.
Using
array_intersect_key()
sounds good.Comment #52
dixon_I saw
hasData()
being discussed here above... Cross-linking to separate issue wherehasData()
is being implemented: #2335879: Change SqlContentEntityStorageSchema::requiresEntityDataMigration() to ask the old storage handler if it has data rather than assuming yes unless NULL storageComment #53
cilefen CreditAttribution: cilefen commentedComment #54
cilefen CreditAttribution: cilefen commentedExplanation: Using ContentEntityInterface, you have to filter out the entities with null storage or queryFactory->get() throws an exception.
Comment #55
cilefen CreditAttribution: cilefen commentedThe tests should go in the existing Drupal\system\Tests\Extension\ModuleHandlerTest or in a new Drupal\system\Tests\Extension\ModuleInstallerTest.
Comment #57
cilefen CreditAttribution: cilefen commentedComment #58
cilefen CreditAttribution: cilefen commentedComment #59
cilefen CreditAttribution: cilefen commentedComment #60
cilefen CreditAttribution: cilefen commentedAt this stage, we have minimally working code. I had to modify ConfigImportAllTest to remove all shortcuts so that module will uninstall when asked. Here is what drush does in this situation:
There are shortcuts:
Shortcuts were removed in the UI:
Comment #61
cilefen CreditAttribution: cilefen commentedFixed a bug found by @chx.
Comment #62
cilefen CreditAttribution: cilefen commentedAfter some discussion with @chx on IRC, we decided to go with a validation service.
Comment #63
chx CreditAttribution: chx commentedAlthough we have identified possible improvements for example rewriting validateUninstall to something like:
but also moving the code in uninstall form checking for requiredness and installedness into a separate service (or two? or is that excessive) those can be done in followups. Once we have this in we can nuke all (existing and forthcoming) hook_system_info_alter easily.
Comment #69
cilefen CreditAttribution: cilefen commentedComment #71
cilefen CreditAttribution: cilefen commentedRenamed $modules_no_content to $modules_validated.
Comment #72
plachCore SQL entity storage supports table layouts with no data table, so we should probably check for the base table instead, if the reason is the one explained in #54.
Comment #73
cilefen CreditAttribution: cilefen commented@plach: Thank you for reviewing.
Comment #74
plachNow that I think about it: how would the interdiff above play with a Mongo storage? I am not entirely sure this is the correct approach: the Contact message entity type has currenty null storage, but there's a contrib module to swap in a regular SQL storage. In a similar scenario I think the check above might be unreliable. Can't we replace it with a check to verify whether the storage is an instance of the Null storage?
Comment #75
Wim LeersPinged chx for #74.
Comment #76
cilefen CreditAttribution: cilefen commentedreroll
Comment #77
chx CreditAttribution: chx commentedIsn't
base_table
coming from the annotation? What does that have to do with the storage? (strange thing, that, but w/e) Also if it turns out it's necessary mongo will lie through its teeth about how beautiful tables it uses for storage. It is a very unscrupulous module -- it ties into Drupal as a SQL driver already (mostly extending the fake driver) -- so lying about storage is not a big deal.Comment #78
effulgentsia CreditAttribution: effulgentsia commentedHow hard would it be to fix #2337753: ContentEntityNullStorage does not implement a query service? If we don't want to hold this issue up on that, then instead of checking for NullStorage or a base table, how about we wrap that get() in a try/catch with an @todo linking to that issue?
Comment #79
chx CreditAttribution: chx commentedAfter a lot of consideration the only way is #37: add a
hasData
method toEntityStorageBase
the exact same code we have in this issue and thenNullStorage
can override it with a simplereturn FALSE
. Because it is possible that a storage service doesn't really have query capabilities but can say something about its own emptiness. To elaborate, most (I'd say all, I did research in a core issue prior) key-value stores have an iterable keyspace but the store itself doesn't have query capabilities (since you are likely to store the entity serialized some way). So while entity queries could be refused it is very easy to see whether the keyspace is empty or not.Comment #80
Wim LeersNW per #79.
Comment #81
Wim LeersActually, I think this means that per #52, this is now blocked on #2335879: Change SqlContentEntityStorageSchema::requiresEntityDataMigration() to ask the old storage handler if it has data rather than assuming yes unless NULL storage, which adds that
::hasData()
method.Comment #82
YesCT CreditAttribution: YesCT commentedadding blocker tag, since this blocks #2338873: Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data, so that the d8 blockers is accurate. Remove the blocker tag when this issue is fixed, and unpostpone 2338873.
Comment #83
YesCT CreditAttribution: YesCT commentedoops. tag.
Comment #84
cilefen CreditAttribution: cilefen commentedComment #85
cilefen CreditAttribution: cilefen commentedThis is a bit naive and it doesn't work, but I thought this was what I would get from #2335879: Change SqlContentEntityStorageSchema::requiresEntityDataMigration() to ask the old storage handler if it has data rather than assuming yes unless NULL storage.
Comment #86
Wim LeersComment #87
cilefen CreditAttribution: cilefen commentedComment #88
BerdirYou still want want to check for the ContentEntityInterface I think.
Comment #89
cilefen CreditAttribution: cilefen commented@Berdir: good point
Comment #90
cilefen CreditAttribution: cilefen commentedRemoved the now-unneeded EntityQuery service from ContentUninstallValidator.
Comment #92
cilefen CreditAttribution: cilefen commentedThank you @dawehner on IRC.
Comment #93
effulgentsia CreditAttribution: effulgentsia commentedThis is looking close. Thank you, @cilefen, for sticking with it.
Why such a low priority? The only reason I can think of is to avoid a query if some other, cheaper, validator returns FALSE for a different reason, but that seems like a premature optimization at this point, since we don't yet know what other validators we'll have. I'd suggest leaving priority out at the moment. Or else, pick either -10 or -100 and have a comment explaining that a <0 priority is chosen due to the expensiveness of the validator.
The
else
should have abreak;
, since why keep running validators after one fails.Also, is the
$this->uninstallValidated
cache useful? As written, it seems problematic, because we should only add to it if all validators return TRUE, not just one. But also, the result can change. For example, in a request, a ModuleInstaller service can be constructed, validate() called, but the module not uninstalled. Then, entities added, at which point subsequent validate() calls should return FALSE. Would it make sense to remove the cache entirely from this issue and only add it later, when we have a concrete situation in which it's helpful?Nothing in this code deals with priority; that's all handled by the service collector. So, I think the second sentence can be removed.
Needs an @return.
While we're in here, let's fix that code comment. There's no such thing as a disabled module anymore. And we should mention something about required and validated.
Comment #94
effulgentsia CreditAttribution: effulgentsia commentedAlso, s/$entities/$entity_types/ or inline the value into the foreach.
Comment #95
cilefen CreditAttribution: cilefen commented@effulgentsia Thank you for reviewing.
1: done
2: done
3: done
4: done
5: more could be done here
6: done
Comment #96
effulgentsia CreditAttribution: effulgentsia commentedThanks! I think all that's left is tests, so "needs work" for that.
Per #95.5, this could also be improved a bit more, but no need for that to hold up a critical.
Comment #97
xjmComment #98
fagoWhy this is restricted to content entities? If I'D implement an entity being no content, the same validation check should be there - right?
I suppose it's not needed for config entities right now as handled already via the config system, but even if so - would it be harmful to handle it via entities as well? If so, we could still blacklist config entities instead of white listing content entities.
Approach and patch looks good to me else.
Comment #99
bircherComment #100
effulgentsia CreditAttribution: effulgentsia commentedWell, the name of the validator is ContentUninstallValidator. The result of a failed validation is that the uninstallation form prevents you from selecting the module. So, we do not want this applying to config entities, since there, we do want you to select the module, and then after selecting it, we delete the config as part of the uninstallation.
I'm not sure about that. We only have two kinds of entity types in core: config and content. And we want different uninstallation logic for them. So how can we predict what a new kind of entity type in contrib will want? I think rather than adding validation for an unknown kind to core's validator, we should leave it to the contrib module that defines additional kinds to add its own validator if it wants that.
Comment #101
fagoGood point. However, you do not necessarily have to introduce a new kind of entity type, you could just opt to implement an entity which is neither content or config. In that case, we should apply the same check though or we'd have the same issue as we have with content entities now.
Thus imo, we should default to include the Validator for non-content non-config entities . But as you clarified, there needs a way to customize the behaviour for an entity type. So what about introducing a setting as part of the entity type, which has respective hard-coded defaults for config and content? E.g. $entity_type->forbidsNonEmptyUninstall() ?
Comment #102
effulgentsia CreditAttribution: effulgentsia commentedWorks for me. Either in this issue or in a followup, with a preference of followup unless it can be implemented and agreed to quickly, since #2338873: Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data is blocked on this.
Comment #103
bircherI was working on adding test coverage but discovered that just hiding the module with confiuration from the UI doesn't solve all problems.
There is still some problem with the test but since its commented out it should still work and can be reviewed and then put back to needs work.
To answer the question about what to validate, since we add a validator service here one could add another special-entity-validator, or anything really to prevent modules from being uninstalled...
Comment #104
bircherNow the tests are added and should pass.
Comment #105
bircherComment #106
alexpottThis is beginning to look really good.
format_plural is deprectated use
\Drupal::translation()->formatPlural()
- I know there are other usages of format_plural in this file but we should avoid added new usages.We should also pass the string translation service into the constructor and call
$this->setStringTranslation()
- will need to update service definition.Same namespace not needed.
Not sure why this change is here - it is also wrong - modules are installed and uninstalled. disable/enable is no longer used (yes we have a lot of docs to fix)
I think we need to key this by the module - and the each module might have several reasons.
I think we should just return an array empty - ie. there are no reasons to not uninstall.
Same namespace - not needed.
Missing test coverage of this text appearing on the module uninstall page
This comment could be expanded to describe why you are doing this.
Uninstalling... (capital U).
Here we should just uninstall help and assert that entity_test has also been uninstalled.
Comment #107
bircher1) Ok, I copied it from the line above. But refactoring the required_by into reasons gets rid of the other one too.
2) fixed
3) fixed
4) I'll leave it alone then, can be set right in another patch
5) fixed
6) fixed
7) fixed
8) fixed
9) fixed
10) fixed
11) fixed
Comment #108
fagoAs discussed with alexpott, this is probably enough of an edge case to require the entity-type implementing module to care about its own module uninstall validation logic. Howsoever, I'm fine with not handling it here.
Took a look at the patch and gave it a test run. It looks pretty good already.
Missing new line after <?php
Incomplete @param docs.
This uses hasData() but it's only available on dynamically fieldable entities. We should probably move it to EntityStorageBase such we can safely check it here.
Unnecessary, could be omitted.
I'm not sure about displaying messages here - this is an API method which should not write messages in case of errors. It could throw exceptions but the defined error behaviour is returning FALSE, so probably it should just do that.
Modules/drush who need to know the reason could still just invoke uninstall validation on their own - as the UI does.
This makes sense to have, but we are introducing a new API here without adding support for the other validations as part of it. According to the docs I'd expect other uninstallation problems to be returned also.
I'd suggest opening a follow-up for converting the other uninstallation checks to the new API as well.
Looks like it returns an empty array instead of NULL. Maybe that would be a good return value for the "all fine" case anyway, i.e. for entity validation you always get a list of validation fails and you have to check whether it's empty.
It doesn't add services, it adds uninstall validators. Whether it's a service or not does not matter to us.
Should be string[]
string[]|null
Misses trailing point. Should clarify when it does it return NULL.
Missing NL after php
If the interface is about modules only, maybe the name should reflect that and be ModuleUninstallValidatorInterface ?
Sentence need to start capitalized. Types should be string[] also.
missing "of"
Missing trailing point.
Is that still necessary? Looks like it's done otherwise in the test case.
Comment #109
fagoThen there is the issue that noData() is not required for content entities, opened a follow-up for moving it up to EntityStorageInterface (API change) #2391829: ContentUninstallValidator relies on the not required ContentEntityStorage::hasData() method.
Comment #110
catchAnother good reason to add the uninstall validate method and stop using hook_system_info_alter() -> #2387983: PluginNotFoundException when enabling module that provides text filter.
Comment #111
bircher5) I changed it to thorw a new exception.
6) Yes it will check all ModuleUninstallValidators that were attached, at the moment just this one, then in a followup we process all of them and the issue catch pointed out would be solved with it.
17) Yes we need to test that you can not uninstall a module that has a module which wants to uninstall a dependent module that has a reason for not being able to be uninstalled. Or in other words: we test that the dependencies are resolved before the validation.
All the others points should be fixed.
Comment #112
bircherComment #113
fagoThanks. Imo this is about to be ready. Only found two nit-picks:
Still misses trailing point.
Misses docs.
Also opened #2392363: [META] Make use of module uninstall validators.
Comment #114
fagoComment #115
fagoComment #116
bircherComment #117
fagoThanks!
Comment #119
Dries CreditAttribution: Dries commentedI'm glad to see this fixed, and that it will make other things more robust such as #110. Committed 8.0.x. Thanks all!
Comment #120
catchJust a couple of wording nitpicks.
The functionality itself looks great and really glad we're doing this.
I like the variable name. We can't install your module because reasons.
How doable is it to link to entity type admin page here?
Shouldn't we do a message per module here? Otherwise it'll be a bit of detective work to figure out which module is responsible for what - not always obvious.
Nice touch.
reasons prevent.
Comment #121
catchCross-posted. That can be a follow-up. Opened #2392533: Grammatical error in ModuleUninstallValidatorException message.
Comment #122
catchAlso no change notice yet.
Comment #123
effulgentsia CreditAttribution: effulgentsia commentedDo we have an existing open issue to also apply this new validation during config deployment (as a config validator)? If not, we should open that as well.
Comment #124
alexpott@effulgentsia I think this is partially covered by #2392351: When an entity bundle config gets deleted, entities of that bundle break - but we also have non-bundable entities and non config entity bundled entities to worry about. One issue that we can't pre-fire all uninstall validators before starting the import - because for example field delete is actually handled by import.
Comment #125
cilefen CreditAttribution: cilefen commentedDraft change record: https://www.drupal.org/node/2392677
Comment #126
fagoad #123,#124:
I agree we should run them to make sure non-config module uninstall validation tasks are run as well. Opened #2392815: Module uninstall validators are not used to validate a config import
Comment #127
plach@cilefen:
The CR is a bit sparse, can we briefly explain what a validator is and add before/after code examples?
Comment #128
cilefen CreditAttribution: cilefen commented@plach Thanks for looking at it. Better now?
Comment #129
plachThanks, published the CR with a small change to enable PHP code syntax highlighting.
Comment #130
BerdirCan someone who worked on this confirm that we can remove menu_link_content_uninstall() now? See #2312389: Remove menu_link_content_uninstall()
Comment #132
cilefen CreditAttribution: cilefen commentedComment #133
mwebaze CreditAttribution: mwebaze at JSI Research & Training Institute, Inc. (JSI) commentedHi,
I know this is rather late. Is there are way the 'ModuleUninstallValidatorException' exception can be caught so custom modules can implement the clean up using hook_uninstall? Would making ModuleInstaller a plugin be helpful so we can implement it in the custom modules?
Thanks,
Michael
Comment #134
cilefen CreditAttribution: cilefen commentedOpen a new issue please.