As discussed in #567564: Disable the taxonomy/term display by default we shouldn't use the menu_alter to redirect merged terms, because this breaks other modules like Taxonomy Breadcrumb and Panels3.
By default, the redirect is now disabled and it's working with other modules, but there might be better ways to implement this.
So I started an issue where we can work on a patch

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna’s picture

Subscribing. I suggest using Path_Redirect if available.

DamienMcKenna’s picture

Initial patch that..

  • creates path_redirect values for the taxonomy/term/[tid] URL of the $merging_terms,
  • creates path_redirect values for any url_alias URLs of the $merging_terms,
  • updates any existing path_redirect values that already existed to point to the $main_term.
DamienMcKenna’s picture

Status: Active » Needs review
dankohn’s picture

That patch has an error in it. You need to change module_enabled('path_redirect') to module_exists('path_redirect')

With that change, the patch appears to do the right thing and is a welcome improvement.

Given that I also confirmed the incompatibility with Panels 3, I would suggest removing the existing redirect functionality entirely, and have the module just do a path_redirect by default instead (but still allow the checkbox to be unchecked). That would make path_redirect a prerequisite for this module, which would be fine.

DamienMcKenna’s picture

Gah. I forgot to update the module with my local test code. Here's a corrected patch.

digi24’s picture

Thanks for the patch Damien. Maybe it would be useful to add a default option or omit the whole options thing. After all, it is the expected behaviour, when using the redirect module:

   if (count($options) > 0) {
     $form['merge']['options'] = array(
       '#type' => 'checkboxes',
       '#title' => t('Options'),
       '#options' => $options,
     );
+    if (module_exists('path_redirect')) {
+      $form['merge']['options']['#default_value'] = array('merge_redirect');
+    }
dicreat’s picture

Thanks for the patch Damien and digi24.
I just make little fix - now patch works with new Path redirect module version 6.x-1.0-beta6. Please review.
Be careful, new version of Path redirect work only with Pathauto 6.x-1.x-dev.

yan’s picture

Status: Needs review » Needs work
FileSize
1.95 KB

Patch failed against 6.x-2.x-dev:

$ patch < taxonomy_manager.admin_.inc_.patch 
patching file taxonomy_manager.admin.inc
Hunk #1 succeeded at 573 (offset 1 line).
Hunk #2 FAILED at 2127.
1 out of 2 hunks FAILED -- saving rejects to file taxonomy_manager.admin.inc.rej
yan’s picture

Status: Needs work » Needs review
FileSize
3.02 KB

Adapted the patch to latest -dev, works fine for me.
Please review.

yan’s picture

Patch from #9 works fine for me, just one "whitespace error":

path_redirect_6.patch:12: trailing whitespace.
$options['merge_redirect'] = t('Use path_redirect to redirect all old URLs to the new term.'); 
Checking patch taxonomy_manager.admin.inc...
Applied patch taxonomy_manager.admin.inc cleanly.
warning: 1 line adds whitespace errors.
kardave’s picture

Applied the last patch against the 6.x-2.2 with no errors or warning. Redirection now works fine!
Thank you very much!

yan’s picture

Status: Needs review » Reviewed & tested by the community

Tried it again against dev and it works just fine. Patch output is this:

$ patch < path_redirect_6.patch 
patching file taxonomy_manager.admin.inc
Hunk #1 succeeded at 571 (offset -1 lines).
Hunk #2 succeeded at 585 (offset -1 lines).
Hunk #3 succeeded at 2131 (offset 1 line).

Can this be committed?

yan’s picture

Confirming again that this works. RTBC

ivnish’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Closed (outdated)