Simple dependency system

Forgive me if I'm mistaken, but shouldn't forum_depends return an array?

I think allowing for both required dependencies as well as optional ones would make it practical for a package management tool to be created. I like it, though, and I think being able to programatically enforce (or at least notify users about) dependencies would assist users who refuse to RTFM.

From my drupal-devel comment, here's how I would suggest formatting the hook_depends:

function filestore2_depends() {
 return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments'));
}

Ok, how about it taking a parameter?

function filestore2_depends($dep_type='required') {
$deps = array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments'));
return $deps[$dep_type];
}

Then, for required dependencies, pass a 'required' argument, and optional ones could take an 'optional' argument.

+1

Not only is this something that I have wanted for filestore2 for quite a while, but has not been on my priority list. But this would be extremely useful for the ecommerce modules, as there seems to be alot of interdependancy.

Also on a side note, it would also be good for a module to be able to reject an enabling. So if the module is going to stop drupal from running if it does have its tables created, it will inform the user and not be turned on. This could actually be done allowing the dependancy hook to do some checking and so stop the module from being able to be enabled.

This would make installing the ecommerce package easier given that modules are no longer listed alphabetically from within their subdirectories. The modules are scattered all over the page unless I adopt some sort of ec_ prefix to module naming.

I wonder how Adrian's installer handles dependencies?

My installer doesn't currently handle dependencies, and I am not convinced this should go into 4.6.

There's more to dependency checking than just 'depends' including fun things like circular dependencies and version checking.
Once we have versioning for drupal modules / schemas .. we can look at this again.

Vlado apparently has dependency checking code already built, that he is going to donate to the installer effort.

The status message needs work (incorrect use of em, confusing wording). Lastly, the way messages are handled and fiddled with is peculiar at best: the logic is spread out over different functions and complicated because of that. I'm not happy with the code as is.

Setting this 'active'. Waiting for new patch.

chx: I do like the approach you took, but I think we should return more information to the administrator.
You did:

  drupal_set_message(t('%module is enabled because other modules depend on it.', array ('%module' =>'<em>'. $file->name .'</em>')), 'error');

First I don't think you need to have the $type = 'error' as the second argument.. Imo it is not an error, just a notification. Or am I wrong?

And secondly, can't we do something like:

  drupal_set_message(t("Because '%module' depends on functionality offered by '%modules', these modules are also enabled to omit possible problems.', array ('%module' => "<em>$file->name</em>", '%modules' => "<em>$required_modules</em>")));

I would really appreciate feedback of any of you...

+1, and a slice of blueberry pie.

I tested this patch against the ecommerce package and it worked as expected. My attached patch simply tweaks a couple minor code details and made the the dependency message a little easier to understand.

Please add this to 4.6. This could be viewed as a major usability benefit for module developers and end users ;-)

Can't we refrase this:

t("The following module has been enabled since it's dependent on %module in order function properly: %dependents.", array ('%module' => '<em>'. $module .'</em>', '%dependents' => '<em>'. implode(', ', $enabled) .'</em>')

I'm not sure why, but I don't think this is userfriendly enough..
Anisa, (I'm not sur eshe is on this list) do you have a better idea?

Two things ..
a) Enabling a module is not installing a module, so the naming of the _install hook (wether it is already being used in my install api or not) is wrong.
b) don't add a hook when we already have one.

hook_info used to be used for the module description, but currently is only being used for auth. I am rolling a patch which uses hook_info() (as I am intending to do in my install patches).

not just permissions - some database table(s) may need manual creation.

Dries, what is wrong with this patch?
I think it is a great (and not even a very big code change) improvement to the current drupal.. It's pretty straight-forward, and makes sense in a lot of ways..

If the patch needs more work, just let it know and i'll see what I can do..

Looking at the new patch, I was surprised to see how small it is... There's much more to dependency system than that.

I don't like the approach at all. You automatically enable the required modules and only inform the user afterwards. As I'm used in Linux packaging tools, I expect to be asked if I really want to enable the required module and the module itself. Those modules might not be installed, in this case have to tell the user to download the required modules and properly install them - create tables, enable, configure, and then try enabling this module again. Your patch does not deal with that and might make the system unusable, when a broken module is automatically enabled.

A real package/dependency system should have more different relationships - depends on, recommends, suggests, conflicts... After you select which modules to enable, you get a listing of additional dependant modules with checkboxes defaulting to most likely action, and possibly disabled. Then you confirm that and selected modules are enabled.

This isn't applying anymore..

Dries, what are your thoughts about the dependency system? I think it's quite good, for the sake of usability, it surely will be!

Please share your thoughts so we can get this slippin' in...

I'll look into this later.

(The forum module depends on the taxonomy module too. Easy enough to fix.)

Setting status back to "code needs work", because:
- archive.module depends on node.module;
- blogapi.module depends on node.module and blog.module/forum.module/page.module/story.module/book.module;
- blog.module depends on node.module;
- book.module depends on node.module;
- forum.module depends on comment.module and taxonomy.module;
- page.module depends on node.module;
- path.module depends on node.module and blog.module/forum.module/page.module/poll.module/story.module;
- poll.module depends on node.module;
- story.module depends on node.module;
- tracker.module depends on node.module and comment.module (?);
- upload.module depends on node.module and blog.module/forum.module (?)/page.module/poll.module/story.module;
- comment.module depends on node.module (?);

Or, am I miunderstanding the point here? Looking at your code, I guess not.. But, maybe it's me...
Please feel free to ignore my comment if useless...

- Do you really need the $new_module stuff? Couldn't you just recurse at the point were you enable the module and avoid the extra logic?
- Why arent you using format_plural() for the message?

+1 works great. Applies to HEAD cleanly but system.module is out of date in the patch (off by one line but still clean). Tried out a few different combos, all modules that should be enabled were. Works on PHP 5.0.5 with no call-by-reference issues either ;-)

"module has been enabled for you because %module needs it to work properly"

So, modules are enabled authomatically? Come on, what's this?, Windows' you-stupid-user-we-know-better-what-you-need philosopy?

And what if I have a replacement module with a different name?

I think a big warning about dependencies is ok. But enforcing a disabled module is way too much...

Just like Jose, I'm not convinced automatically enabling modules is a good thing.

IMO automatically enabled modules are ok iff there is message "Following modules were also enabled because of dependiences: foo, bar".

Sorry then, didn't check carefully enough.

I don't see any harm in automatically enabling modules. If user A is trying to get module M to work and it depends on module M2, why shouldn't M2 be enabled to get it work properly? As long as a message is clearly stated that is the case (which indeed it is) the module will work right away.

Otherwise, user A goes to test out M2 to find out it doesn't work. Guess what? They have to go all the way back to the modules page just to enable module M. That should have happened right away.

Great usability boost from this patch. Tested and works as tested. Actually pretty slick, why *can't* Drupal be a bit smarter than it's users? ;-)

I'm going to put this on hold for 4.8.

@Dries, what is the reason for postponing this to 4.8? I do agree with chx, this is working and a pretty cool patch, a nice usability boost indeed. I would say commit it now as well, unless of course there are other things I'm not aware of :-)

Work is being done on a better system. Plus, the forum-comment problems have been taken care of. It is not a critical feature, IMO.

Lets go with code that is ready to commit instead of waiting for new code to come along. Comitting this now doesn't prevent a better system from being added later.

The only thing I see is that hook_info() could live in ...install.

There is already a drupal_info() in drupal.module? Why don't we call this hook _depends or _dependencies and have it return a simple array?

+1

it will make dependencies easier. I can generate a draft hook_depends, subject to review for all contrib and core modules, it is no problem at all.

Just give me a shout if required.

Thanks chx for this simple and much needed addition, and thanks for your persistance. I'd love to see this functionality in for 4.7. The lack of a dependency system does present significant usability issues. I've personally answered more than one help request that arise from people not understanding that they need to enable a required module.

I've read over carefully but not installed and tested the patch (the forum part of the patch no longer applies). The code is clean and straightforward.

Questions/comments:

1. What happens when someone tries to turn off a module that is required by other modules? It looks like the module would be immediately reenabled--in other words, it would look to the user like her/his effort to deactivate the module failed. Do we need to detect if a module is being deactivated, and if so also deactivate modules that require it? Should we simply issue a message, e.g., "Before you can deactivate %module, you must deactivate the following modules that depend on %module: %dependencies."

2. Ideally, we would (a) minimize code in modules that is run only when the module is enabled, and (b) not require module authors to make changes now that we are pretty sure they'll have to delete/revise in 4.8/5.0. Do we have an idea now of what install files in 4.8 will look like? If, for instance, they'll be a .install file, I'm wondering if we could pull the dependency instructions into such a file, maybe even trying to anticipate the probable format to be used. That way, module users wouldn't have to modify their .module files (now and in the future).

Looks this was read and reviewed many times. What is preventing this from getting commited?

- Update for HEAD when we want to push this.
- Decide on a metadata format for modules and use it for dependency data.
- See what, if anything, we can do for UI review and testing before or after this patch. (Not a requirement now, since it can be improved incrementally later.)

I don't think dependencies should silently enable modules without notification.

Sorry, misread the patch.

But it looks almost certain that we will go with metadata files in the end. If we do, dependencies need to go in the metadata files. I wouldn't want to add a new hook which is going to be removed and put into the metadata.

I think we should brainstorm about this whole meta-data story first. What goes into the meta-data files, and what doesn't? It's still blurry in front of my eyes.

Seems to be a duplicate of http://drupal.org/node/84875 which has been committed.