refactor includes to pull hooks into the module

Is this really safe to do? Can I safely move the lines:

// Look for any extensions installed in the pathauto directory
$path = drupal_get_path('module', 'pathauto');
$files = file_scan_directory($path, '^pathauto_.*\.inc$');
foreach ($files as $filename => $file) {
  include_once($filename);
}

to pathauto_menu(!$may_cache)?

Can I get an answer on this, please? I'm really not sure what to do.

I think it's totally safe! It will execute on every page-view though, so I really don't see the point in doing that, except for comforming to standards - which is important.

here's a patch to do this...I'd appreciate a second set of eyes if anyone wants to confirm this concept

status update...

I've been testing this for a little while and it seems to work fine - will apply in the next few days.

Not sure on this one, I think there is a better way todo this..

Maybe we can have a look how system.module and image.inc do use this kind of code and learn from there.
Will investigate soon.

Moshe, do you have any recommendations about this?

If we look at common.inc (which includes image.inc) we see that _drupal_bootstrap_full() includes the image.inc file. That's not really comparable to the way that pathauto works.

If we look at og.module we see that it includes the files in og_init.

The docs for hook_init make it pretty clear that hook_menu is the right place for this.

Steef and I discussed this on IRC and he agreed to this approach.

Fixed in 4.7 and HEAD.

Thanks, moshe.

This actually breaks pathauto when doing updates to a node so I'm reverting it. We'll have to find a better solution.

Is there any update on why pathauto declarations are problematic at hook_menu time ?

CCK currently implements hook_init only for this reason (well, we include views integration stuff there too, but that could easily go away to hook_menu).

hook_init forces content.module to be loaded when serving cached pages (yerk !), and I'd really like to get rid of that unneeded performance hit in the next release.

yched - you can try the patch from #4 (it may not apply, but the change is trivial) and then test pathauto.

AFAICT this breaks pathauto in about 75% of the use cases. So, that's bad (TM).

I believe the problem is that the .inc files implement the hook_node/hook_user etc. and that if those are not in the .module OR included at the top then they won't work.

I guess one solution would be to refactor the code and put hook_node and hook_user in the pathauto.module and then have them call other functions.

It would be nice to get a second opinion on this - yched, do you have a second to test out the patch from #4 (to confirm my memory) and provide input on the idea of the hooks?

I believe the problem is that the .inc files implement the hook_node/hook_user etc. and that if those are not in the .module OR included at the top then they won't work.

Yes, that would be probable.

But then, sorry for being selfish :-), my main concern here is the way we in CCK include our own small 'content_pathauto.inc' file - this one does not define any 'regular' (core) hooks, only hook_pathauto_node.

I'm wondering if the include of this file can be safely moved from content_init to content_menu($may_cache = false).
I'm not familiar with pathauto's guts, and can't seem to find in the code where hook_pathauto_node is invoked.

@yched well, in 5.x-1 hook_pathauto_node is invoked on line 58. http://cvs.drupal.org/viewcvs/drupal/contributions/modules/pathauto/path...

In 5.x-1 it's no longer invoked :) Instead it just calls token_get_list for the settings page and token_get_values during the hook_nodeapi hook_taxonomy hook_user. So, basically I think in both cases it probably should be fine to put it in a hook_menu(FALSE).

I'm changing the title and purpose of the issue to the real point - refactoring pathauto so that we don't need to include these all the time and moving some of the unnecessary functions out of the main .module file.

Re-arranged in the DRUPAL-5--2 branch. Hopefully it works this time ;)

@yched - I believe you can (if you haven't already) move your includes to the hook_menu !$may_cache.

@greggles : cool, thanks for letting me know :-)
This is true only for pathauto 5.x-2 branch, though, right ? That would mean officially requiring pathauto 2 for CCK ?
I see the project page now lists 2.0 as the official release (I think it was still 1.n last time I checked). Maybe we should wait 'til it gets out of beta ? We made an early move recommending Views 1.6 while it was still beta, and we might not want to repeat that :-)

Related point : Do you know if current CCK will work indifferently with pathauto 1.x and 2 ? If not we should probably add a compatibility note on the project(s) page(s), and maybe figure a way to have the next CCK release be a transition one (working with both) ?

@yched - I think it should work for both 5.x-1 and 5.x-2, really. I'm not suggesting that you reqiure or recommend Pathauto 5.x-2.x just yet. I think that the cck<->token<->pathauto feature set is greater than the current cck<->pathauto feature set, but I don't really know too closely.

I think that Drupal 6.x-1.x version of CCK will provide a natural place to drop support for Pathauto 5.x-1. The 6.x version of pathauto will only be this "version 2" token-dependent style.

OK - It's just that the pathauto code is 'troublesome' only for CCK D5 (forces us to use hook_init and thus loads the entire content.module even for cached pages). In D6 we shouldn't have that problem anyway (hook_init / hook_boot have changed slightly)
If moving to pathauto 2 (when it's ready) for D5 allows us to solve that, I'm all for it.