Error was bugging me - here is my fix (last line of code block).

extlink.js

  // Find all links which are NOT internal and begin with http (as opposed
  // to ftp://, javascript:, etc. other kinds of links.
  // When operating on the 'this' variable, the host has been appended to
  // all links by the browser, even local ones.
  // In jQuery 1.1 and higher, we'd use a filter method here, but it is not
  // available in jQuery 1.0 (Drupal 5 default).
  var external_links = new Array();
  var mailto_links = new Array();
  $("a", context).not("." + Drupal.settings.extlink.extClass + ", ." + Drupal.settings.extlink.mailtoClass).each(function(el) {
CommentFileSizeAuthor
#14 extlink-issue822706.patch748 bytesiva2k
#6 extlink_warning.jpg66.31 KBiva2k
#4 missing.jpg143.04 KBWillHall
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

External Links isn't throwing any JS errors for me, are you actually getting an error of some sort?

WillHall’s picture

Firefox 3.6.3

Check "Warnings" Tab

quicksketch’s picture

Status: Active » Postponed (maintainer needs more info)

I still don't get any errors. Here's the original line as it is in code right now. I also don't see any missing closing parenthesis:

$("a:not(." + Drupal.settings.extlink.extClass + ", ." + Drupal.settings.extlink.mailtoClass + ")", context).each(function(el) {

Of course down below there is the closing }) for the each() statement.

WillHall’s picture

FileSize
143.04 KB

Not that this will help you reproduce the problem but I have attached a screenie.

The :not pseudo selector seems to have a problem with trailing values on my end - switching it up to the above solved the problem on multiple sites. The module works regardless - it was just cluttering up my error console with the warning.

:shrug: Perhaps its environmental.

quicksketch’s picture

Status: Postponed (maintainer needs more info) » Closed (fixed)

Closing after lack of other reports or ability to reproduce. Please reopen if this problem persists or is reproduceable on a fresh install.

iva2k’s picture

Status: Closed (fixed) » Active
FileSize
66.31 KB

I confirm seeing this problem in WinXP Firefox 3.6.12 (see attached screenshot). It can be part of jQuery version dependency. I'm using jQuery v1.3.2 (via jQuaery update module).

WillHall's fix removes the warning.

May I suggest just including the proposed fix into CVS, instead of outright dismissal based on inability to reproduce? Proposed code is clear and raises no questions.

quicksketch’s picture

iva2k: I'm unable to reproduce, find the problem, or figure out what the suggested solution is here. If you can provide a patch I'd be happy to check the differences, but right now I don't even know what's being changed.

WillHall’s picture

The actual fix is easier than I originally provided.

$("a:not(." + Drupal.settings.extlink.extClass + ", ." + Drupal.settings.extlink.mailtoClass + ")" + , context).each(function(el) {

the change is

" + , context).each

We need to tack on the the var after we leave the "

HTH

quicksketch’s picture

the change is

" + , context).each

We need to tack on the the var after we leave the "

This doesn't look right, the + is just floating out there, not concatenating any strings. Once again, I think we could avoid confusion here by providing a patch: http://drupal.org/patch/create.

iva2k’s picture

I'll roll a patch. Just give me some time.

quicksketch’s picture

Status: Active » Postponed (maintainer needs more info)

Still at a loss here. The next version (1.12) I'm rolling tonight but I still don't know what the problem is. Moving back to postponed until I can get a patch or a better explanation.

quicksketch’s picture

Status: Postponed (maintainer needs more info) » Closed (cannot reproduce)

Closing after lack of response. I'll assume this is fixed in 1.12.

andrewsuth’s picture

Same issue here. Using jquery_update (jQuery 1.3) causes the warning as outlined above.

Solution #8 fixed the warning with jquery_update but causes an error when not using the module. The solution given in #1 works for both.

iva2k’s picture

Status: Closed (cannot reproduce) » Reviewed & tested by the community
FileSize
748 bytes

Oops, I promised a patch a while back... got swamped by other stuff. Here it is. It is based on #1 (actually #0), which #13 confirms works (it works on my live sites as well).

quicksketch’s picture

Hm, still looks like this patch is unnecessary. Can anyone explain what's wrong with the current code?

iva2k’s picture

in #7 you asked for the patch - I posted it here.

#0, #4, #6, #13 - all saw the JS warnings. It looks like an elusive problem - I tried to reproduce it today, but was unable to get to the same warnings. I should mention that my FireFox got updated awhile back, but jQuery is still the same.

What worries me, is that #13 is very recent. If this intermittent problem is still out there, why not just commit the proposed fix? When I tested it before, the problem was always there, and it was gone for good with the proposed fix that I rolled into the patch. My worry is that error message will keep popping up. It will eat more of everybody's time than just making the dive for the code change. If code is functionally identical, what stops you from using it?

quicksketch’s picture

To me this is mostly a matter of just trying to figure out what's wrong with the existing code. The new code definitely looks just as correct as the old code, I just can't tell why there should be any difference. Understanding what the problem is will help me understand and avoid similar problems in the future. Right now this just looks like swapping two identical lines, which has me questioning how it could fix a problem that I've personally never seen.

iva2k’s picture

> ...a problem that I've personally never seen
I understand your desire to reproduce it if you want to debug it, but how much time are you willing to spend on it? There are couple of screenshots in the above posts to "see". I consider this problem so minuscule that I would not waste my time on debugging FF+jQ around it. There are better problems waiting solution and I better use my time there. The power of community effort is such that I find often that is better to rely on other people when I had problems in my modules that I was not able to reproduce. I already spent more time on this one than what it deserves, and think you did too. Do as you desire, but I'll keep the patch in my installations, and will swear each time I will need to re-apply it after any other updates.

elachlan’s picture

Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

Is this still an issue?

Does it affect newer versions (7.x, 8.x)?

elachlan’s picture

Issue summary: View changes
Status: Postponed (maintainer needs more info) » Closed (won't fix)