Because your module wants to implement custom_url_rewrite_outbound, if other modules want to define it or implemented they are not able to since it can only be defined once and could possibly cause PHP fatal errors in other modules if they do not check if the function is not already defined.
I've created the URL alter module to help convert this into a hook that modules can use, and I want to provide patches to all the modules that want to implement custom_url_rewrite functions to be able to take advantage of it.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | fb_url_alter.diff | 7.26 KB | Dave Cohen |
| #1 | 530352-url-alter.patch | 7.71 KB | dave reid |
Comments
Comment #1
dave reidPatch attached for review.
Comment #2
Dave Cohen commentedI 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.
Comment #3
dave reidThat'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. :)
Comment #4
avpadernoDefining an empty hook is a way better than forcing users to add some code into their settings.php.
Comment #5
Dave Cohen commentedIf 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?
Comment #6
dave reidurl_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.
Comment #7
avpadernoShould not the patch add
fb_boot()? Ifcustom_url_rewrite_inbound()is called during the path bootstrap, then the module must implementhook_boot()(even if the hook doesn't execute any code).Comment #8
dave reid@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.
Comment #9
Dave Cohen commentedFYI I'm testing the patch on my local server. Will check it in once I have put it through its paces.
Comment #10
Dave Cohen commentedHere's the patch I am testing (added change to fb_canvas.module)
Comment #11
avpadernoSee #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 functioncustom_url_rewrite_inbound()Drupal for Facebook defines when url_alter.module is not enabled.Comment #12
Dave Cohen commentedMaybe 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.
Comment #14
dave reidFollow-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)!
Comment #15
Dave Cohen commentedPlease don't make me do this again.
;)
Comment #16
dave reidI 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. :)
Comment #18
socialnicheguru commentedWhich patch in this thread is the correct one?
Thanks.
Comment #19
Dave Cohen commentedYou should no longer need the patch. That latest modules include it. If the two modules are not working together, please submit a new issue.