require_once(drupal_get_path('module', 'activity') ."/contrib/{$module}/{$module}.module");

is pretty bad. Makes upgrading annoying (as I can't just remove the directory for a new version, I have to carefully pick out any extra modules I may have added), and development painful. Note that Drupal always knows the path to a module, regardless of whether it is installed or not - you can run drupal_get_path('module', 'favoritenodesactivity') just fine. This would immediately solve my annoyance, and open the path to easier third party development.

CommentFileSizeAuthor
#7 activity.module.355633.patch3.27 KBjaydub

Comments

morbus iff’s picture

Title: Forced contrib path makes upgrading annoying, development lame » Forced development assumptions are annoying

Another annoyance:

  $module_nice_name = drupal_ucfirst(str_replace('_', ' ', substr($module, 0, -8)));

which makes it impossible for a properly named module to implement your hooks. I should be able to have a module called "mymodule.module" that responds to your hook, without forcing me to create yet another module called mymoduleactivity.

jaydub’s picture

Ok I replaced the use of require_once in the original issue with the d6 function module_load_include() which should be more flexible.

Will come back and address #1 later.

morbus iff’s picture

#1 should be ripped out entirely. My feeling is:

* rip out the "list all the settings at the bottom of the Activity Settings page"
* force each module to define their own menu underneath the primary settings, as they should.

The current functionality surrounding #1 is just plain ol' bad design.

morbus iff’s picture

As for module_load_include(), that's not the right answer. It works, from a technical standpoint, but it's a direct abuse and overloading of the function. The function is focused around loading /include/ files, not the actual .module file itself. Just because it CAN, due to the way it has been programmed, doesn't mean you should ignore the /intent/ behind its documentation and purpose.

Unless you can find an instance in core that abuses the function this way, please change it back to include_once and drupal_get_path.

lelizondo’s picture

Priority: Normal » Critical

#359209: require_once() Bug is related to this issue.. I'm using 5.x-4.x.. I think this should be critical since I can't install because of:

Fatal error: require_once() [function.require]: Failed opening required 'sites/all/modules/activity/contrib/ogactivity/ogactivitymodule' (include_path='.:/usr/share/php:/usr/share/pear') in /pathgoeshere/sites/all/modules/activity/activity.module on line 895
sirkitree’s picture

Priority: Critical » Normal
Issue tags: +activity-6.x-1-0-rc1
jaydub’s picture

Assigned: Unassigned » jaydub
Status: Active » Needs review
StatusFileSize
new3.27 KB

Going back to #1, this patch removes the 'nice name' code in favor of something that doesn't depend on the module being named XXXXactivity.module.

Still think it's ok to have the settings menu item for each module be generated in activity.module rather than in each contrib.

sirkitree’s picture

Assigned: jaydub » sirkitree
Category: bug » task
Status: Needs review » Postponed

The patch is fine, but really doesn't address the underlying issue here, which is that we're not using the hook system correctly at all. We're forcing modules to have a particular naming conventions in order to comply to our wishes, instead of just providing a hook definition that ANY module can implement. This is actually what led to some of our headaches in having to maintain these sub-modules in the first place.

The next version of this module will not have any of these limitations and we'll be making better use of drupal_alter(), module_implements(), and module_invoke_all(). Let's postpone this one as it's a pretty major undertaking.

sirkitree’s picture

I'm doing a little preliminary work here and trying to get feedback on the next version. Posting a link for reference.
http://groups.drupal.org/node/19249#comment-67046

sirkitree’s picture

Status: Postponed » Closed (won't fix)

closing. 1.x no longer supported.