Fix module_enable call for i18n_block

Looks great to me!

I spoke too soon, we should probably use render caching instead with a #pre_render function.

The reason is that there can be role specific parts (as usual for menus), so DRUPAL_CACHE_PER_ROLE and also there is at a minimum i18n support via language needed, which drupal_render_cid_parts() will take care of.

I think given that some of this could be PER_USER (in certain scenarios), we would need to either:

a) Add a variable to turn it off / on
b) Add an alter hook to change the cache_info before returning?

Or would hook_block_view_alter() be needed to be used for that?

I have domain access enabled on my site. Would the mentioned patch #1 handle the caching with domain access enabled

This looks like it has a similar intent, but different approach from #2187927: Mega Menu Blocks don't cache

Part of the issue is that module_enable is called. Uhh... why is this? This should really be module_exists and then a conditional if it does. Currently, if i18n_block doesn't exist on a site it'll sit here and churn every page load.

Even if it does exist... it'll still churn every page load off of this one call.

Yeah I'm thinking this is actually just a mistake now that I see it.

If you'd like profiling proof, this patch did this to a site I was debugging:

Prior to patch
Executed 519 queries in 189.28 ms. Queries exceeding 5 ms are highlighted. Page execution time was 1819.86 ms. Memory used at: devel_boot()=2.12 MB, devel_shutdown()=51.2 MB, PHP peak=53.75 MB.

AFTER FIXING TB_MEGAMENU
Executed 200 queries in 53.01 ms. Queries exceeding 5 ms are highlighted. Page execution time was 510.65 ms. Memory used at: devel_boot()=2.18 MB, devel_shutdown()=46.77 MB, PHP peak=48.25 MB.

uhhhhhhhhhhhhh.....

Uh, yes ...

RTBC - Nice find, @btopro!

This is critical as its a module_enable call at runtime!

( I thought variable_set was the worst I would see at runtime for every request ...)

Just goes to show, there's nothing new on the internet.. I just traced this & arrived to report it too :)

Thanks @Catch!

ok so can we get #9 in and speed up 9,500+ sites when they upgrade? :)

I can't apply #4 against latest dev version.
If I apply it manually, I get no errors, but also no data where the block should be.

The function before patch now looks like this:

/*
 * Implementation of hook_block_view()
 */
function tb_megamenu_block_view($delta = 0) {
  static $added_js_css = false;
  if(!$added_js_css) {
    $added_js_css = true;
    if(module_exists('fontawesome') && file_exists(libraries_get_path('fontawesome') . '/css/font-awesome.css')) {
      tb_megamenu_add_css(libraries_get_path('fontawesome') . '/css/font-awesome.css');
    }else{
      drupal_add_css(FONT_AWESOME_44, array(
        'type' => 'external'
      ));
    }

    tb_megamenu_add_css(drupal_get_path('module', 'tb_megamenu') . '/css/bootstrap.css');
    tb_megamenu_add_css(drupal_get_path('module', 'tb_megamenu') . '/css/base.css');
    tb_megamenu_add_css(drupal_get_path('module', 'tb_megamenu') . '/css/default.css');
    tb_megamenu_add_css(drupal_get_path('module', 'tb_megamenu') . '/css/compatibility.css');
    drupal_add_js(drupal_get_path('module', 'tb_megamenu') . '/js/tb-megamenu-frontend.js');
    drupal_add_js(drupal_get_path('module', 'tb_megamenu') . '/js/tb-megamenu-touch.js');
  }
  return array('content' => array(
    '#type' => 'markup',
    '#markup' =>  theme('tb_megamenu', array('menu_name' => $delta))
  ));
}

and this is how I applied the patch:

/*
 * Implementation of hook_block_view()
 */
function tb_megamenu_block_view($delta = 0) {
  static $added_js_css = false;
  if(!$added_js_css) {
    $added_js_css = true;
    if(module_exists('fontawesome') && file_exists(libraries_get_path('fontawesome') . '/css/font-awesome.css')) {
      tb_megamenu_add_css(libraries_get_path('fontawesome') . '/css/font-awesome.css');
    }else{
      drupal_add_css(FONT_AWESOME_44, array(
        'type' => 'external'
      ));
    }

    tb_megamenu_add_css(drupal_get_path('module', 'tb_megamenu') . '/css/bootstrap.css');
    tb_megamenu_add_css(drupal_get_path('module', 'tb_megamenu') . '/css/base.css');
    tb_megamenu_add_css(drupal_get_path('module', 'tb_megamenu') . '/css/default.css');
    tb_megamenu_add_css(drupal_get_path('module', 'tb_megamenu') . '/css/compatibility.css');
    drupal_add_js(drupal_get_path('module', 'tb_megamenu') . '/js/tb-megamenu-frontend.js');
    drupal_add_js(drupal_get_path('module', 'tb_megamenu') . '/js/tb-megamenu-touch.js');
  }
//  return array('content' => array(
//    '#type' => 'markup',
//    '#markup' =>  theme('tb_megamenu', array('menu_name' => $delta))
//  ));
  
  return array(
    'content' => array(
      '#cache' => array(
        'bin' => 'cache_block',
        'keys' => array(
          'tb_megamenu',
          $delta,
        ),
        'granularity' => array(
          DRUPAL_CACHE_PER_ROLE,
        ),
      ),
      '#pre_render' => array('_tb_megamenu_block_pre_render'),
      '#delta' => $delta,
    ),
  );
}

/**
+ * Pre render function for tb_megamenu_block_view().
+ */
function _tb_megamenu_block_pre_render($element) {
  $element['menu']['#markup'] = theme('tb_megamenu', array('menu_name' => $element['#delta']));
  return $element;
 }

Any ideas?

@aambro are you referring to the right patch and the right issue here? this patch doesn't deal with 'fontawesome' at all...

Applied patch #9 against 7.x-1.0-rc2 manually. Works fine. Thanks!

Applied patch #9 manually against 7.x-1.0-rc1 works and reverts visible issues in megamenu as well. (module was frozen at rc1 in a testing environment when I discovered its update was an issue).

This is not only a performance issue, but also causes i18 to be enabled but not intended to be if it was installed, but not enabled already. This can happen in cases such as multi-site where the module is in all sites and needed for another site, but not the site tb_megamenu is enabled in. Or in the case of i18 module not being used, but still in the modules directory (poor code cleanup. it happens). In this case if the module is very out of date because it is not being updated as a disabled module, it is possible this bug could open a security risk.

This patch is very simple and straightforward and should be fast-tracked to a recommended release.

Just piling on that this should get in ASAP.

This patch does indeed fix the big no no of module_enable, but I have found an issue where we shouldn't really be just checking if the module is enabled, we should be checking if i18_mode is actually enabled on this block.

Just because the module is enabled does not mean that this block has had any translation specific settings applied to it.

I've discovered an issue where because the module is enabled, but the block in question does not necessarily have any translation settings applied to it (i18n_mode == 0), and because the block title comes is not set in the block->title but instead comes from views, available in $block_content['subject'] we end up with no block title being set from this logic block:

      if (isset($vars['showblocktitle']) && $vars['showblocktitle']) {
        // If i18n_block is not enabled, so we will set title to subject.
        if (!$is_enabled_i18n_block) { // **I don't run as i'm just checking if the module is enabled, NOT if translation enabled on this block**
          $block->subject = $block->title ? $block->title : $block_content['subject'];
        }
        elseif (!empty($block->title) && $block->title != '<none>') { // ** I don't run, as $block->title is not set, but we have a title in $block_content['subject'] that will currently never get used.**
          // Check plain here to allow module generated titles to keep any markup.
          $block->subject = $block->title;
          $block->subject = i18n_string_plain("blocks:$block->module:$block->delta:title", $block->subject);
        }
      }

The patch i've written here checks against if we have actually got $block->i18n_mode enabled, then we can safely say that the block has i18n enabled settings, thus should have a block->title that will be set (for translation purposes).

This will then solve the issue with block titles that are coming from views that are currently missing, just because the i18n_block module is enabled but not necessarily used on this block.

I have faced with the same performance issues. And fixed it in the same way as suggested in
tb-megamenu-fix-performance-2370309-9.patch

Please add patches to main code branch. Current module state (tb_megamenu-7.x-1.0-rc2) is really frustrating.

RTBC for #18, this needs to be fixed.

RTBC for #18 too. Can be committed to the main branch .

The patch from #18 works like a charm.

Yay finalllyyyyy found what was choking this module! TYVM for patch

#18 really needs to be pushed to the stable release. I didn't read this post before profiling the site and finding that module_enable was a HUGE issue in our website and I had to patch it for the site to go life. After applying the same patch as #9 our site speed was increased by 800%.

I contacted the author of the module and asked to fix this issue and create a new stable release.

I hope they will react.