Request to redirect URLs for comments to the parent node

Basically, I'm seeing people starting landing on my site at the comment pages. That's flagging up the problem for me. It's only a matter of time before my own SEO takes a big hit.

I've just started playing around with http://drupal.org/sandbox/Ayesh/1578662.

It doesn't rewrite the URLs, but it uses drupal_goto to send any visitor who lands on /comment/[cid] to the right page. And - it seems to get the comment pagination right by calling comment_get_display_page().

IIRC, drupal_goto uses 301, so that should mitigate the SEO hit almost entirely. Including it in the URL rewriting would be even better, but even this would be better than nothing.

How would you feel about incorporating the few lines of code in that sandbox module (with due credit to Ayesh)?

I'd happily have a go at writing a patch for G-R for it.

Well, ours has reached 12 months now, so I'll give it a go ;-)

See attached.

• Added an admin option to toggle this new feature
• Added a hook to menu_alter that sends visitors to pages with URL /comment/[cid] to the correct page of the parent node
• {Tried to follow what seem to be your naming conventions etc!}

Two things that I think could be better, but I don't know (quickly - which is all the time I've got right now) how to solve them.

1. It would be nice if Drupal actually rewrote the URL, so that the /comment/[cid] URLs do not appear in menus, views, etc. That would be more elegant than just redirecting the requests.
2. Initially, I put the check to see if this setting applied into the internal function that handled the rewriting. The problem with that was that if the setting was turned off, hook_menu_alter had already been called, which meant that nothing was returned to the browser at all. So I moved the check into the hook_menu_alter function itself. Now the internal function doesn't even get called if this particular feature is disabled. That solved that problem, but it means that the menu caches need to be cleared whenever this particular feature is enabled/disabled, which is not ideal.

What d'ya think?

One more thought - do we need to check that what follows the "comment/" line is only numeric, to prevent potential injection flaws?

We could just do a menu_rebuild

Agreed, in principle. Using admin_menu, it certainly seems that the menu cache is the specific cache to flush in order to make the change effective.

For some reason, though, putting a call to menu_rebuild at the end of globalredirect_settings_submit_save doesn't have this result. If the comment-url rewriting feature is turned off at the settings page, then comment URLs stop being rewritten straight away. However when it's turned on, it needs a subsequent call to rebuild the menu. Perhaps I'm calling it too early, but I can't see where else I can hook a menu rebuild. (menu_rebuild returns true in either case)

>> Rewriting the URLs would be nice (maybe hook_url_outbound_alter?) - however the "scope" of GlobalRedirect is to maintain site structure via URL redirects, not to rewrite the content on the site. I feel that when features like that creep in, the weight of the module starts to increase. Bearing in mind this is something that gets included on every page and runs all the time, it needs to be lightweight. (IMHO)

I agree with you, there.

In theory, no check should be needed. The parameter gets passed into your function which then tries to load it. Non-numeric would fail to load a comment and therefore nothing would happen.

And agree there too. The drupal_not_found() call should take care of all such cases.

Something's not right with this.

Periodically, the setting seems to be being turned off. The check-box on Global Redirect's settings page remains checked, but the /comment/cid pages stop being redirected as they should be. Using drush cc all doesn't fix this, but using admin_menu to flush all caches in a web browser instantly fixes it and pages redirect again as they should do.

Putting a few calls to watchdog shows that the hook_menu_alter function isn't being called. That's getting too deep into the core hook for me to understand exactly what's failing. Why would a module register a new hook function like this, and then it suddenly stops being invoked?

Can @ayesh or @nicholasThompson shed any light on this one?

Just for the sake of a complete record, this went in at http://drupalcode.org/project/globalredirect.git/commit/c500ede

It's flagged as an experimental implementation, so I guess review and feedback is still welcomed to help the maintainers work out whether this is the correct way to implement this functionality.

It looks like this got lost when Global Redirect merged into Redirect. #3495377: /comment/ID does not redirect to the canonical URL