Problem

  1. After #61285: Remove poll module from core, Poll module was still enabled in system.module:enabled.

  2. DrupalKernel rebuilds the service container on every single request, because %container.modules% != system.module:enabled.

Details

  1. Modules are not supposed to just simply vanish from the filesystem while still being enabled, so that's just a "user error".
  2. However, the continuous/infinite rebuilding of the service container makes this issue a major bug.
  3. This even happens after executing the rebuild.php script.

Steps to reproduce

  1. Inject a bogus module name into the system.module:enabled config.
  2. Run rebuild.php.
  3. Verify that the container is not rebuilt on every page request.

Comments

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

At least in Drupal 7, the filesystem also gets scanned to look for missing modules every request, see #1081266: Avoid re-scanning module directory when a filename or a module is missing. Currently we don't warn anywhere about this, could be worth adding to system_requirements().

Crell’s picture

I suppose the first question is whether we want to gracefully handle modules going missing (rebuild the container once to eliminate the module and then save that, effectively force-disabling a missing module) or loudly notify the user of the problem and let them deal with it (similar to the "OMG you have a security release" message). I think there's a good case to be made either way.

sun’s picture

All I would have expected is an uncaught exception with the message:

Unable to find 'poll' module. Either restore its files (and uninstall it before removing them), or remove it from the system.module:enabled configuration and uninstall its configuration and data manually.

Crell’s picture

Well, we should probably give end users something a little nicer than an uncaught exception error if possible.

sun’s picture

  1. Can you clarify what you mean with "a little nicer"?
  2. I don't know whether it is safe to proceed with the bootstrap if a module vanishes. It might be safe to boot the maintenance theme and render an error page. But doing that will require a good amount of hacky code (directly in DrupalKernel) for this special case, which at least I don't want to support in the first place — removing enabled and actively used code from the filesystem and expecting things still to work as usual is a bogus expectation, and the system shouldn't even try to cope with that.
catch’s picture

I'd actually lean towards failing with an error message as well, it's a completely unsupported condition to be in.

If we do so, we might also want to present the ability to remove the missing module from config though via the UI (i.e. a callback linked from the error message?).

Crell’s picture

My concern is that not all module removals will cause the site to come crashing down, but we'd be forcing it to come crashing down for random site visitors if, eg, someone is moving a module around (from sites/all to sites/default or similar). I agree it's not a *supported* state by any means, but we shouldn't force it to crash harder than it does naturally.

A "yes I meant to remove that, trust me, update your records" link a la #6 sounds like a good feature to include.

sun’s picture

Issue tags: +Needs manual testing

Yes, moving a module to a different location should still be supported - somehow.

Since %container.modules% not only contains the module names, but also their filesystem locations, moving a module will apparently trigger the same error condition.

Thus, when implementing this, we definitely need to perform manual testing for moving a module in the filesystem. Can't be tested in an automated way, for obvious reasons.

I think that the error condition should be smart enough to essentially call dfac(), which contains the required logic for coping with modules that have been moved in the filesystem. That's more or less the reason for why I cross-linked #1872522: Compiled data in PHP storage is cleared too late in drupal_flush_all_caches() in the issue summary.

I'm not sure whether I agree with putting a action link into that error is a good idea. Here's why:

  1. It involves security conditions — which cannot be performed without bootstrapping the kernel, and alas, if any module vanished, we cannot know the state of security anymore.
  2. Executing that callback equally requires security checks, and additionally, some higher-level services to actually execute the operation.
  3. Blatantly removing the module from the list of enabled modules without properly uninstalling it is NOT what you want in ~100% of all cases. If we're trying to be "helpful" for end-users with this, then that's the exact opposite of helpful. We don't need to repeat the entire discussion of #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed — all of the points over there apply identically here.
jmarkel’s picture

Before getting too crazed about what one ought or ought not do re deleting modules, one should keep in mind the situation (which I have found myself in more than once) where a contrib module 's code or configuration is so borked that the only way to recover is to delete the module altogether. In such circumstances the last thing one would want is a system that won't function until the module is put back, since it's only when the module is NOT there that the system will function at all. (In case you're wondering, I can't think of a specific example, but it has happened to me, particularly when I was a bit more of a Drupal noob than I am now).

I.e. I'd say a better course would be Crell's (from #7) - warn the user at the very least, and let the system continue running if it's able to.

sun’s picture

The concrete proposed exception message in #3 provides sufficient details to allow everyone to recover from a completely hosed system, in case they're unable to restore the files and perform a proper uninstallation procedure.

So I'm a little lost, both in terms of "little bit nicer", as well as "consider edge-cases, please" — #3 gives (admittedly packed but) concise instructions for how to recover. This scenario won't happen on production sites in the first place, only on developer sites, on which someone screws with the filesystem.

That said, next to moving modules in the filesystem, the same situation is able to happen when configuration is staged. Most likely, the answer for that means to stage + import configuration changes first (since otherwise, module info + APIs are no longer available), and only after properly staging + uninstalling, you're allowed to remove a module. (Sounds sensible to me.)

Crell’s picture

It would be so nice if everyone using Drupal ran a proper dev/stage/prod split like we tell them to, but many many many people do not. For one thing, Drupal <=7 makes that prohibitively hard for people who aren't dedicated to that workflow on principle. That's pretty much the exact problem that CMI is trying to solve, but we have how many hundreds of thousands of people now trained to Do It Wrong(tm) because Drupal made doing the right thing harder than doing the wrong thing? We cannot simply abandon them to broken sites.

The message from #3 may be all well and good and useful to someone who is using the file system of his site and knows what he's doing and what system.module:enabled means.

But right now, sun, it sounds like your suggestion is that if sharelinks module goes missing (worst case right now, your "Tweet this" link doesn't appear, too bad so sad) anonymous site visitors get a maintenance page with that message in it. That is, as you like to say, "bogus".

catch’s picture

This scenario won't happen on production sites in the first place, only on developer sites, on which someone screws with the filesystem.

This almost certainly will happen on production sites - those that did a force-removal of a module several versions ago and ran in that state for a long time. Unless we somehow lose information about these modules during the {system} -> CMI upgrade path but I'd expect it to get copied across as-is.

tstoeckler’s picture

I still don't see how seeing

Unable to find 'poll' module. Either restore its files (and uninstall it before removing them), or remove it from the system.module:enabled configuration and uninstall its configuration and data manually.

is a problem though in that case, even for non-developers. You download 'poll' module and everything is back to normal.

tstoeckler’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

@tim.plunkett just discovered that #2161995: Faulty module cannot be removed from module list; DrupalKernel rebuilds container on every page request was a dupe.

Both issues contain some valuable info, and even though the other has an initial patch, this one holds much more in-depth considerations on the fundamental problem space itself, so I've closed the other one.

Will revisit this shortly.

Berdir’s picture

sun’s picture

Berdir’s picture

Not sure how #2228215: Remove module check in DrupalKernel affected this exactly but I think the rebuilding was removed, so it's still broken if this happens...

mgifford’s picture

Assigned: sun » Unassigned
jhedstrom’s picture

Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs issue summary update

Removing an enabled module no longer causes the service container to be rebuilt on every request, so if there's anything left to be done, this needs a summary/title update.

dawehner’s picture

Status: Postponed (maintainer needs more info) » Fixed

The actual issue got solved in the meantime, we don't just randomly rebuild the service container on every request. Rebuilding is now explicit, so I think this is solved.

Status: Fixed » Closed (fixed)

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