Remove bootstrap_menu_local_task() override since it's no longer needed

Maybe whenever stuff like this happens, it is time to replace the ternary ?: with if / else :)

git blame pointed me to commit e165225ed5 and issue #2827761: Menu HTML breaks.

And commit b8be0646716d9bd6066128e403fcc9b8e22cefbd with message "Fix bugs".

(I wrote this before your comment, I think it applies anyway)

The implementation in Drupal core seems fine,
https://api.drupal.org/api/drupal/includes%21menu.inc/function/theme_men...

function theme_menu_local_task($variables) {
  $link = $variables['element']['#link'];
  $link_text = $link['title'];

  if (!empty($variables['element']['#active'])) {
    // Add text to indicate active tab for non-visual users.
    $active = '<span class="element-invisible">' . t('(active tab)') . '</span>';

    // If the link does not contain HTML already, check_plain() it now.
    // After we set 'html'=TRUE the link will not be sanitized by l().
    if (empty($link['localized_options']['html'])) {
      $link['title'] = check_plain($link['title']);
    }
    $link['localized_options']['html'] = TRUE;
    $link_text = t('!local-task-title!active', array('!local-task-title' => $link['title'], '!active' => $active));
  }

  return '<li' . (!empty($variables['element']['#active']) ? ' class="active"' : '') . '>' . l($link_text, $link['href'], $link['localized_options']) . "</li>\n";
}

The $link_text is left untouched for non-active local tasks.
Only for the active local task, the link text is checkplained before going into t().

The smelly thing about this core code is how it writes to array values instead of using local variables. But this should not have visible effects, it is just bad practice.

So why the different logic in Bootstrap?

About your patch..

+  if (empty($options['html'])) {
+    $title = filter_xss_admin($link['title']);
+    $options['html'] = TRUE;
+  }

Core theme_menu_local_task(), via l(), would have applied check_plain() in this case. In your patched code you apply filter_xss_admin() instead of check_plain(). I'd say this is probably wrong..

This comment is still wrong..

+  // Otherwise, just pass the title as is and l() will use check_plain().
+  else {
+    $title = $link['title'];
+  }

In this case the thing will *not* be filtered by l() because $options['html'] === TRUE.

Otherwise the code looks like it is mostly equivalent with core.. no additional classes added.
Maybe we could drop this theme implementation altogether!

If we do this, I think it might still be wise to have
- one commit to fix the theme implementation (possibly replacing it with a copy from core)
- another commit to drop the theme implementation.

This gives a more useful git history, and allows to revert to a non-broken state.

Another difference I notice to core is the use of drupal_attributes()..

  $attributes = array();
  [..]
  if (!empty($variables['element']['#active'])) {
    [..]
    $attributes['class'][] = 'active';
    [..]
  }
  [..]
  return '<li' . drupal_attributes($attributes) . '>' . l($title, $href, $options) . "</li>\n";

The only two possible values of $attributes are array() and array('class' => array('active')).
So it would be faster to just do (!empty($variables['element']['#active']) ? ' class="active"' : '') like core.

Thanks!

Kind of funny how a bug actually determines that some inherited legacy code is irrelevant :D

Or it means that noone likes to review existing code unless there is a bug..
(no blame or guilt implied)

So, I'm ok with your proposal and will commit both shortly:

- one commit to fix the theme implementation (possibly replacing it with a copy from core)
- another commit to drop the theme implementation.

function bootstrap_menu_local_task($variables) {
  return theme_menu_local_task($variables);
}

My idea was to insert a copy of core implementation instead of this call.. this would allow to see the diff between the different implementations.
Then the second commit message would say "drop custom implementation, because it is identical with core".
But I don't care, main thing is it is fixed :)