Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: clydefrog Status: patch Forgive me if I'm mistaken, but shouldn't forum_depends return an array? clydefrog Previous comments: ------------------------------------------------------------------------ March 5, 2005 - 16:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels.

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." chx Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 01:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 02:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 02:26 : javanaut 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:

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

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. chx Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 01:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 02:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 02:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 02:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 02:33 : javanaut Ok, how about it taking a parameter? <?php 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.

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) chx Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 01:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 02:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 02:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 02:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 02:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 6, 2005 - 03:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 6, 2005 - 03:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work...

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: mathias Status: patch 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? mathias Previous comments: ------------------------------------------------------------------------ March 5, 2005 - 18:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 5, 2005 - 19:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 5, 2005 - 19:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 5, 2005 - 19:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 5, 2005 - 19:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 5, 2005 - 20:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 5, 2005 - 20:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ March 5, 2005 - 20:12 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ March 5, 2005 - 22:13 : gordon +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.

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. chx Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 01:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 02:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 02:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 02:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 02:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 6, 2005 - 03:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 6, 2005 - 03:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ March 6, 2005 - 03:12 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ March 6, 2005 - 05:13 : gordon +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. ------------------------------------------------------------------------ March 6, 2005 - 09:19 : mathias 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? ------------------------------------------------------------------------ March 6, 2005 - 11:42 : adrian 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.

Issue status update for http://drupal.org/node/18447 Project: Drupal -Version: cvs +Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: Dries Status: patch 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. Dries Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 02:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 03:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 03:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 03:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 03:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 6, 2005 - 04:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 6, 2005 - 04:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ March 6, 2005 - 04:12 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ March 6, 2005 - 06:13 : gordon +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. ------------------------------------------------------------------------ March 6, 2005 - 10:19 : mathias 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? ------------------------------------------------------------------------ March 6, 2005 - 12:42 : adrian 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. ------------------------------------------------------------------------ March 6, 2005 - 12:57 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ March 6, 2005 - 13:08 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons.

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch One s too much... chx Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 01:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 02:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 02:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 02:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 02:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 6, 2005 - 03:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 6, 2005 - 03:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ March 6, 2005 - 03:12 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ March 6, 2005 - 05:13 : gordon +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. ------------------------------------------------------------------------ March 6, 2005 - 09:19 : mathias 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? ------------------------------------------------------------------------ March 6, 2005 - 11:42 : adrian 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. ------------------------------------------------------------------------ March 6, 2005 - 11:57 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ March 6, 2005 - 12:08 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ March 8, 2005 - 20:56 : Dries 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. ------------------------------------------------------------------------ March 12, 2005 - 11:37 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ March 13, 2005 - 21:21 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one.

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: stefan nagtegaal Status: patch chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php 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: <?php 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... stefan nagtegaal Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 01:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 02:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 02:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 02:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 02:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 6, 2005 - 03:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 6, 2005 - 03:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ March 6, 2005 - 03:12 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ March 6, 2005 - 05:13 : gordon +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. ------------------------------------------------------------------------ March 6, 2005 - 09:19 : mathias 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? ------------------------------------------------------------------------ March 6, 2005 - 11:42 : adrian 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. ------------------------------------------------------------------------ March 6, 2005 - 11:57 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ March 6, 2005 - 12:08 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ March 8, 2005 - 20:56 : Dries 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. ------------------------------------------------------------------------ March 12, 2005 - 11:37 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ March 13, 2005 - 21:21 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ March 13, 2005 - 21:29 : chx One s too much... ------------------------------------------------------------------------ March 13, 2005 - 21:31 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...?

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. chx Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 01:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 02:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 02:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 02:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 02:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 6, 2005 - 03:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 6, 2005 - 03:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ March 6, 2005 - 03:12 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ March 6, 2005 - 05:13 : gordon +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. ------------------------------------------------------------------------ March 6, 2005 - 09:19 : mathias 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? ------------------------------------------------------------------------ March 6, 2005 - 11:42 : adrian 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. ------------------------------------------------------------------------ March 6, 2005 - 11:57 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ March 6, 2005 - 12:08 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ March 8, 2005 - 20:56 : Dries 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. ------------------------------------------------------------------------ March 12, 2005 - 11:37 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ March 13, 2005 - 21:21 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ March 13, 2005 - 21:29 : chx One s too much... ------------------------------------------------------------------------ March 13, 2005 - 21:31 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ March 14, 2005 - 11:28 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php 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: <?php 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... ------------------------------------------------------------------------ March 26, 2005 - 00:17 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible.

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: stefan nagtegaal Status: patch Can't we refrase this: <?php 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? stefan nagtegaal Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 01:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 02:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 02:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 02:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 02:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 6, 2005 - 03:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 6, 2005 - 03:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ March 6, 2005 - 03:12 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ March 6, 2005 - 05:13 : gordon +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. ------------------------------------------------------------------------ March 6, 2005 - 09:19 : mathias 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? ------------------------------------------------------------------------ March 6, 2005 - 11:42 : adrian 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. ------------------------------------------------------------------------ March 6, 2005 - 11:57 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ March 6, 2005 - 12:08 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ March 8, 2005 - 20:56 : Dries 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. ------------------------------------------------------------------------ March 12, 2005 - 11:37 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ March 13, 2005 - 21:21 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ March 13, 2005 - 21:29 : chx One s too much... ------------------------------------------------------------------------ March 13, 2005 - 21:31 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ March 14, 2005 - 11:28 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php 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: <?php 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... ------------------------------------------------------------------------ March 26, 2005 - 00:17 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ March 26, 2005 - 02:46 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ March 26, 2005 - 04:24 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-)

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: adrian Status: patch 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). adrian Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 02:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 03:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 03:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 03:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 03:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 6, 2005 - 04:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 6, 2005 - 04:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ March 6, 2005 - 04:12 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ March 6, 2005 - 06:13 : gordon +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. ------------------------------------------------------------------------ March 6, 2005 - 10:19 : mathias 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? ------------------------------------------------------------------------ March 6, 2005 - 12:42 : adrian 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. ------------------------------------------------------------------------ March 6, 2005 - 12:57 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ March 6, 2005 - 13:08 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ March 8, 2005 - 21:56 : Dries 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. ------------------------------------------------------------------------ March 12, 2005 - 12:37 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ March 13, 2005 - 22:21 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ March 13, 2005 - 22:29 : chx One s too much... ------------------------------------------------------------------------ March 13, 2005 - 22:31 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ March 14, 2005 - 12:28 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php 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: <?php 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... ------------------------------------------------------------------------ March 26, 2005 - 01:17 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ March 26, 2005 - 03:46 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ March 26, 2005 - 05:24 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-) ------------------------------------------------------------------------ March 26, 2005 - 16:55 : stefan nagtegaal Can't we refrase this: <?php 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?

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch Attachment: http://drupal.org/files/issues/dependency_1.patch (2.69 KB) I contacted Anisa who had a better wording idea. Thanks! To quote her: "Keep in mind that to the user, this is a surprise.  They may not have read the documentation properly, and not anything about X.  'for you' suggests that this is a service, not a scary-computer-doing-things-on-its-own behavior. I added the line about permissions and settings to remind the user that just because X module has been enabled for them, not everything will be exactly perfect." and she had had advice about changing "permissions" (and maybe settings) to a link, but that's not for this patch... it is listed in the usability group's todo for every module. chx Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 01:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 02:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 02:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 02:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 02:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 6, 2005 - 03:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 6, 2005 - 03:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ March 6, 2005 - 03:12 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ March 6, 2005 - 05:13 : gordon +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. ------------------------------------------------------------------------ March 6, 2005 - 09:19 : mathias 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? ------------------------------------------------------------------------ March 6, 2005 - 11:42 : adrian 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. ------------------------------------------------------------------------ March 6, 2005 - 11:57 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ March 6, 2005 - 12:08 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ March 8, 2005 - 20:56 : Dries 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. ------------------------------------------------------------------------ March 12, 2005 - 11:37 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ March 13, 2005 - 21:21 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ March 13, 2005 - 21:29 : chx One s too much... ------------------------------------------------------------------------ March 13, 2005 - 21:31 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ March 14, 2005 - 11:28 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php 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: <?php 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... ------------------------------------------------------------------------ March 26, 2005 - 00:17 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ March 26, 2005 - 02:46 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ March 26, 2005 - 04:24 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-) ------------------------------------------------------------------------ March 26, 2005 - 15:55 : stefan nagtegaal Can't we refrase this: <?php 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? ------------------------------------------------------------------------ March 26, 2005 - 21:12 : adrian 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). ------------------------------------------------------------------------ March 26, 2005 - 23:33 : chx Attachment: http://drupal.org/files/issues/dependency_0.patch (2.61 KB) OK I changed back to hook_info. However, hook_info PHPdoc needs a change.

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: stefan nagtegaal Status: patch 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.. stefan nagtegaal Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 01:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 02:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 02:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 02:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 02:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 6, 2005 - 03:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 6, 2005 - 03:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ March 6, 2005 - 03:12 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ March 6, 2005 - 05:13 : gordon +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. ------------------------------------------------------------------------ March 6, 2005 - 09:19 : mathias 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? ------------------------------------------------------------------------ March 6, 2005 - 11:42 : adrian 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. ------------------------------------------------------------------------ March 6, 2005 - 11:57 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ March 6, 2005 - 12:08 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ March 8, 2005 - 20:56 : Dries 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. ------------------------------------------------------------------------ March 12, 2005 - 11:37 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ March 13, 2005 - 21:21 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ March 13, 2005 - 21:29 : chx One s too much... ------------------------------------------------------------------------ March 13, 2005 - 21:31 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ March 14, 2005 - 11:28 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php 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: <?php 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... ------------------------------------------------------------------------ March 26, 2005 - 00:17 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ March 26, 2005 - 02:46 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ March 26, 2005 - 04:24 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-) ------------------------------------------------------------------------ March 26, 2005 - 15:55 : stefan nagtegaal Can't we refrase this: <?php 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? ------------------------------------------------------------------------ March 26, 2005 - 21:12 : adrian 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). ------------------------------------------------------------------------ March 26, 2005 - 23:33 : chx Attachment: http://drupal.org/files/issues/dependency_0.patch (2.61 KB) OK I changed back to hook_info. However, hook_info PHPdoc needs a change. ------------------------------------------------------------------------ March 28, 2005 - 01:01 : chx Attachment: http://drupal.org/files/issues/dependency_1.patch (2.69 KB) I contacted Anisa who had a better wording idea. Thanks! To quote her: "Keep in mind that to the user, this is a surprise.  They may not have read the documentation properly, and not anything about X.  'for you' suggests that this is a service, not a scary-computer-doing-things-on-its-own behavior. I added the line about permissions and settings to remind the user that just because X module has been enabled for them, not everything will be exactly perfect." and she had had advice about changing "permissions" (and maybe settings) to a link, but that's not for this patch... it is listed in the usability group's todo for every module. ------------------------------------------------------------------------ March 28, 2005 - 03:42 : moshe weitzman not just permissions - some database table(s) may need manual creation.

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch Any chance for 4.6? chx Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 01:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 02:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 02:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 02:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 02:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 6, 2005 - 03:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 6, 2005 - 03:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ March 6, 2005 - 03:12 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ March 6, 2005 - 05:13 : gordon +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. ------------------------------------------------------------------------ March 6, 2005 - 09:19 : mathias 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? ------------------------------------------------------------------------ March 6, 2005 - 11:42 : adrian 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. ------------------------------------------------------------------------ March 6, 2005 - 11:57 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ March 6, 2005 - 12:08 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ March 8, 2005 - 20:56 : Dries 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. ------------------------------------------------------------------------ March 12, 2005 - 11:37 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ March 13, 2005 - 21:21 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ March 13, 2005 - 21:29 : chx One s too much... ------------------------------------------------------------------------ March 13, 2005 - 21:31 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ March 14, 2005 - 11:28 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php   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: <?php   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... ------------------------------------------------------------------------ March 26, 2005 - 00:17 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ March 26, 2005 - 02:46 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ March 26, 2005 - 04:24 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-) ------------------------------------------------------------------------ March 26, 2005 - 15:55 : stefan nagtegaal Can't we refrase this: <?php 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? ------------------------------------------------------------------------ March 26, 2005 - 21:12 : adrian 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). ------------------------------------------------------------------------ March 26, 2005 - 23:33 : chx Attachment: http://drupal.org/files/issues/dependency_0.patch (2.61 KB) OK I changed back to hook_info. However, hook_info PHPdoc needs a change. ------------------------------------------------------------------------ March 28, 2005 - 01:01 : chx Attachment: http://drupal.org/files/issues/dependency_1.patch (2.69 KB) I contacted Anisa who had a better wording idea. Thanks! To quote her: "Keep in mind that to the user, this is a surprise.  They may not have read the documentation properly, and not anything about X.  'for you' suggests that this is a service, not a scary-computer-doing-things-on-its-own behavior. I added the line about permissions and settings to remind the user that just because X module has been enabled for them, not everything will be exactly perfect." and she had had advice about changing "permissions" (and maybe settings) to a link, but that's not for this patch... it is listed in the usability group's todo for every module. ------------------------------------------------------------------------ March 28, 2005 - 03:42 : moshe weitzman not just permissions - some database table(s) may need manual creation. ------------------------------------------------------------------------ April 3, 2005 - 20:48 : stefan nagtegaal 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.. ------------------------------------------------------------------------ April 3, 2005 - 22:25 : chx Attachment: http://drupal.org/files/issues/dependency_2.patch (2.9 KB) reword again to include moshe's suggestion. theme placeholder usage. proper cvs diff.

Issue status update for http://drupal.org/node/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: stefan nagtegaal Status: patch 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... stefan nagtegaal Previous comments: ------------------------------------------------------------------------ March 6, 2005 - 02:55 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ March 6, 2005 - 03:22 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ March 6, 2005 - 03:26 : javanaut 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:

<?php function filestore2_depends() { return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ March 6, 2005 - 03:29 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ March 6, 2005 - 03:33 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ March 6, 2005 - 04:00 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ March 6, 2005 - 04:11 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ March 6, 2005 - 04:12 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ March 6, 2005 - 06:13 : gordon +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. ------------------------------------------------------------------------ March 6, 2005 - 10:19 : mathias 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? ------------------------------------------------------------------------ March 6, 2005 - 12:42 : adrian 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. ------------------------------------------------------------------------ March 6, 2005 - 12:57 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ March 6, 2005 - 13:08 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ March 8, 2005 - 21:56 : Dries 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. ------------------------------------------------------------------------ March 12, 2005 - 12:37 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ March 13, 2005 - 22:21 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ March 13, 2005 - 22:29 : chx One s too much... ------------------------------------------------------------------------ March 13, 2005 - 22:31 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ March 14, 2005 - 12:28 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php   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: <?php   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... ------------------------------------------------------------------------ March 26, 2005 - 01:17 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ March 26, 2005 - 03:46 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ March 26, 2005 - 05:24 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-) ------------------------------------------------------------------------ March 26, 2005 - 16:55 : stefan nagtegaal Can't we refrase this: <?php 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? ------------------------------------------------------------------------ March 26, 2005 - 22:12 : adrian 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). ------------------------------------------------------------------------ March 27, 2005 - 00:33 : chx Attachment: http://drupal.org/files/issues/dependency_0.patch (2.61 KB) OK I changed back to hook_info. However, hook_info PHPdoc needs a change. ------------------------------------------------------------------------ March 28, 2005 - 02:01 : chx Attachment: http://drupal.org/files/issues/dependency_1.patch (2.69 KB) I contacted Anisa who had a better wording idea. Thanks! To quote her: "Keep in mind that to the user, this is a surprise.  They may not have read the documentation properly, and not anything about X.  'for you' suggests that this is a service, not a scary-computer-doing-things-on-its-own behavior. I added the line about permissions and settings to remind the user that just because X module has been enabled for them, not everything will be exactly perfect." and she had had advice about changing "permissions" (and maybe settings) to a link, but that's not for this patch... it is listed in the usability group's todo for every module. ------------------------------------------------------------------------ March 28, 2005 - 04:42 : moshe weitzman not just permissions - some database table(s) may need manual creation. ------------------------------------------------------------------------ April 3, 2005 - 21:48 : stefan nagtegaal 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.. ------------------------------------------------------------------------ April 3, 2005 - 23:25 : chx Attachment: http://drupal.org/files/issues/dependency_2.patch (2.9 KB) reword again to include moshe's suggestion. theme placeholder usage. proper cvs diff. ------------------------------------------------------------------------ April 12, 2005 - 01:27 : chx Any chance for 4.6? ------------------------------------------------------------------------ April 12, 2005 - 13:34 : menesis 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.

Issue status update for http://drupal.org/node/18447 Post a follow up: http://drupal.org/project/comments/add/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx -Status: patch (code needs work) +Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/dependency_3.patch (2.87 KB) Rerolled. chx Previous comments: ------------------------------------------------------------------------ Sun, 06 Mar 2005 00:55:36 +0000 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:22:17 +0000 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:26:40 +0000 : javanaut 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:

<?php function filestore2_depends() {  return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:29:24 +0000 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:33:55 +0000 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:00:27 +0000 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:11:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:12:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ Sun, 06 Mar 2005 04:13:14 +0000 : gordon +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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 08:19:09 +0000 : mathias 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? ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:42:01 +0000 : adrian 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:57:54 +0000 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ Sun, 06 Mar 2005 11:08:37 +0000 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ Tue, 08 Mar 2005 19:56:02 +0000 : Dries 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. ------------------------------------------------------------------------ Sat, 12 Mar 2005 10:37:22 +0000 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:21:21 +0000 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:29:31 +0000 : chx One s too much... ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:31:17 +0000 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ Mon, 14 Mar 2005 10:28:54 +0000 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php   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: <?php   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... ------------------------------------------------------------------------ Fri, 25 Mar 2005 23:17:35 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ Sat, 26 Mar 2005 01:46:00 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ Sat, 26 Mar 2005 03:24:59 +0000 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-) ------------------------------------------------------------------------ Sat, 26 Mar 2005 14:55:08 +0000 : stefan nagtegaal Can't we refrase this: <?php 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? ------------------------------------------------------------------------ Sat, 26 Mar 2005 20:12:52 +0000 : adrian 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). ------------------------------------------------------------------------ Sat, 26 Mar 2005 22:33:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_0.patch (2.61 KB) OK I changed back to hook_info. However, hook_info PHPdoc needs a change. ------------------------------------------------------------------------ Mon, 28 Mar 2005 00:01:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_1.patch (2.69 KB) I contacted Anisa who had a better wording idea. Thanks! To quote her: "Keep in mind that to the user, this is a surprise.  They may not have read the documentation properly, and not anything about X.  'for you' suggests that this is a service, not a scary-computer-doing-things-on-its-own behavior. I added the line about permissions and settings to remind the user that just because X module has been enabled for them, not everything will be exactly perfect." and she had had advice about changing "permissions" (and maybe settings) to a link, but that's not for this patch... it is listed in the usability group's todo for every module. ------------------------------------------------------------------------ Mon, 28 Mar 2005 02:42:35 +0000 : moshe weitzman not just permissions - some database table(s) may need manual creation. ------------------------------------------------------------------------ Sun, 03 Apr 2005 19:48:04 +0000 : stefan nagtegaal 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.. ------------------------------------------------------------------------ Sun, 03 Apr 2005 21:25:38 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_2.patch (2.9 KB) reword again to include moshe's suggestion. theme placeholder usage. proper cvs diff. ------------------------------------------------------------------------ Mon, 11 Apr 2005 23:27:53 +0000 : chx Any chance for 4.6? ------------------------------------------------------------------------ Tue, 12 Apr 2005 11:34:46 +0000 : menesis 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. ------------------------------------------------------------------------ Fri, 01 Jul 2005 18:08:12 +0000 : stefan nagtegaal 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... ------------------------------------------------------------------------ Fri, 05 Aug 2005 18:12:09 +0000 : chx I will reroll this when I get home. Either this or Adrian's install system needs to get into 4.7. Maybe this system is not ideal and does not deal with broken modules and such but you can always enable such things by hand. So, this won't make anything worse. (And I want my pie from matthias...)

Issue status update for http://drupal.org/node/18447 Post a follow up: http://drupal.org/project/comments/add/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: stefan nagtegaal -Status: patch (ready to be committed) +Status: patch (code needs work) 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... stefan nagtegaal Previous comments: ------------------------------------------------------------------------ Sun, 06 Mar 2005 00:55:36 +0000 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:22:17 +0000 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:26:40 +0000 : javanaut 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:

<?php function filestore2_depends() {  return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:29:24 +0000 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:33:55 +0000 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:00:27 +0000 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:11:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:12:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ Sun, 06 Mar 2005 04:13:14 +0000 : gordon +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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 08:19:09 +0000 : mathias 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? ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:42:01 +0000 : adrian 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:57:54 +0000 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ Sun, 06 Mar 2005 11:08:37 +0000 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ Tue, 08 Mar 2005 19:56:02 +0000 : Dries 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. ------------------------------------------------------------------------ Sat, 12 Mar 2005 10:37:22 +0000 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:21:21 +0000 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:29:31 +0000 : chx One s too much... ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:31:17 +0000 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ Mon, 14 Mar 2005 10:28:54 +0000 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php   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: <?php   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... ------------------------------------------------------------------------ Fri, 25 Mar 2005 23:17:35 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ Sat, 26 Mar 2005 01:46:00 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ Sat, 26 Mar 2005 03:24:59 +0000 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-) ------------------------------------------------------------------------ Sat, 26 Mar 2005 14:55:08 +0000 : stefan nagtegaal Can't we refrase this: <?php 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? ------------------------------------------------------------------------ Sat, 26 Mar 2005 20:12:52 +0000 : adrian 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). ------------------------------------------------------------------------ Sat, 26 Mar 2005 22:33:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_0.patch (2.61 KB) OK I changed back to hook_info. However, hook_info PHPdoc needs a change. ------------------------------------------------------------------------ Mon, 28 Mar 2005 00:01:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_1.patch (2.69 KB) I contacted Anisa who had a better wording idea. Thanks! To quote her: "Keep in mind that to the user, this is a surprise.  They may not have read the documentation properly, and not anything about X.  'for you' suggests that this is a service, not a scary-computer-doing-things-on-its-own behavior. I added the line about permissions and settings to remind the user that just because X module has been enabled for them, not everything will be exactly perfect." and she had had advice about changing "permissions" (and maybe settings) to a link, but that's not for this patch... it is listed in the usability group's todo for every module. ------------------------------------------------------------------------ Mon, 28 Mar 2005 02:42:35 +0000 : moshe weitzman not just permissions - some database table(s) may need manual creation. ------------------------------------------------------------------------ Sun, 03 Apr 2005 19:48:04 +0000 : stefan nagtegaal 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.. ------------------------------------------------------------------------ Sun, 03 Apr 2005 21:25:38 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_2.patch (2.9 KB) reword again to include moshe's suggestion. theme placeholder usage. proper cvs diff. ------------------------------------------------------------------------ Mon, 11 Apr 2005 23:27:53 +0000 : chx Any chance for 4.6? ------------------------------------------------------------------------ Tue, 12 Apr 2005 11:34:46 +0000 : menesis 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. ------------------------------------------------------------------------ Fri, 01 Jul 2005 18:08:12 +0000 : stefan nagtegaal 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... ------------------------------------------------------------------------ Fri, 05 Aug 2005 18:12:09 +0000 : chx I will reroll this when I get home. Either this or Adrian's install system needs to get into 4.7. Maybe this system is not ideal and does not deal with broken modules and such but you can always enable such things by hand. So, this won't make anything worse. (And I want my pie from matthias...) ------------------------------------------------------------------------ Wed, 07 Sep 2005 04:24:48 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_3.patch (2.87 KB) Rerolled. ------------------------------------------------------------------------ Wed, 07 Sep 2005 07:51:08 +0000 : Dries I'll look into this later. (The forum module depends on the taxonomy module too. Easy enough to fix.)

Issue status update for http://drupal.org/node/18447 Post a follow up: http://drupal.org/project/comments/add/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/dependency_4.patch (2.88 KB) Added taxonomy to forum_info. chx Previous comments: ------------------------------------------------------------------------ Sun, 06 Mar 2005 00:55:36 +0000 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:22:17 +0000 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:26:40 +0000 : javanaut 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:

<?php function filestore2_depends() {  return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:29:24 +0000 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:33:55 +0000 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:00:27 +0000 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:11:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:12:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ Sun, 06 Mar 2005 04:13:14 +0000 : gordon +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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 08:19:09 +0000 : mathias 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? ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:42:01 +0000 : adrian 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:57:54 +0000 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ Sun, 06 Mar 2005 11:08:37 +0000 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ Tue, 08 Mar 2005 19:56:02 +0000 : Dries 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. ------------------------------------------------------------------------ Sat, 12 Mar 2005 10:37:22 +0000 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:21:21 +0000 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:29:31 +0000 : chx One s too much... ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:31:17 +0000 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ Mon, 14 Mar 2005 10:28:54 +0000 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php   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: <?php   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... ------------------------------------------------------------------------ Fri, 25 Mar 2005 23:17:35 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ Sat, 26 Mar 2005 01:46:00 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ Sat, 26 Mar 2005 03:24:59 +0000 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-) ------------------------------------------------------------------------ Sat, 26 Mar 2005 14:55:08 +0000 : stefan nagtegaal Can't we refrase this: <?php 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? ------------------------------------------------------------------------ Sat, 26 Mar 2005 20:12:52 +0000 : adrian 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). ------------------------------------------------------------------------ Sat, 26 Mar 2005 22:33:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_0.patch (2.61 KB) OK I changed back to hook_info. However, hook_info PHPdoc needs a change. ------------------------------------------------------------------------ Mon, 28 Mar 2005 00:01:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_1.patch (2.69 KB) I contacted Anisa who had a better wording idea. Thanks! To quote her: "Keep in mind that to the user, this is a surprise.  They may not have read the documentation properly, and not anything about X.  'for you' suggests that this is a service, not a scary-computer-doing-things-on-its-own behavior. I added the line about permissions and settings to remind the user that just because X module has been enabled for them, not everything will be exactly perfect." and she had had advice about changing "permissions" (and maybe settings) to a link, but that's not for this patch... it is listed in the usability group's todo for every module. ------------------------------------------------------------------------ Mon, 28 Mar 2005 02:42:35 +0000 : moshe weitzman not just permissions - some database table(s) may need manual creation. ------------------------------------------------------------------------ Sun, 03 Apr 2005 19:48:04 +0000 : stefan nagtegaal 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.. ------------------------------------------------------------------------ Sun, 03 Apr 2005 21:25:38 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_2.patch (2.9 KB) reword again to include moshe's suggestion. theme placeholder usage. proper cvs diff. ------------------------------------------------------------------------ Mon, 11 Apr 2005 23:27:53 +0000 : chx Any chance for 4.6? ------------------------------------------------------------------------ Tue, 12 Apr 2005 11:34:46 +0000 : menesis 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. ------------------------------------------------------------------------ Fri, 01 Jul 2005 18:08:12 +0000 : stefan nagtegaal 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... ------------------------------------------------------------------------ Fri, 05 Aug 2005 18:12:09 +0000 : chx I will reroll this when I get home. Either this or Adrian's install system needs to get into 4.7. Maybe this system is not ideal and does not deal with broken modules and such but you can always enable such things by hand. So, this won't make anything worse. (And I want my pie from matthias...) ------------------------------------------------------------------------ Wed, 07 Sep 2005 04:24:48 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_3.patch (2.87 KB) Rerolled. ------------------------------------------------------------------------ Wed, 07 Sep 2005 07:51:08 +0000 : Dries I'll look into this later. (The forum module depends on the taxonomy module too. Easy enough to fix.) ------------------------------------------------------------------------ Wed, 07 Sep 2005 08:05:51 +0000 : stefan nagtegaal 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... ------------------------------------------------------------------------ Wed, 07 Sep 2005 08:37:39 +0000 : chx The dependency system itself works nicely. That we can add more stuff to use it -- true. That may come later. First, get the stuff in.

Issue status update for http://drupal.org/node/18447 Post a follow up: http://drupal.org/project/comments/add/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch (ready to be committed) format_plural accepts no parameters, while I have a few. The new_module logic is good because you do not recurse for every single module only after a bunch of them. Performance is better. chx Previous comments: ------------------------------------------------------------------------ Sun, 06 Mar 2005 00:55:36 +0000 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:22:17 +0000 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:26:40 +0000 : javanaut 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:

<?php function filestore2_depends() {  return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:29:24 +0000 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:33:55 +0000 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:00:27 +0000 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:11:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:12:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ Sun, 06 Mar 2005 04:13:14 +0000 : gordon +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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 08:19:09 +0000 : mathias 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? ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:42:01 +0000 : adrian 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:57:54 +0000 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ Sun, 06 Mar 2005 11:08:37 +0000 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ Tue, 08 Mar 2005 19:56:02 +0000 : Dries 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. ------------------------------------------------------------------------ Sat, 12 Mar 2005 10:37:22 +0000 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:21:21 +0000 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:29:31 +0000 : chx One s too much... ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:31:17 +0000 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ Mon, 14 Mar 2005 10:28:54 +0000 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php   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: <?php   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... ------------------------------------------------------------------------ Fri, 25 Mar 2005 23:17:35 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ Sat, 26 Mar 2005 01:46:00 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ Sat, 26 Mar 2005 03:24:59 +0000 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-) ------------------------------------------------------------------------ Sat, 26 Mar 2005 14:55:08 +0000 : stefan nagtegaal Can't we refrase this: <?php 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? ------------------------------------------------------------------------ Sat, 26 Mar 2005 20:12:52 +0000 : adrian 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). ------------------------------------------------------------------------ Sat, 26 Mar 2005 22:33:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_0.patch (2.61 KB) OK I changed back to hook_info. However, hook_info PHPdoc needs a change. ------------------------------------------------------------------------ Mon, 28 Mar 2005 00:01:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_1.patch (2.69 KB) I contacted Anisa who had a better wording idea. Thanks! To quote her: "Keep in mind that to the user, this is a surprise.  They may not have read the documentation properly, and not anything about X.  'for you' suggests that this is a service, not a scary-computer-doing-things-on-its-own behavior. I added the line about permissions and settings to remind the user that just because X module has been enabled for them, not everything will be exactly perfect." and she had had advice about changing "permissions" (and maybe settings) to a link, but that's not for this patch... it is listed in the usability group's todo for every module. ------------------------------------------------------------------------ Mon, 28 Mar 2005 02:42:35 +0000 : moshe weitzman not just permissions - some database table(s) may need manual creation. ------------------------------------------------------------------------ Sun, 03 Apr 2005 19:48:04 +0000 : stefan nagtegaal 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.. ------------------------------------------------------------------------ Sun, 03 Apr 2005 21:25:38 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_2.patch (2.9 KB) reword again to include moshe's suggestion. theme placeholder usage. proper cvs diff. ------------------------------------------------------------------------ Mon, 11 Apr 2005 23:27:53 +0000 : chx Any chance for 4.6? ------------------------------------------------------------------------ Tue, 12 Apr 2005 11:34:46 +0000 : menesis 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. ------------------------------------------------------------------------ Fri, 01 Jul 2005 18:08:12 +0000 : stefan nagtegaal 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... ------------------------------------------------------------------------ Fri, 05 Aug 2005 18:12:09 +0000 : chx I will reroll this when I get home. Either this or Adrian's install system needs to get into 4.7. Maybe this system is not ideal and does not deal with broken modules and such but you can always enable such things by hand. So, this won't make anything worse. (And I want my pie from matthias...) ------------------------------------------------------------------------ Wed, 07 Sep 2005 04:24:48 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_3.patch (2.87 KB) Rerolled. ------------------------------------------------------------------------ Wed, 07 Sep 2005 07:51:08 +0000 : Dries I'll look into this later. (The forum module depends on the taxonomy module too. Easy enough to fix.) ------------------------------------------------------------------------ Wed, 07 Sep 2005 08:05:51 +0000 : stefan nagtegaal 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... ------------------------------------------------------------------------ Wed, 07 Sep 2005 08:37:39 +0000 : chx The dependency system itself works nicely. That we can add more stuff to use it -- true. That may come later. First, get the stuff in. ------------------------------------------------------------------------ Wed, 07 Sep 2005 09:00:36 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_4.patch (2.88 KB) Added taxonomy to forum_info. ------------------------------------------------------------------------ Wed, 07 Sep 2005 09:13:57 +0000 : Kjartan - 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?

Issue status update for http://drupal.org/node/18447 Post a follow up: http://drupal.org/project/comments/add/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: m3avrck Status: patch (ready to be committed) +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 ;-) m3avrck Previous comments: ------------------------------------------------------------------------ Sun, 06 Mar 2005 00:55:36 +0000 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:22:17 +0000 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:26:40 +0000 : javanaut 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:

<?php function filestore2_depends() {  return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:29:24 +0000 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:33:55 +0000 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:00:27 +0000 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:11:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:12:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ Sun, 06 Mar 2005 04:13:14 +0000 : gordon +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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 08:19:09 +0000 : mathias 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? ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:42:01 +0000 : adrian 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:57:54 +0000 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ Sun, 06 Mar 2005 11:08:37 +0000 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ Tue, 08 Mar 2005 19:56:02 +0000 : Dries 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. ------------------------------------------------------------------------ Sat, 12 Mar 2005 10:37:22 +0000 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:21:21 +0000 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:29:31 +0000 : chx One s too much... ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:31:17 +0000 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ Mon, 14 Mar 2005 10:28:54 +0000 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php   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: <?php   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... ------------------------------------------------------------------------ Fri, 25 Mar 2005 23:17:35 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ Sat, 26 Mar 2005 01:46:00 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ Sat, 26 Mar 2005 03:24:59 +0000 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-) ------------------------------------------------------------------------ Sat, 26 Mar 2005 14:55:08 +0000 : stefan nagtegaal Can't we refrase this: <?php 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? ------------------------------------------------------------------------ Sat, 26 Mar 2005 20:12:52 +0000 : adrian 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). ------------------------------------------------------------------------ Sat, 26 Mar 2005 22:33:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_0.patch (2.61 KB) OK I changed back to hook_info. However, hook_info PHPdoc needs a change. ------------------------------------------------------------------------ Mon, 28 Mar 2005 00:01:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_1.patch (2.69 KB) I contacted Anisa who had a better wording idea. Thanks! To quote her: "Keep in mind that to the user, this is a surprise.  They may not have read the documentation properly, and not anything about X.  'for you' suggests that this is a service, not a scary-computer-doing-things-on-its-own behavior. I added the line about permissions and settings to remind the user that just because X module has been enabled for them, not everything will be exactly perfect." and she had had advice about changing "permissions" (and maybe settings) to a link, but that's not for this patch... it is listed in the usability group's todo for every module. ------------------------------------------------------------------------ Mon, 28 Mar 2005 02:42:35 +0000 : moshe weitzman not just permissions - some database table(s) may need manual creation. ------------------------------------------------------------------------ Sun, 03 Apr 2005 19:48:04 +0000 : stefan nagtegaal 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.. ------------------------------------------------------------------------ Sun, 03 Apr 2005 21:25:38 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_2.patch (2.9 KB) reword again to include moshe's suggestion. theme placeholder usage. proper cvs diff. ------------------------------------------------------------------------ Mon, 11 Apr 2005 23:27:53 +0000 : chx Any chance for 4.6? ------------------------------------------------------------------------ Tue, 12 Apr 2005 11:34:46 +0000 : menesis 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. ------------------------------------------------------------------------ Fri, 01 Jul 2005 18:08:12 +0000 : stefan nagtegaal 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... ------------------------------------------------------------------------ Fri, 05 Aug 2005 18:12:09 +0000 : chx I will reroll this when I get home. Either this or Adrian's install system needs to get into 4.7. Maybe this system is not ideal and does not deal with broken modules and such but you can always enable such things by hand. So, this won't make anything worse. (And I want my pie from matthias...) ------------------------------------------------------------------------ Wed, 07 Sep 2005 04:24:48 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_3.patch (2.87 KB) Rerolled. ------------------------------------------------------------------------ Wed, 07 Sep 2005 07:51:08 +0000 : Dries I'll look into this later. (The forum module depends on the taxonomy module too. Easy enough to fix.) ------------------------------------------------------------------------ Wed, 07 Sep 2005 08:05:51 +0000 : stefan nagtegaal 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... ------------------------------------------------------------------------ Wed, 07 Sep 2005 08:37:39 +0000 : chx The dependency system itself works nicely. That we can add more stuff to use it -- true. That may come later. First, get the stuff in. ------------------------------------------------------------------------ Wed, 07 Sep 2005 09:00:36 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_4.patch (2.88 KB) Added taxonomy to forum_info. ------------------------------------------------------------------------ Wed, 07 Sep 2005 09:13:57 +0000 : Kjartan - 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? ------------------------------------------------------------------------ Wed, 07 Sep 2005 09:39:11 +0000 : chx format_plural accepts no parameters, while I have a few. The new_module logic is good because you do not recurse for every single module only after a bunch of them. Performance is better. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:31:44 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_5.patch (4.5 KB) format_plural? Be it. Recursion problems? Removed. I patched format_plural so that it accepts the same args as t and passes them over to t.

Issue status update for http://drupal.org/node/18447 Post a follow up: http://drupal.org/project/comments/add/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: Jose A Reyero Status: patch (ready to be committed) "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... Jose A Reyero Previous comments: ------------------------------------------------------------------------ Sun, 06 Mar 2005 00:55:36 +0000 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:22:17 +0000 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:26:40 +0000 : javanaut 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:

<?php function filestore2_depends() {  return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:29:24 +0000 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:33:55 +0000 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:00:27 +0000 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:11:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:12:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ Sun, 06 Mar 2005 04:13:14 +0000 : gordon +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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 08:19:09 +0000 : mathias 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? ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:42:01 +0000 : adrian 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:57:54 +0000 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ Sun, 06 Mar 2005 11:08:37 +0000 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ Tue, 08 Mar 2005 19:56:02 +0000 : Dries 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. ------------------------------------------------------------------------ Sat, 12 Mar 2005 10:37:22 +0000 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:21:21 +0000 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:29:31 +0000 : chx One s too much... ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:31:17 +0000 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ Mon, 14 Mar 2005 10:28:54 +0000 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php   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: <?php   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... ------------------------------------------------------------------------ Fri, 25 Mar 2005 23:17:35 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ Sat, 26 Mar 2005 01:46:00 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ Sat, 26 Mar 2005 03:24:59 +0000 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-) ------------------------------------------------------------------------ Sat, 26 Mar 2005 14:55:08 +0000 : stefan nagtegaal Can't we refrase this: <?php 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? ------------------------------------------------------------------------ Sat, 26 Mar 2005 20:12:52 +0000 : adrian 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). ------------------------------------------------------------------------ Sat, 26 Mar 2005 22:33:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_0.patch (2.61 KB) OK I changed back to hook_info. However, hook_info PHPdoc needs a change. ------------------------------------------------------------------------ Mon, 28 Mar 2005 00:01:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_1.patch (2.69 KB) I contacted Anisa who had a better wording idea. Thanks! To quote her: "Keep in mind that to the user, this is a surprise.  They may not have read the documentation properly, and not anything about X.  'for you' suggests that this is a service, not a scary-computer-doing-things-on-its-own behavior. I added the line about permissions and settings to remind the user that just because X module has been enabled for them, not everything will be exactly perfect." and she had had advice about changing "permissions" (and maybe settings) to a link, but that's not for this patch... it is listed in the usability group's todo for every module. ------------------------------------------------------------------------ Mon, 28 Mar 2005 02:42:35 +0000 : moshe weitzman not just permissions - some database table(s) may need manual creation. ------------------------------------------------------------------------ Sun, 03 Apr 2005 19:48:04 +0000 : stefan nagtegaal 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.. ------------------------------------------------------------------------ Sun, 03 Apr 2005 21:25:38 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_2.patch (2.9 KB) reword again to include moshe's suggestion. theme placeholder usage. proper cvs diff. ------------------------------------------------------------------------ Mon, 11 Apr 2005 23:27:53 +0000 : chx Any chance for 4.6? ------------------------------------------------------------------------ Tue, 12 Apr 2005 11:34:46 +0000 : menesis 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. ------------------------------------------------------------------------ Fri, 01 Jul 2005 18:08:12 +0000 : stefan nagtegaal 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... ------------------------------------------------------------------------ Fri, 05 Aug 2005 18:12:09 +0000 : chx I will reroll this when I get home. Either this or Adrian's install system needs to get into 4.7. Maybe this system is not ideal and does not deal with broken modules and such but you can always enable such things by hand. So, this won't make anything worse. (And I want my pie from matthias...) ------------------------------------------------------------------------ Wed, 07 Sep 2005 04:24:48 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_3.patch (2.87 KB) Rerolled. ------------------------------------------------------------------------ Wed, 07 Sep 2005 07:51:08 +0000 : Dries I'll look into this later. (The forum module depends on the taxonomy module too. Easy enough to fix.) ------------------------------------------------------------------------ Wed, 07 Sep 2005 08:05:51 +0000 : stefan nagtegaal 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... ------------------------------------------------------------------------ Wed, 07 Sep 2005 08:37:39 +0000 : chx The dependency system itself works nicely. That we can add more stuff to use it -- true. That may come later. First, get the stuff in. ------------------------------------------------------------------------ Wed, 07 Sep 2005 09:00:36 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_4.patch (2.88 KB) Added taxonomy to forum_info. ------------------------------------------------------------------------ Wed, 07 Sep 2005 09:13:57 +0000 : Kjartan - 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? ------------------------------------------------------------------------ Wed, 07 Sep 2005 09:39:11 +0000 : chx format_plural accepts no parameters, while I have a few. The new_module logic is good because you do not recurse for every single module only after a bunch of them. Performance is better. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:31:44 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_5.patch (4.5 KB) format_plural? Be it. Recursion problems? Removed. I patched format_plural so that it accepts the same args as t and passes them over to t. ------------------------------------------------------------------------ Thu, 08 Sep 2005 21:30:23 +0000 : m3avrck +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 ;-) ------------------------------------------------------------------------ Thu, 08 Sep 2005 22:47:54 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_6.patch (4.92 KB) thehunkmonkgroup asked me that if a required module is not present then what? He is right, absolutely right.

Issue status update for http://drupal.org/node/18447 Post a follow up: http://drupal.org/project/comments/add/18447 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch (ready to be committed) And what could I say that will convince you? We are trying to convince contrib authors to work together but it's a support nightmare because of dependency issues. Messages? I fail to see the point. "Warning: you enabled forum but not enabled comments. Forum won't work" What else would be my answer if "dang, why you can't do it for me??" chx Previous comments: ------------------------------------------------------------------------ Sun, 06 Mar 2005 00:55:36 +0000 : chx Attachment: http://drupal.org/files/issues/depend.patch (3.22 KB) As discussed on the developer list. This was surprisingly easy. There is still an issue: t('The configuration options have been saved.') is displayed more than once if there is a dependency forced module switch on. I have tried something (you'll see) for some reason it does not work. Number of page reloads can be decreased if Drupal could be reinitialized after system_listing_save instead of redirecting but I doubt this would be feasible. Although I marked this is feature request, this is more a usability issue (for the forum-comment dependency) and as a side effect it is a useful feature, too (for any other module "bundles"). I want criticism! I'd like to get torn apart by wild animals. Heavy! Eaten by some squirrels. ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:22:17 +0000 : clydefrog Forgive me if I'm mistaken, but shouldn't forum_depends return an array? ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:26:40 +0000 : javanaut 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:

<?php function filestore2_depends() {  return array('required'=>array('fscache'), 'optional'=>array('taxonomy','comments')); } ?> ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:29:24 +0000 : chx If you take a look at module_invoke_all, you will see that a string is fine, 'cos if you return a string it does $result[] However, an associative array is not good with module_invoke_all 'cos for returned arrays there is an array_merge where "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one." ------------------------------------------------------------------------ Sun, 06 Mar 2005 01:33:55 +0000 : javanaut Ok, how about it taking a parameter? <?php 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:00:27 +0000 : chx Attachment: http://drupal.org/files/issues/depend_0.patch (3.6 KB) I would leave associative arrays for the future. As we do not have a package tool, there is little use of 'optional' -- I'd like to get this into 4.6 After testing the patch, I have changed something, which I should not have done, so another version is attached. ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:11:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_1.patch (4.37 KB) This one should really work... ------------------------------------------------------------------------ Sun, 06 Mar 2005 02:12:13 +0000 : chx Attachment: http://drupal.org/files/issues/depend_2.patch (3.15 KB) ------------------------------------------------------------------------ Sun, 06 Mar 2005 04:13:14 +0000 : gordon +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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 08:19:09 +0000 : mathias 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? ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:42:01 +0000 : adrian 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. ------------------------------------------------------------------------ Sun, 06 Mar 2005 10:57:54 +0000 : chx Attachment: http://drupal.org/files/issues/depend_3.patch (3.26 KB) Well, I know there could be problems. However, circular dependecy is handled with my code. You have module A, this depends on B, this depends on C which depends on A. You tick A, press Save. Next listing will tick B instead of you, press Save. Next listing will tick C instead of you, press Save. Next listing will be happy, 'cos A is on. Adrian convinced me that taking a parameter is a good thing, so it's in. And he asked for calling the hook _info. So be it. And he pointed out that I do not understand drupal_set_message which was true. So now the status message is now displayed only once. Shall I protect the new error message from such a duplication? And please, get this simple system into 4.6 'cos (as already stated) the forum-comment dependency has threads on drupal.org already so it could be called a bug. ------------------------------------------------------------------------ Sun, 06 Mar 2005 11:08:37 +0000 : chx Attachment: http://drupal.org/files/issues/depend_4.patch (3.27 KB) Minor things: default hook_info key shall be called 'depends' not 'required' (Adrian). Message should be 'enabled' for consistency reasons. ------------------------------------------------------------------------ Tue, 08 Mar 2005 19:56:02 +0000 : Dries 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. ------------------------------------------------------------------------ Sat, 12 Mar 2005 10:37:22 +0000 : Dries Setting this 'active'. Waiting for new patch. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:21:21 +0000 : chx Attachment: http://drupal.org/files/issues/depend_5.patch (3.3 KB) Here is new version. Hope you like it more than the previous one. ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:29:31 +0000 : chx One s too much... ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:31:17 +0000 : chx Attachment: http://drupal.org/files/issues/depend_6.patch (3.3 KB) grrrr... it submitted without attaching... is there some way to make preview required for me...? ------------------------------------------------------------------------ Mon, 14 Mar 2005 10:28:54 +0000 : stefan nagtegaal chx: I do like the approach you took, but I think we should return more information to the administrator. You did: <?php   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: <?php   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... ------------------------------------------------------------------------ Fri, 25 Mar 2005 23:17:35 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1.patch (2.34 KB) Whole new dependency system! This has nothing to do the with previous patches. Those were a bit hackish. hook_install('depends') may return a string or an array of strings. The modules whose name is returned, will be switched on. These new modules' dependencies are also dealt with. And so on. I have thought of using format_plural, but the arguments make that impossible. ------------------------------------------------------------------------ Sat, 26 Mar 2005 01:46:00 +0000 : chx Attachment: http://drupal.org/files/issues/dependency1_0.patch (2.34 KB) A few 's are gone according to Steven's advice. ------------------------------------------------------------------------ Sat, 26 Mar 2005 03:24:59 +0000 : mathias Attachment: http://drupal.org/files/issues/dependency.patch (2.62 KB) +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 ;-) ------------------------------------------------------------------------ Sat, 26 Mar 2005 14:55:08 +0000 : stefan nagtegaal Can't we refrase this: <?php 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? ------------------------------------------------------------------------ Sat, 26 Mar 2005 20:12:52 +0000 : adrian 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). ------------------------------------------------------------------------ Sat, 26 Mar 2005 22:33:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_0.patch (2.61 KB) OK I changed back to hook_info. However, hook_info PHPdoc needs a change. ------------------------------------------------------------------------ Mon, 28 Mar 2005 00:01:32 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_1.patch (2.69 KB) I contacted Anisa who had a better wording idea. Thanks! To quote her: "Keep in mind that to the user, this is a surprise.  They may not have read the documentation properly, and not anything about X.  'for you' suggests that this is a service, not a scary-computer-doing-things-on-its-own behavior. I added the line about permissions and settings to remind the user that just because X module has been enabled for them, not everything will be exactly perfect." and she had had advice about changing "permissions" (and maybe settings) to a link, but that's not for this patch... it is listed in the usability group's todo for every module. ------------------------------------------------------------------------ Mon, 28 Mar 2005 02:42:35 +0000 : moshe weitzman not just permissions - some database table(s) may need manual creation. ------------------------------------------------------------------------ Sun, 03 Apr 2005 19:48:04 +0000 : stefan nagtegaal 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.. ------------------------------------------------------------------------ Sun, 03 Apr 2005 21:25:38 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_2.patch (2.9 KB) reword again to include moshe's suggestion. theme placeholder usage. proper cvs diff. ------------------------------------------------------------------------ Mon, 11 Apr 2005 23:27:53 +0000 : chx Any chance for 4.6? ------------------------------------------------------------------------ Tue, 12 Apr 2005 11:34:46 +0000 : menesis 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. ------------------------------------------------------------------------ Fri, 01 Jul 2005 18:08:12 +0000 : stefan nagtegaal 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... ------------------------------------------------------------------------ Fri, 05 Aug 2005 18:12:09 +0000 : chx I will reroll this when I get home. Either this or Adrian's install system needs to get into 4.7. Maybe this system is not ideal and does not deal with broken modules and such but you can always enable such things by hand. So, this won't make anything worse. (And I want my pie from matthias...) ------------------------------------------------------------------------ Wed, 07 Sep 2005 04:24:48 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_3.patch (2.87 KB) Rerolled. ------------------------------------------------------------------------ Wed, 07 Sep 2005 07:51:08 +0000 : Dries I'll look into this later. (The forum module depends on the taxonomy module too. Easy enough to fix.) ------------------------------------------------------------------------ Wed, 07 Sep 2005 08:05:51 +0000 : stefan nagtegaal 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... ------------------------------------------------------------------------ Wed, 07 Sep 2005 08:37:39 +0000 : chx The dependency system itself works nicely. That we can add more stuff to use it -- true. That may come later. First, get the stuff in. ------------------------------------------------------------------------ Wed, 07 Sep 2005 09:00:36 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_4.patch (2.88 KB) Added taxonomy to forum_info. ------------------------------------------------------------------------ Wed, 07 Sep 2005 09:13:57 +0000 : Kjartan - 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? ------------------------------------------------------------------------ Wed, 07 Sep 2005 09:39:11 +0000 : chx format_plural accepts no parameters, while I have a few. The new_module logic is good because you do not recurse for every single module only after a bunch of them. Performance is better. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:31:44 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_5.patch (4.5 KB) format_plural? Be it. Recursion problems? Removed. I patched format_plural so that it accepts the same args as t and passes them over to t. ------------------------------------------------------------------------ Thu, 08 Sep 2005 21:30:23 +0000 : m3avrck +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 ;-) ------------------------------------------------------------------------ Thu, 08 Sep 2005 22:47:54 +0000 : chx Attachment: http://drupal.org/files/issues/dependency_6.patch (4.92 KB) thehunkmonkgroup asked me that if a required module is not present then what? He is right, absolutely right. ------------------------------------------------------------------------ Sun, 11 Sep 2005 15:46:05 +0000 : Jose A Reyero "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... ------------------------------------------------------------------------ Sun, 18 Sep 2005 17:21:21 +0000 : Dries Just like Jose, I'm not convinced automatically enabling modules is a good thing.