After updating to 6.x-1.2 today I noticed that none of the other JavaScript on my site (some jQuery plugins and FCKeditor) worked anymore in Firefox 3.0.5/Mac, Safari 3.2.1/Mac, and in FF 3.0.5/Win. The error consoles in all browsers reported an undefined value in line 6 of admin_menu.js; Firefox specifically reports that Drupal.settings.admin_menu is undefined. IE7 and IE6 on Windows both report the same error, referring specifically to Drupal.settings.admin_menu.margin_top, but the other JavaScript on the site continues to work in those browsers.

This is the offending snippet of code from 6.x-1.2:

$(document).ready(function() {
  // Apply margin-top if enabled; directly applying marginTop doesn't work in IE.
  if ($('#admin-menu').size()) {
    if (Drupal.settings.admin_menu.margin_top) {
      $('body').addClass('admin-menu');
    }
    if (Drupal.settings.admin_menu.position_fixed) {
      $('#admin-menu').css('position', 'fixed');
    }
    // Move page tabs into administration menu.
    if (Drupal.settings.admin_menu.tweak_tabs) {
      $('ul.tabs.primary li').each(function() {
        $(this).addClass('admin-menu-tab').appendTo('#admin-menu > ul');
      });
      $('ul.tabs.secondary').appendTo('#admin-menu > ul > li.admin-menu-tab.active');
    }
  }

If I remove the entire if statement for Drupal.settings.admin_menu.margin_top, I then get the same undefined value error for Drupal.settings.admin_menu.position_fixed, and if I remove that if statement, I then get the error for Drupal.settings.admin_menu.tweak_tabs. So the root of the problem seems to be that none of those values are being properly defined.

I checked out admin_menu.module and compared the part of the code that attempts to define Drupal.settings.admin_menu.margin_top to the same function in the 6.x-1.1 version. I replaced this code from 6.x-1.2 admin_menu.module line 91:

if ($setting = variable_get('admin_menu_margin_top', 1)) {
      drupal_add_js(array('admin_menu' => array('margin_top' => $setting)), 'setting');
    }

with this code from 6.x-1.1 admin_menu.module line 96:

drupal_add_js(array('admin_menu' => array('margin_top' => variable_get('admin_menu_margin_top', 1))), 'setting');

and everything worked fine again: no JavaScript errors, no undefined values, and all unrelated JavaScript working normally in all browsers tested.

I can't figure out how to fix the root problems here, but hopefully this will help someone else! I did note that the $setting variable from the 6.x-1.2 admin_menu.module snippet above doesn't seem to actually be set anywhere, in case that helps.

CommentFileSizeAuthor
#21 admin_menu-DRUPAL-6--1.js-settings.patch1.52 KBsun
#9 admin_menu-DRUPAL-6--1.js-settings-error.patch1.03 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Postponed (maintainer needs more info)

I cannot replicate this error - do you have JS aggregation enabled? If you have, did you clear your cache?

jackalope’s picture

jackalope commented

That's the Optimize JavaScript Files setting in admin/settings/performance, right? If so, then no, I had that disabled.

jackalope’s picture

jackalope commented

Ack. After all of that, suddenly I can't replicate the problem, either. I'm not sure what changed. In response to comment 1 I did temporarily change some settings in Performance (before no caching at all was enabled) and I'm not sure if that cleared things up. I'll do some more testing to make sure that it is indeed consistently cleared up.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Category: bug » support
Priority: Critical » Normal
Status: Postponed (maintainer needs more info) » Fixed

Marking as fixed for now.

salobaas’s picture

salobaas commented
Status: Fixed » Active

Sorry it is not fixed by itself. You can replicate it only when you turn the toggle "Adjust top margin" off. Then you will see that this variable is used but not defined.

Cheers, Fons.

peregrine’s picture

peregrine commented

Same issue exists in 5.x-2.7

mkrakowiak’s picture

mkrakowiak commented

Subscribing for 5.x-2.7.

jackalope’s picture

jackalope commented

Fons is absolutely right. The error went away for me because I'd toggled "Keep menu at top of page" on. If either "Keep menu at top of page" or "Adjust top margin" is toggled on, the error is not present; if neither of those two is toggled, then the error appears again.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: error in admin_menu.js breaks all other javascript in some browsers » Drupal.settings.admin_menu is undefined JS error in some browsers
Category: support » bug
Status: Active » Needs review
StatusFileSize
new admin_menu-DRUPAL-6--1.js-settings-error.patch1.03 KB

Seems like my browser is apparently so cool that it does not bail out due to this error. ;) Please test attached patch.

jackalope’s picture

jackalope commented

Patch in #9 worked for me. No settings toggled in Admin Menu config, no JS errors, all JS on page working properly. Sweet! Thanks for such a quick fix.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Fixed

Thanks, committed to all branches.

babbage’s picture

babbage commented

Yep, can confirm that I was having .js issues in Safari with the 1.2 update too—was about to post an issue—and the 1.3 update has fixed the problem. Thanks.

bago’s picture

bago commented

if (Drupal.settings.admin_menu) is not enough.
if (Drupal && Drupal.settings && Drupal.settings.admin_menu) is the right thing.
This issue should be reopened because if you don't have a Drupal.settings variable (and it is not a core variable) this still raise a javascript error.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Drupal.settings is a JavaScript variable provided by Drupal core.

bago’s picture

bago commented

@sun: if no other module initialize it that variable is uninitialized.
a basic drupal installation with only the admin_menu enabled will show you the javascript error.

1. drupal.js setup Drupal but no settings variable.

2. Drupal.extend in drupal_get_js is the one creating that variable.

3. look at drupal_get_js() in common.inc.
-----
it contains a:
foreach ($javascript as $type => $data) {
if (!$data) continue;
-----
So the Drupal.extend setting up the settings property is only present when a javascript of type 'setting' is present.

4. so, if no one add a js with type = 'setting' (_drupal_add_js with a type = 'setting') you won't have that property.

5. AFAICT a core drupal 5 install does not call _drupal_add_js with a 'setting' type.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

zanoman’s picture

zanoman commented

Just updated from 5.x-2.6 to 5.x-2.8 on Drupal 5.16 and the issue described here appeared for me.

But my FF simply reported:
Error: Drupal.settings is undefined
Source File: http://togethearth.con/modules/admin_menu/admin_menu.js
Line: 5

Safari also reported the error.

I thought it was a core bug and written a bug report in #447350.

I downgraded, everything is fine but I'd really like to be able to update it.

zanoman’s picture

zanoman commented
Status: Closed (fixed) » Active

In #447350, you'll have pictures.

scb’s picture

scb commented

subscribing
same issue here (5.x-2.8)
If the the option "Apply margin-top to page body" in the admin_menu settings page is on, no error is thrown and it works fine.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

@bago: Thanks! That makes perfectly sense - sorry for being ignorant.

So who writes this one line patch to let others test it?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Fixed
StatusFileSize
new admin_menu-DRUPAL-6--1.js-settings.patch1.52 KB

Thanks for reporting, reviewing, and testing! Committed attached patch to 6.x-1.x and 5.x-2.x branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.