Integrate with url_alter.module

Patch attached for review.

I can't recall the exact case, but I'm certain that (at least for D5 when I originally developed this) the custom_url rewrite function had to be defined before modules are loaded. That's why they're in the settings include file, and not in a module like with domain or i18n modules.

So my recomendation for users of the url_alter module is that they define custom url alter function in their settings.php file. And those definitions have to be smart enough to call the url_alter function if they are defined, and the fb functions if they are defined.

I appreciate the patch, but I'm afraid if I move these functions into the module something will break.

That's not the reason with Drupal 6. custom_url_rewrite_outbound is only used with url() which is only loaded on a full bootstrap which includes all modules. We have not yet had any problems with other module implementing it yet, so I'd really love for you to reconsider.

Edit: custom_url_rewrite_inbound is called during the path bootstrap, but we can get around this by adding an empty hook_boot() so that the module file is included during the cache bootstrap, which is earlier than the path bootstrap.

So really, there's nothing wrong. :)

Defining an empty hook is a way better than forcing users to add some code into their settings.php.

If I define custom_url_rewrite_inbound() during cache bootstrap, will that screw up url_alter.module, or is that where it defines it?

Does url_alter.module rely on it's weight to come before other modules?

url_alter.module implements an empty hook_boot() and has a module weight of -1000. So if there are no definitions of custom_url_rewrite_* functions in the settings.php file, the url_alter.module gets to define the functions first. Using the patch in #1 will allow fb.module to work gracefully when both url_alter.module is enabled or disabled.

Should not the patch add fb_boot()? If custom_url_rewrite_inbound() is called during the path bootstrap, then the module must implement hook_boot() (even if the hook doesn't execute any code).

@KiamLaLuno: Oh crap, that's a good point. That's the one downside at the cost of making this function work with more than one module. Luckily that's one thing we *won't* have to do with the new code registry in Drupal 7. I'm so looking forward to it.

FYI I'm testing the patch on my local server. Will check it in once I have put it through its paces.

Here's the patch I am testing (added change to fb_canvas.module)

See #7, #8; if the module doesn't implement hook_boot() (even using an empty function), Drupal will not find the function used as implementation of the custom hook defined by url_alter.module, nor will it find the function custom_url_rewrite_inbound() Drupal for Facebook defines when url_alter.module is not enabled.

Maybe my memory is faulty regarding the need to have custom_url function before modules are loaded. I've been testing the patch without fb_boot defined, and haven't noticed problems. Then again, haven't tested all that much. So if problems occur I'll revisit this.

Until then, I've checked in the patch #10.

I'm not ignoring your comment, KiamLaLuno, but I don't understand the issues entirely, and having these edits in my files without checking them in was causing me grief.

Also, I have not tested with url_alter.module, only without it.

If you guys understand a problem that I don't, please submit another patch here. Thanks.

Automatically closed -- issue fixed for 2 weeks with no activity.

Follow-up notice that I'm going to be committing a new version of the URL alter module that changes the hooks to hook_url_inbound_alter() and hook_url_outbound_alter(). This is for consistency with the hooks that were accepted into Drupal 7 (#320331: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks)!

Please update fb_url_alter_inbound() to fb_url_inbound_alter() and fb_url_alter_outbound() to fb_url_outbound_alter() as soon as you can since I'm creating the new URL alter release tomorrow. Plus this will make your module one-step closer to being Drupal 7 compatible (then you can even drop all the custom_url_rewrite juggling)!

Please don't make me do this again.

;)

I promise. I apologize for the lack of oversight that the hooks were named incorrectly. They won't change anymore since they're the same as the D7 core hooks. :)

Automatically closed -- issue fixed for 2 weeks with no activity.