Problem/Motivation

Install profiles specify modules to install as dependencies[]. These are not enforced as hard dependencies though and can be disabled. (#808162: update.php fails when optional modules disabled for background). If a distribution wants to keep the install profile module around, tough luck. There's no mechanism right now to discern between "the install profile module is recommended" and "the install profile module is required to make the distribution work". This limits distrbutions as for the latter purpose another module needs to be maintained, confusing users and making the life of the distribution maintainer harder.

Proposed resolution

-Convert dependencies[] to hard requirements for install profiles.
-Removes all special casing that undoes it
-Ensure dependencies[] behaves the same for modules and install profiles
-Introduce support for recommends[]. This will install any module that is available in the modules system (just as dependencies[] does) but these modules can later be disabled/uninstalled. (Note: If a user git clones an install profile the dependencies will not be downloaded and this could lead to errors during install if dependencies were not downloaded. See comment #39)

Remaining tasks

-Re-roll patch in #26 replacing modules for recommends
-Add recommends[] support to module_enable()

User interface changes

-Add indicator in the UI to show that x module recommends y module.

Original report by catch

Install profiles specify modules to install alongside them as dependencies[], however they're generally not enforced as hard dependencies, see #808162: update.php fails when optional modules disabled and linked issues for background.

To fix this, we need to

* ensure that install profile dependencies are shown on admin/modules (note: #293223: Roll back "Hide required core modules" has been committed)
* update the core install profiles to always use hook_enable()
* document the API change for existing contrib profiles.

When the original patch went in, the idea was to use .info files to gather dependencies for Drupal.org packaging, however as far as I know this now uses .make files, so it shouldn't affect that at all, although it'd be good to have that confirmed, I've only half-followed the packaging work.

**Summary built from comments by catch in #37 and David_Rothstein in #39.

Files: 
CommentFileSizeAuthor
#71 drupal_820054_71.patch7 KBXano
FAILED: [[SimpleTest]]: [MySQL] 57,120 pass(es), 16 fail(s), and 8 exception(s). View

Comments

carlos8f’s picture

I'm not clear on how profiles would use hook_enable(). The modules need to be installed in a proper batch... Damien suggests adding a hook_optional_profile_modules(), but I don't see why we couldn't just add modules[] or optional_modules[] to the .info file, to keep the .module code minimal. We can safely put this off to D8, right? I think it's OK in D7 that we treat profile dependencies only as such during the install process.

catch’s picture

We have at least two critical bugs where the dependencies being enforced on runtime, (or not enforcing them) has caused side effects with other code - it's impossible to hook_system_info_alter() dependencies[] for a required module, and have that work at the moment, and the update.php bug, there may be other spots too.

In both cases we can only add more workarounds - since the same API means two different things at the moment, and is enforced inconsistently in different places.

I would like to fix it here, it's an API change though, so the question is whether we continue to add workarounds and special cases in D7, or break the API. Having to add modules[] or optional_modules[] to .info files makes it seem like more of a D8 task though.

webchick’s picture

Version: 7.x-dev » 8.x-dev

I am not real keen on changing the API at this point to add the concept of "soft" dependencies. It's way late in the cycle for such a thing, and it's not clear to me what sort of changes would have to happen on the d.o back-end to support them now that we have all the packaging stuff. IMO, this is D8 material.

And though we refactored install profiles this release to behave as modules do and utilize the same hooks, updating, etc. (which is a good thing!), they're still fundamentally not the same thing. "Dependencies" in the context of an install profiles IMO still means "stuff required to get Drupal up and running", not "stuff that this profile will force the site to always have turned on even though there is no way to go back and change what profile you have installed without futzing with the database". That doesn't make any sense to me.

If the profile does have the need of such hard dependencies, it's trivial to add a required module with the "hard" dependencies to the list of your profile's dependencies so it's enabled on install and subsequently "locked" in. I don't see this as a horrifying workaround, especially at this stage, and it's what we've been doing since D5.

Short version: I'm fine with the workaround in #808162: update.php fails when optional modules disabled.

catch’s picture

I was already quite resigned to fixing this in D8 but just a couple of clarifications:

1. Drupal.org packaging, afaik. is using .make files and doesn't use dependencies[] at all, so there shouldn't be any infra changes required.

2. If you add a module as a dependency of an install profile, then make other modules depend on that, then the original module itself I don't think will be required - because that's a 'soft' dependency same as all the others, so there's not much in the way of workarounds for this if you really, really need it.

webchick’s picture

1. D'oh! I missed that in your initial post, sorry. :( It'd be good to have confirmation from the infra team, but that sounds about right.

2. Sorry, I apparently didn't explain myself properly. What I mean is this: if I'm an installation profile author, and I care about "hard" dependencies, I can create my own module with required = TRUE and dependencies[] = whatever in its .info file and then specify it as a module to enable by default in my installation profile. This is what I've been doing for this since Drupal 5.

The list of modules in installation profiles have always traditionally been only about "enable these things on set up" not "make these things hard dependencies." The fact that these modules are now specified via "dependencies[]" in the profile .info file doesn't change this fact, IMO. This was a simple syntactical change to make profiles and modules more unified in their approach to ease the learning curve.

webchick’s picture

Category: task » feature
Priority: Critical » Normal

In fact, I'd go one further with this. This seems clearly a feature request to me, and not one I'm even sure makes sense. Having two ways of specifying modules to enable on set up for installation profiles seems like a DX regression to me.

catch’s picture

I can create my own module with required = TRUE and dependencies[] = whatever in its .info file and then specify it as a module to enable by default in my installation profile.

No, you can't:

1. Required modules are hidden from admin/modules

2. If we unhide required modules from admin/modules, then requirements kick in for profiles too - see the post where I originally ran into this whole series of bugs at http://drupal.org/node/562694#comment-2777880

Which is why it is so horribly, horribly broken as it is in HEAD.

webchick’s picture

Oh dear lord. :P~

Ok, let's take a look at #562694: Text formats should throw an error and refuse to process if a plugin goes missing then and see what we can do. I still think this approach is not the right way to fix it, but we'll see.

webchick’s picture

Oh, and fwiw, I hate from the very pit of my soul that we completely hide required modules from the modules page and don't display their dependencies, so if this creates a legitimate critical bug reason for rolling back that stupid behaviour, then bonus. :P

Sounds like we need to special-case profiles in http://api.drupal.org/api/function/system_modules/7, too though (or go the system_info_alter() route as you did there). I can see what you're getting at with this becoming a slow avalanche of special-casing for install profiles, but atm it still seems preferable to changing the API so dramatically at this point.

catch’s picture

I think at the moment it'd just be update.php and system_module() (or at least that's all I know about), so yeah it's fixable without changing the API, it's just stacking up in a fair number of places at this point.

yoroy’s picture

Oh dear, hiding required core modules hides important info when/how please? I could probably tolerate a collapsed fieldset with the core required ones, you know, at the bottom somewhere. Maybe :) We probably would have if we managed to do something with the new/on/off/ sorting idea. I'd hate to frustrate distribution/packaging efforts.

David_Rothstein’s picture

Title: Install profile dependencies should be real dependencies » Install profile "dependencies" are inconsistent with regular modules dependencies
Version: 8.x-dev » 7.x-dev
Category: feature » bug

I think if we retitle this slightly it is neither a task nor a feature request, but rather a bug... Whether or not it can be solved in an acceptable way for D7 is another question, of course :)

The problem is that even if we add special-case code like in #808162: update.php fails when optional modules disabled all over core that doesn't really fix the problem for real, because anyone trying to use the API for dependencies is going to get tripped up by this. Suppose I want to write some code that shows me a list of all modules that are required due to other module dependencies - well, if I simply array_merge() all the 'dependencies' for all modules on the site then my list is incorrect, unless I remember to add a special case for the install profile in my code too. It is a bug in the API.

Fundamentally I think the bug is related to this:

mysql> SELECT name,filename,type FROM system where name = 'standard';
+----------+------------------------------------+--------+
| name     | filename                           | type   |
+----------+------------------------------------+--------+
| standard | profiles/standard/standard.profile | module | 
+----------+------------------------------------+--------+

We store in the database the fact that the install profile is of type 'module', and if so, it needs to fully act like one. Now, one could argue pretty reasonably that that should be type = 'profile' instead (yes, I know install profiles can implement hooks and all that in D7, but so can themes, and that doesn't mean we pretend a theme is a module when it isn't). That would be too late to change in D7, so the alternative is that we treat it like an actual, bona fide module, and if so, it's a bug to have 'dependencies' mean one thing for some modules and a different thing for others.

I think something similar to @carlos8f's idea is simplest: Just add a new key like modules[] to the .info file, in addition to dependencies[]. It's an API change, and if that's really unacceptable then there's not much we can do, but (a) it's a very simple API change that should take about 3 seconds for people who already wrote D7 install profiles to fix in their code, and (b) it's not really a new API change so much as a followup to an API change that was made months ago but whose implications were apparently not fully understood at the time.

By the way, reverting the hiding of required modules is in progress here: #293223: Roll back "Hide required core modules"

carlos8f’s picture

Yes, with modules[]/dependencies[] in $profile.info, #293223: Roll back "Hide required core modules" (required message saying "Required by: Profile name X"), we have a nice working system and can remove cruft like #808162: update.php fails when optional modules disabled. The change to install.core.inc/simpletest DWTC::setUp() to merge and install modules[] + dependencies[] would be really easy to do.

One thing I've been pondering is if all the above is in place, we might be able to get rid of required=TRUE, instead those are just dependencies[] of a profile. The reason is it doesn't really make sense for a module to be required in itself, apart from other modules or profiles. But standard.profile requiring node.module (optional for other profiles) makes perfect sense.

webchick’s picture

Now, one could argue pretty reasonably that that should be type = 'profile' instead (yes, I know install profiles can implement hooks and all that in D7, but so can themes, and that doesn't mean we pretend a theme is a module when it isn't). That would be too late to change in D7...

Hm. Why? That certainly seems like a much more reasonable fix than changing the developer-facing API at this point, and it helps clear up a WTF. Is there some sort of downside I'm missing?

webchick’s picture

And the stuff described at #13 around removing required=true sounds like major refactoring with questionable benefit. I don't see why we would want to force every install profile to have to re-declare node and system modules as a dependency when huge swaths of Drupal will break without it.

carlos8f’s picture

Removing required=true is obviously a D8 idea, the direction being making Drupal more of a packagable framework, and using dependencies instead. Maybe the benefit is questionable, I just wanted to bring it up.

I'm not sure if type = 'profile' would be any easier, and might lead to even more special casing to get the profile to act like a module in the right places. I guess the next step is to try writing a patch (for either solution) and see where we get.

David_Rothstein’s picture

Yeah, my though was that moving to type = 'profile' would probably involve a lot more code in a lot of different places (all throughout the API for invoking hooks, etc). However, @webchick might be right that it would mostly be internal changes.

The difference, though, is that whereas changing the meaning of 'dependencies' is a small, focused API change, changing the fact that the profile is stored as a module or not seems like it could potentially have more wide-ranging and harder-to-predict effects elsewhere. I don't really know for sure, though.

catch’s picture

The original patches to change profiles were quite far reaching, so I have the same overall feeling as David here. One is a relatively simple fix with only small API repercussions, the other could end up with a tonne of refactoring spinning around.

carlos8f’s picture

Status: Active » Needs review
FileSize
3.49 KB
PASSED: [[SimpleTest]]: [MySQL] 20,924 pass(es). View

Here's a tentative implementation. The check added in #808162: update.php fails when optional modules disabled is now unnecessary, so I removed that.

One thing I noticed while writing the patch is that the installer doesn't actually sort the modules before installing them, which is a little disturbing. I think before sending them to the batch they should be ordered by the "sort" property added by graph.inc / system_rebuild_module_data() to make sure nothing breaks. The order modules are specified in .info shouldn't really matter, in terms of dependencies being ordered first. I'll probably open another issue for that.

carlos8f’s picture

Created #833192: Installer might install modules in wrong order, which would help this issue a little by allowing profiles to specify modules[] and dependencies[] in any order.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev

I guess we are in Drupal 8 territory here...

Another interesting way in which profile "dependencies" are inconsistent with module dependencies is that they don't support including version strings as part of the dependency, as modules do. For example, dependencies[] = some_module (2.x) (see #211747: Allow specifying version information as part of module dependencies).

Of course, I discovered that that feature even exists while reviewing #1049116: module_enable() doesn't account for version strings in dependencies[], which shows that the feature doesn't even work correctly for modules :)... but it is a feature that module dependencies are supposed to fully support, and the profile "dependencies" don't even remotely do so.

Mark Trapp’s picture

boombatower’s picture

indeed, subscribe

catch’s picture

Priority: Normal » Major

This caught out drush and led to a critical core bug report #1170362: Install profile is disabled for lots of different reasons and core doesn't allow for that.

Bumping to major.

catch’s picture

FileSize
2.98 KB
PASSED: [[SimpleTest]]: [MySQL] 32,185 pass(es). View

Re-roll of #20.

sun’s picture

Status: Needs review » Needs work

The proposed change from dependencies[] to modules[] does not feel right to me. Especially in light of D8+, where we actually need to go in the opposite direction.

AFAICS, we need to fix install_system_module() and make it perform the same weighting of dependencies as module_enable() does.

catch’s picture

Status: Needs work » Needs review

@sun, this has nothing to do with weighting of dependencies, it's about the fact that install profile dependencies are not dependencies at all. All dependencies[] does in .install files for profiles is provide a list of modules to enable (it's not even used for d.o packaging).

Then due to that, everywhere else in core (and drush) has to special case install profiles dependencies to stop them from working again.

That has nothing to do with ordering of dependencies or anything like that.

sun’s picture

Title: Install profile "dependencies" are inconsistent with regular modules dependencies » dependencies[] in install profiles are not dependencies; they can be disabled

Before reading up on all comments on this issue, this is what I originally wanted to write, but now I see that it's bogus:
______________________________
That only describes how broken the current system is. The current patch would manifest the situation, AFAICS. There are at least two issues that validly complain about dependencies[] not working like they should (possibly more):

#1093420: Recursive module dependencies of installation profile are not enabled in DrupalWebTestCase::setUp
#1057412: Testing of modules in profile

(found those after running into the exact same issue with #375397: Make Node module optional)

So,

  1. this patch gives the impression that we're ignoring the problem instead of fixing it. dependencies[] in install profiles should work like dependencies[] are supposed to work. Otherwise, we're introducing a major DrupalWTF for install profiles.
  2. I'm especially opposed to introducing a special notion of modules[], as we've identified in many different areas that the notion of modules and themes need to be unified into extensions, including low-level registration and behavior. Prime examples for that are #371375: Never name a module and a theme the same name!, #1067408: Themes do not have an installation status, #474684: Allow themes to declare dependencies on modules, and others.

______________________________

First and foremost, the issue title confused me. So, better title (hopefully). At least, it seems to be more in line with what the patch attempts to do.

The sole fact that dependencies[] of install profiles are not necessarily true dependencies[] (because they can be disabled) means that we have to introduce a new .info property for profiles. I'm still opposed to modules[], so I'd rather think along the lines of redhat/debian (?) package properties, as already proposed in #328932: Modules and partial dependencies - enhances[] and enhancedby[] field in modules' .info.yml files, #92233: Modules in conflict - conflicts[], breaks[], brokenby[] field in modules .info file, and more commonly http://drupal.org/project/module_supports :

; A hard dependency, as you'd expect.
dependencies[] = node
; A soft dependency; actually not a dependency at all.
recommends[] = contextual

Regarding the install profile itself and changing {system}.type into 'profile', I actually believe that most aspects have been cleaned up already. I'd imagine that the only required changes would be to module_list(), _system_rebuild_module_data(), drupal_alter(), and drupal_get_filename().

David_Rothstein’s picture

I don't think recommends[] is quite right because they're more than just recommended; they're still required, but only at install time. So maybe something more like installs[] or install_dependencies[]?

Or perhaps we could just remove them from the .info file entirely? Per #1 above, Damien suggested a hook_optional_profile_modules(), which of course is very similar to hook_profile_modules() which we had in D6.

Overall, I'm not sure what the benefit was of moving them to info files in D7 anyway, to be honest. Having them there means you can't put any code logic around them; e.g. if you want to write an install profile that automatically "inherits" from another profile (by installing the same or similar list of modules) you could easily do that in D6, but in D7 it's extremely awkward and close to impossible.

webchick’s picture

The idea was to make profiles behave more as modules do, for better DX. Lots of people know how to write modules, but writing install profiles was always this process of learning janky hooks for things that would just be .info file properties, lacking basic fundamental capabilities like _alter() hooks and the ability to perform database updates, etc.

In retrospec though, it looks like in this case we went a bit too far.

catch’s picture

If we had something like install[] that could possibly be made available to modules too, would mean install if available. Could also leave dependency support in but remove the special casing so it works like everywhere else.

catch’s picture

I'm looking at http://www.debian.org/doc/debian-policy/ch-relationships.html

Depends
This declares an absolute dependency. A package will not be configured unless all of the packages listed in its Depends field have been correctly configured (unless there is a circular dependency as described above).

The Depends field should be used if the depended-on package is required for the depending package to provide a significant amount of functionality.

The Depends field should also be used if the postinst or prerm scripts require the depended-on package to be unpacked or configured in order to run. In the case of postinst configure, the depended-on packages will be unpacked and configured first. (If both packages are involved in a dependency loop, this might not work as expected; see the explanation a few paragraphs back.) In the case of prerm or other postinst actions, the package dependencies will normally be at least unpacked, but they may be only "Half-Installed" if a previous upgrade of the dependency failed.

Finally, the Depends field should be used if the depended-on package is needed by the postrm script to fully clean up after the package removal. There is no guarantee that package dependencies will be available when postrm is run, but the depended-on package is more likely to be available if the package declares a dependency (particularly in the case of postrm remove). The postrm script must gracefully skip actions that require a dependency if that dependency isn't available.

Recommends
This declares a strong, but not absolute, dependency.

The Recommends field should list packages that would be found together with this one in all but unusual installations.

It seems like we could use recommends[] to mean that there is not a hard dependency (i.e. allow those modules to be disabled), but if they're available in the file system, enable them at the same time if they're not already enabled. To me this fits closely enough to what the debian spec says.

sun’s picture

Exactly. That's what http://drupal.org/project/module_supports introduces as well.

However, there'd still be a difference then:

  • for modules, recommends[] would only recommend to install another one without installing it.
  • for installation profiles, recommends[] would actually install the recommended module.

Thus, installation profiles don't have a way to recommend modules like modules do.

catch’s picture

Well I meant if the module is in the file system, we'd actually install it if recommends[] was used. This means modules in the same project, you'd always get them enabled, modules in different projects, would depend on what you had available.

That could end up annoying though if lots of modules started using it and you had to go through and disable some again (bit like disabling the overlay every time you install D8).

If you wanted to recommend something without it being automatically installed, there is still suggests and enhances available.

If none of this fits, then we've got a good reason to go with something custom, maybe enable[]?

sun’s picture

oh, you left out those -- indeed, that would work IMHO:

Suggests

This is used to declare that one package may be more useful with one or more
others. Using this field tells the packaging system and the user that the
listed packages are related to this one and can perhaps enhance its usefulness,
but that installing this one without them is perfectly reasonable.

Enhances

This field is similar to Suggests but works in the opposite direction. It is
used to declare that a package can enhance the functionality of another
package.

catch’s picture

So actual proposal:

dependencies[] - we make this a hard requirement for install profiles, remove all special casing that undoes it - behaves exactly the same for modules and profiles.

Introduce support for recommends[] - this will install any module that is available in the modules system (same as dependencies[]) but you can disable and uninstall those modules same as usual. This is what currently exists for install profiles, we keep that same behaviour, but allow modules to do the same thing for consistency. fwiw if you git clone an install profile, you won't get all the dependencies, wonder what happens in that case with dependencies[] and the installer..

For 'you might want to use this', suggests[] still exists in the debian spec, so the other issue that wants to add support for this on d.o can use that. This issue doesn't get into that at all.

This means if we went this way, the following is likely needed:

- re-roll the patch for s/modules/recommends/
- add recommends[] support to module_enable()
- do we need an indicator in the UI to show that x module recommends y module and that enabling one enables the other? I would love it if we could just introduce the API support and leave the UI for the recommends/supports/enhances issue but I dunno.

catch’s picture

Status: Needs review » Needs work

I'll try to look at a patch for these two so we can see what it looks like, ought to be relatively straightforward hopefully.

- re-roll the patch for s/modules/recommends/
- add recommends[] support to module_enable()

David_Rothstein’s picture

Introduce support for recommends[] - this will install any module that is available in the modules system (same as dependencies[]) but you can disable and uninstall those modules same as usual. This is what currently exists for install profiles, we keep that same behaviour, but allow modules to do the same thing for consistency. fwiw if you git clone an install profile, you won't get all the dependencies, wonder what happens in that case with dependencies[] and the installer..

What happens is that Drupal won't let you install due to the missing modules.

As I said in #30, what you're proposing for recommends[] is not actually the same behavior that currently exists for install profiles.

It might make sense to just change the behavior of install profiles to work that way, but a side effect of that is that currently, a profile's hook_install() can assume that all the profile modules are installed and enabled when it runs. With this change, profiles would potentially have to stick module_exists() all over the place in their hook_install(), and doing that would kind of cripple them - since the point of an install profile is to be able to configure the site initially exactly the way it wants to.

On the other hand, to the extent this only affects people who clone a profile using Git (since tarballs from d.o. have the profile modules already in them), we could just decide to ignore the problem; i.e., people who use Git are responsible for downloading the right modules on their own and assumed to know what they're doing, and if they don't they might get fatal errors in hook_install() rather than the friendlier message from install.php that they currently get.

catch’s picture

For #39 that all sounds right, and yeah I'm hoping the latter will be fine since install profiles are packaged (and presumably the .make file will work with drush make from a git checkout? I haven't used that yet..).

DamienMcKenna’s picture

An oddity I just ran into with D7:

  • Rules (7.x-2.0-rc1) was downloaded.
  • Entities API was not downloaded.
  • Rules was listed as a dependency on the install profile, Entities API was not. Note: Entities API is a dependency for Rules.
  • The install ran fine, it installed all of the other modules & no errors showed up, except that once it tried to bring up install.php?op=finished the Rules module gave an error that one of the Entities API classes could not be loaded.

Should I open a separate ticket to deal with this or keep it within this larger issue and aim for a backport of the eventual fix?

catch’s picture

That sounds like a separate issue to me. I have a feeling dependencies of dependencies of install profiles are completely ignored.

DamienMcKenna’s picture

catch’s picture

Title: dependencies[] in install profiles are not dependencies; they can be disabled » Add support for recommends[] and fix install profile dependency special casing
Component: base system » install system
Category: bug » task

While this inconsistency has caused numerous bugs in Drupal 7 this isn't actually a bug, just a wtf, switching to task.

nedjo’s picture

In D7, an install profile's dependencies will be enforced (at least through the modules admin UI) if you explicitly unhide the install profile by putting

hidden = FALSE

in its .info file.

Edit: in which case you'll probably also want to put

required = TRUE

so the install profile can't be disabled.

joelcollinsdc’s picture

@ #45, wow good catch.

Not sure if issue follower counts are added up somewhere but i'd like to vote +1 for fixing this.

sun’s picture

I'd love to move forward here. Any takers? The issue summary should also get updated with #37.

Potentially relevant: I plan to remove the weird conversion of install profiles into modules in:
#1676196: Install profiles are registered as modules

Ideally, we'd implant the new handling for recommends[], supports[], etc directly into the module/extension system, so it would generally work for everything — however, I suspect that this would be a giant patch, also involving rather large UI changes, so I'd recommend to limit the scope to installation profiles for now. That is, because it is really important to get this issue resolved for install profiles (but not really urgent for modules/themes). Making the code generic can happen in a follow-up issue, IMHO.

kbasarab’s picture

Updated issue summary from comments in #37 and #39.

kbasarab’s picture

Issue summary: View changes

Issue Summary recreation from windsprints: http://initiatives.drupalofficehours.org/task/800

bleen’s picture

Issue summary: View changes

Updated issue summary.

bleen’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
7.77 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

This patch attempts to get the ball rolling on this once again. So far it seems to be working when doing a module install but I havent played too much with doing installation profile installs yes.

Of note:

  • There is still some debug code in here (like adding views as recommended to aggregator)
  • This needs tests

I dont want to go too far down the rabbit hole though without getting some feedback. Setting to needs review to see how badly testbot chokes

Status: Needs review » Needs work

The last submitted patch, 820054-recommends.patch, failed testing.

tedbow’s picture

Ok it seems like it fails the test bot b/c finds it breaks the install.

I was debugging it the problem is the installer tries to install the comment module before the node module.

Fatal error: Call to undefined function node_type_get_names() in /Applications/MAMP/htdocs/drupal8_git/core/modules/comment/comment.module on line 104

  foreach ($modules as $module) {
    if (!empty($files[$module]->info['required'])) {
      $required[$module] = $files[$module]->sort;
    }
    else {
      $non_required[$module] = $files[$module]->sort;
    }
  }
  arsort($required);
  arsort($non_required);

  $operations = array();

For $non_required modules the order before the sort is:

"$non_required"	Array [23]	
	node	-12	
	block	-2	
	color	-3	
	comment	-4	
	contextual	-5	
	help	-9	
	image	-10	
	menu	-11	
	number	-13	
	options	-14	
	path	-16	
	taxonomy	-20	
	dblog	-6	
	search	-18	
	shortcut	-19	
	toolbar	-21	
	overlay	-15	
	field_ui	-7	
	file	-8	
	rdf	-17	
	views	0	
	views_ui	-22	
	standard	-23	

Then after the sort:

"$non_required"	Array [23]	
	views	0	
	block	-2	
	color	-3	
	comment	-4	
	contextual	-5	
	dblog	-6	
	field_ui	-7	
	file	-8	
	help	-9	
	image	-10	
	menu	-11	
	node	-12	
	number	-13	
	options	-14	
	overlay	-15	
	path	-16	
	rdf	-17	
	search	-18	
	shortcut	-19	
	taxonomy	-20	
	toolbar	-21	
	views_ui	-22	
	standard	-23	

So I haven't looked how the sort works but that seems to be the problem.

tedbow’s picture

Status: Needs work » Needs review
FileSize
7.99 KB
FAILED: [[SimpleTest]]: [MySQL] 46,240 pass(es), 25 fail(s), and 5 exception(s). View

Ok, I have commented out 2 lines and this makes the install work for the standard profile.

/core/includes/install.core.inc
line #1542

  //arsort($required);
  //arsort($non_required);

So I know why these lines SHOULD be there. The $files array elements have the property "sort" and it seems we should sort the array by it. But the weights seems to be wrong when we have this patch(don't know why).

So why does it work with these lines commented our?? 2 possibilites

  1. Luck... array just happens to be an order that works
  2. The module is already be sorted somewhere else

Status: Needs review » Needs work

The last submitted patch, 820054-52-recommends.patch, failed testing.

YesCT’s picture

Issue tags: +Needs reroll

patch no longer applies. needs reroll (helpful doc: http://drupal.org/patch/reroll)

lbainbridge’s picture

Assigned: Unassigned » lbainbridge

I am working on a reroll, will post later today.

lbainbridge’s picture

Assigned: lbainbridge » Unassigned
Status: Needs work » Needs review
FileSize
7.57 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Ok, the function _module_build_dependencies($files) was moved to core/lib/Drupal/Core/Extension/ModuleHandler.php and renamed buildModuleDependencies(array $modules). I moved the functionality over and altered the .info files to .info.yml

Status: Needs review » Needs work

The last submitted patch, 820054-56-recommends.patch, failed testing.

disasm’s picture

Thanks for the reroll lbainbridge! This was very close to being correct on the first try. I think I debugged the problem to what you see below.

Please provide an interdiff when you make the change. Reviewers will thank you for it!

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined
@@ -170,22 +170,46 @@ public function getBootstrapModules() {
-          $dependency_data = $this->parseDependency($dependency);
-          $graph[$module->name]['edges'][$dependency_data['name']] = $dependency_data;
+          $dependency_data = drupal_parse_dependency($dependency);

this needs to stay $this->parseDependency($dependency);
My hypothesis is this is what is calling the installation failure.

lbainbridge’s picture

Status: Needs work » Needs review
FileSize
7.51 KB
FAILED: [[SimpleTest]]: [MySQL] 51,221 pass(es), 29 fail(s), and 1 exception(s). View

Thanks disasm, looking it over that would also be a problem with the added code for $recommendations_data, here is another reroll that should hopefully pass installation.

Here's the changes between my old patch and my new one:

diff --git a/core/lib/Drupal/Core/Extension/ModuleHandler.php b/core/lib/Drupal/Core/Extension/ModuleHandler.php
index 85676ba..9a64c54 100644
--- a/core/lib/Drupal/Core/Extension/ModuleHandler.php
+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -180,7 +180,7 @@ public function buildModuleDependencies(array $modules) {
       // Add information about dependencies to a graph.
       if (isset($module->info['dependencies']) && is_array($module->info['dependencies'])) {
         foreach ($module->info['dependencies'] as $dependency) {
-          $dependency_data = drupal_parse_dependency($dependency);
+          $dependency_data = $this->parseDependency($dependency);
           $dependencies_graph[$module->name]['edges'][$dependency_data['name']] = $dependency_data;
         }
       }
@@ -188,7 +188,7 @@ public function buildModuleDependencies(array $modules) {
       // Add information about recommendations to a graph.
       if (isset($module->info['recommends']) && is_array($module->info['recommends'])) {
         foreach ($module->info['recommends'] as $recommendation) {
-          $recommendation_data = drupal_parse_dependency($recommendation);
+          $recommendation_data = $this->parseDependency($recommendation);
           $recommendations_graph[$module->name]['edges'][$recommendation_data['name']] = $recommendation_data;
         }
       }

Status: Needs review » Needs work

The last submitted patch, 820054-59-recommends.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2013

@lbainbridge - Thanks for the reroll!

We worked on this during Sprint Weekend and I didn't mention interdiffs to everyone. Instructions on providing an interdiff.txt are here: http://drupal.org/documentation/git/interdiff

It looks like you've provided the interdiff inline with your comment, which is probably fine in this case. But now you know for next time :)

Edit: And it installs! Thanks @disasm.

disasm’s picture

The last submitted patch, 820054-59-recommends.patch, failed testing.

mtift’s picture

I've been looking into why the most recent patch had 29 fails. For example, I ran CommentTranslationUITest and the first problem is with a "Call to undefined function language_negotiation_include()" in core/modules/translation_entity/translation_entity.install. The function language_negotiation_include() does exist in core/modules/language/language.module. A few other tests produced the same error, such as CustomBlockTranslationUITest.php and NodeTranslationUITest.php.

So I'm thinking this "recommends" patch won't apply until #1506868: Rewrite language_negotiation_include to support form_state and use it in language.admin.inc is fixed. I'll keep looking into some of the other test results.

disasm’s picture

+++ b/core/includes/install.core.incundefined
@@ -1669,8 +1669,8 @@ function install_profile_modules(&$install_state) {
-  arsort($required);
-  arsort($non_required);
+  //arsort($required);
+  //arsort($non_required);
 

lets just remove this. leaving commented out dead code (especially without a comment explaining why it's being commented out is bad.

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined
@@ -170,22 +170,46 @@ public function getBootstrapModules() {
+          $recommendation_data = $this->parseDependency($recommendation);

I haven't dug into this very deeply, but my thought is the errors are probably coming from the parseDependency method. The rest of the code looks like it's just variable rewrite and comment changes.

YesCT’s picture

Issue tags: -Needs reroll

just removing the reroll tag since the most recent patch applies ok.

xjm’s picture

@David_Rothstein suggests in #987242: The "Promoted to front page" checkbox doesn't do anything if the /node front page listing isn't used that we should add a way for modules to recommend (but not require) Views. Would the recommends[] functionality here jive with that?

klonos’s picture

Please bear with me as I explain this whole idea...

Now, I'm not suggesting we remove the dependencies[] functionality, but to me it makes much more sense to use requires[] and optional[] for hard and soft dependencies respectively. It feels more "natural" and I believe it would also cover #328932: Modules and partial dependencies - enhances[] and enhancedby[] field in modules' .info.yml files.

So, how about we implemented and slowly started evangelizing the support of requires[] and optional[] but still support dependencies[] as an alternative/legacy form of requires[] (and let it slowly die - till D9)?

Now I know that the word "required" is already used in .info files to signify modules that need to be enabled by default at all times (required = TRUE) and that could lead to potential confusion, but...

...how about we (re)move the required = TRUE from the module.info files (only core modules can actually use it AFAIK) and keep all required modules in a single place? I propose this place for core to be a core.info file of a respective core profile that stays hidden (hidden = TRUE) from the installation UI. All other core and non-core profiles would depend on this core profile (requires[] = core in their respective .info files) and then specify requires[] only for modules additional to these included in the core profile. So, what I'm actually proposing is introducing this concept of sub-profiles or profile dependencies/requirements if you like.

To sum it up:

- Introduce requires[] and optional[] to .info files of modules, themes and profiles
- Slowly deprecate dependencies[] in favor of requires[].
- Introduce a special "core" profile that has hidden = TRUE being the first thing enabled during installation.
- Replace core = 9.x with required[] = core (9.x) so that this core profile becomes a requirement for all other profiles, modules and themes.
- Move the required = TRUE from the .info of modules to required[] = ... entries in core.info

The functionality of required = TRUE would still remain 99% the same, because in order to be allowed to disable a required module one would first have to disable core.profile. That of course would not be possible because core.info would specify hidden = TRUE.

I believe that this plan would bring a little more consistency in .info files. Or I'm talking crazy(?)

klonos’s picture

...well in D8 we use .info.yml files, so for example /core/profiles/minimal/minimal.info.yml:

name: Minimal
description: 'Build a custom site without pre-configured functionality. Suitable for advanced users.'
version: VERSION
core: 8.x
dependencies:
  - node
  - block
  - dblog

would become /core/profiles/core/core.info.yml:

name: core
description: 'Build a custom site without pre-configured functionality. Suitable for advanced users.'
version: VERSION
hidden: TRUE
requires:
  - node
  - block
  - dblog

and then /core/profiles/standard/standard.info.yml:

name: Standard
description: 'Install with commonly used features pre-configured.'
version: VERSION
requires:
  - core (9.x)
  - history
  - breakpoint
  - ckeditor
  ...
  - path
  - taxonomy
  - search
  - shortcut
  - toolbar
  ...

Notice how in the standard profile:

1. core: 8.x is moved in the "requires" section as - core (9.x)
2. in the "requires" section the entries for node, block and dblog were removed (because they are installed with core.profile) and only modules additional to those defined by the core profile are there.

PS: does anybody know if there is a specific reason why entries in the dependencies lists are not in alphabetical order?

Xano’s picture

...how about we (re)move the required = TRUE from the module.info files (only core modules can actually use it AFAIK) and keep all required modules in a single place?

Distributions can use it as well.

Also, renaming existing things will make it harder to get this approved by core maintainers. If we simply add recommended[] or optional[], or whatever we want to call it, and display the information on the modules page and optionally install all available modules that are recommended, that won't break backwards compatibility.
If we do rename the keys, let's be consistent and go for required_dependencies and optional_dependencies.

Xano’s picture

Status: Needs work » Needs review
FileSize
7 KB
FAILED: [[SimpleTest]]: [MySQL] 57,120 pass(es), 16 fail(s), and 8 exception(s). View

Re-roll, plus I undid the commenting as mentioned in #65.

Status: Needs review » Needs work

The last submitted patch, drupal_820054_71.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

Priority: Major » Normal

This is less messy than it used to be now that disabled modules are gone, although we still have a fair bit of special casing of install profiles. Downgrading to 'normal' though.

Xano’s picture

An example of how this will impact core itself: content entities have list controllers, but they only show very basic lists of entities. The more flexible lists are views, but you will never see those, unless Views is enabled.

Xano’s picture

Issue summary: View changes

Updated issue summary.

penyaskito’s picture

This could also help testing if used in modules and not only profiles.

There is no way that I'm aware of for testing D7 modules integrations with other modules that are not required dependencies, but "enhancements". This would make possible to download those modules which have an integration in testing environments for extending test coverage.

Xano’s picture

There is no way that I'm aware of for testing D7 modules integrations with other modules that are not required dependencies, but "enhancements". This would make possible to download those modules which have an integration in testing environments for extending test coverage.

To do this, the project needs at least one module that depends on the optional module that should be checked out by the testbot. The easiest solution at this moment, if Foo depends on Bar, is adding a foo_test module that depends on bar and is hidden.

andypost’s picture

Related issues: +#328932: Modules and partial dependencies - enhances[] and enhancedby[] field in modules' .info.yml files

Could be useful for #1548204: Remove user signature and move it to contrib to allow forum module recommend a user signature module

mitokens’s picture

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +minor version target

Didn't happen.

There's potentially still some scope to improve things in minor releases, so bumping.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.