update.php fails when optional modules disabled

This is a critical bug. If you do the standard profile and then disable color.module you can't update. This is a regression from how things work D6.

How's this for a basic fix?

I use a regex for the lack of a better flag identifying a profile.

Do we need test for this?

Steps to reproduce before patch:

1) Install using standard install profile
2) Disable Overlay module
3) Visit /update.php
4) Cant proceed

Unresolved dependency	- Overlay (Disabled)
Standard requires this module.

Added functional tests.

Are you sure that module_disable() should disable the block module? Even if there's dependent 'Dashboard'?

yes, I wrote the dependency-checking logic for module_disable() and enable ;) with the second parameter set to FALSE, we force disabling only the module specified and not the dependent chain.

Yes, I think that would be another issue. I'm not sure whether to make an exception for core modules or not, might be better to be consistent and update all modules.

#5: update-remove-profile-requirements.2.patch queued for re-testing.

Hm. This will break the situation where someone adds a new dependency to their install profile and expects the update script to check it on update, but I'm guessing that's going to be a less common problem than someone wanting to disable one of the default enabled modules.

preg_match() is slow, however. Can we safely replace that with a strpos? There's no way to name a module something.profile.module, right?

This will break the situation where someone adds a new dependency to their install profile and expects the update script to check it on update

That's not really supported at the moment. Dependencies in install profiles are install-time only, IMO this is a bit of a mis-use of the dependency system and we should be able to distinguish between modules to be installed with the profile vs. modules actually required by the profile, but that realisation came far too late for D7.

system_requirements() does enough expensive checks that I don't think preg_match() vs. strpos() will make any difference, however it's such a simple check it makes sense for it to be a strpos() there anyway.

Changed preg_match() to substr() with a negative offset. I concur with catch, if a profile dependency is added, that should only affect the requirements for new installs, since the profile is really an install script.

Oops :) Thanks for rerolling. Let's get this in so folks can at least disable stuff and then update stuff.

I'll follow up in #799314: Checking/resolving new dependencies for updated modules with some other issues I found with system_requirements().

Weird. I still don't understand why we can't just use strpos here, since "." is not a valid function character.

I'm sorry, but I disagree here.

The dependencies of the installation profiles should be hard dependencies. You should not be able to disable them, as any other dependencies. Installation profiles could have 'soft' dependencies (ie.: modules that are enabled during the installation, but can be disabled later), but in that case, they have to module_enable() those themselves.

To me we are fixing up the wrong thing here: the actual problem is that standard.profile lists the overlay (and many other modules) as a hard dependencies, while those are merely soft dependencies that it should enable manually.

I'm not too fond of the idea that profiles use module_enable() manually, since then we wouldn't take advantage of the batch API. I also disagree that profiles should have 'hard' dependencies that can't be disabled... I don't see a good case for that, at least in D7. The install profile sets up an initial configuration, and if you want to change it, you should be able to. I think a profile (after installation) can't assume its "dependencies" are enabled, and should check module_exists() unless the module has required=TRUE. It's implied that they are only dependencies during the install process.

@Damien. While I agree with this in principle, I think it's 'by design' at the moment, even if that design is completely broken. http://drupal.org/node/562694#comment-2777880 is where I ran into it. #509392: Introduce .info files for install profiles is the original issue where it was pretty much ignored. I think this is doable for Drupal 7 (now that I remembered we use .make for d.o packaging), but it's an API change and requires a few related issues to be fixed, I opened #820054: Add support for recommends[] and fix install profile dependency special casing and I'm marking this issue postponed on it - if that gets moved to Drupal 8 then we can re-open this one.

Crossposted with carlos, the batch API issue is a good argument - it was impossible to install system module on godaddy until a couple of weeks ago, so installing 20-odd modules at once is never going to happen, and that's precisely the kind of refactoring we're not going to get done in Drupal 7.

So let's unpostpone this. It'd be good to at least add a @todo in this patch pointing to that issue though.

The easiest solution would be to restore a hook_profile_modules(), just renaming it hook_profile_optional_modules() (or something like that).

I have a slight preference for substr() here since with strpos() we lose accuracy by searching for .profile anywhere in the string instead of exactly at the end. Minor point :)

"since the profile is really an install script."

NO. Thats what they were until D7. Now, they can carry update functions and participate in hooks and all that. They are live breathing modules and I expect that sites will use profiles to deploy their updates to live environment (for example).

Having said that, I don't really care if we call these profile dependencies "hard" or not.

@moshe, thanks for correcting me. I haven't been involved in the re-envisioning of profiles, but I'm willing to help at finishing it.

An API change is needed to let profiles specify both hard and soft dependencies. I'd suggest having profiles specify something like modules[] or optional_modules[] in their .info files. Then marking the actual dependencies[] on the modules page as required by the profile. Can we get approval for this API change and implement it in #820054: Add support for recommends[] and fix install profile dependency special casing or just go with the quick fix for now?

Confirmed overlay module disabled generates error message. Applied 808162-remove-profile-reqs.patch and retested. Drupal updates without error.

#ladrupal Code Sprint

-- Mark

I put my opinion about the API change over at #820054-3: Add support for recommends[] and fix install profile dependency special casing. Still need a re-roll of this patch with strpos() or someone to explain why this can't be done. Should be pretty simple.

@webchick strpos() could not be used here because profile-name can contain .(dot) (some.profile.profile) it's not function-ware it's about file-name so substr($file->filename, -8, 8) check LAST 8 chars of filename.

Well, I meant more that since in order for a profile's hooks to fire (remember, profiles are essentially "special" modules now), the profile would need to be named in such a way that the profile name only contained valid characters for a PHP function; function some.profile_install() would create a parse error.

I suppose that it's remotely possible that someone somewhere could create a something.profile.profile that just does dependencies and no hooks, but this seems highly doubtful. I'd actually be kind of surprised if module_invoke didn't choke on it before that.

But given the choice between ugly substring counting and a preg_match(), I went with preg_match() since as catch points out this isn't a performance-critical part of the code. I committed #5 to HEAD, so we can knock out a critical. I'd really love a follow-up that just does what I asked for, though. ;)

But given the choice between ugly substring counting and a preg_match(), I went with preg_match() since as catch points out this isn't a performance-critical part of the code. I committed #5 to HEAD, so we can knock out a critical. I'd really love a follow-up that just does what I asked for, though. ;)

How about this for a followup?

#829378: Update system should use the API to identify the install profile, rather than preg_match()

I'm pretty sure that not only can we avoid preg_match(), but also can avoid any kind of string-parsing at all...