Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I wanted to override just a part of extlink.js, and found I had to override the whole thing. So I figured some refactoring was in order. Patch forthcoming.
Comment | File | Size | Author |
---|---|---|---|
#23 | extlink-1329786-23.patch | 8.93 KB | elachlan |
#21 | extlink-1329786-21.patch | 8.45 KB | elachlan |
#18 | extlink-1329786-18.patch | 7.78 KB | elachlan |
#15 | extlink-1329786-15.patch | 7.31 KB | elachlan |
#13 | extlink-1329786-13.patch | 7.52 KB | elachlan |
Comments
Comment #1
ksenzeeThis patch should apply against the master branch. I haven't changed any actual functionality - just moved things around. Here's what I did:
- Replaced Drupal.settings with the settings parameter from Drupal.attachBehaviors. This is technically more correct for D7, although I doubt it makes any functional difference. However, it does have the advantage of being faster; accessing a.b.c is slower than accessing b.c.
- Replaced jQuery .each() with for loops. It turns out that function-based iteration is a lot slower than traditional loop iteration. (If we wanted to get even faster, we could use a do-while and decrement a counter, but I find that harder to read.)
- Factored out the one-liners that add the extlink and mailto classes into a separate function. There was a lot going on in that one line, and a lot of it was duplicate code that it made sense to consolidate.
- Moved the extlinkAttach function into the Drupal.extlink namespace, so people who want to override the whole thing don't have to add to the global namespace. I did leave in a check for whether the extlinkAttach function exists, for the benefit of people who were already overriding it.
Comment #2
quicksketchThis patch looks like a good improvement. I don't think the backwards compatible function is really needed though, because extlinkAttach() is already locally scoped inside of the
(function ($) {
wrapper at the top of the file.Comment #3
ksenzeeRight, the original function is file scoped, but anyone who needs to override it has to put *their* extlinkAttach function in the global scope in order for Drupal.behaviors.extlink.attach to be able to see it. That's what I was thinking it would be nice to avoid.
Comment #4
RobLoachThis is a huge improvement! Looking at the current extlink.js makes me vomit a bit.
We're on Drupal 7, let's get rid of all the old jQuery version checks.
Would be nice to not need that try/catch. Is it just the url.match calls that throw it on IE7?
Probably don't need that version check.
$.each($links_to_process, function(index, link) {
});
Comment #5
ksenzeeI'm going to disagree with you about jQuery.each. See my second point in #1. We ought to be avoiding it anywhere we can in performance-sensitive code.
Comment #6
mstef CreditAttribution: mstef commentedEdit: ignore this patch
Comment #7
mstef CreditAttribution: mstef commentedSlight reroll of #1 to properly apply with drush make.
Comment #8
elachlan CreditAttribution: elachlan commentedThis is a real worthwhile change and should really be done before #2038235: Add minified version of extlink.js.
I will work on it soon, unless someone wants to re-roll a patch.
Comment #9
elachlan CreditAttribution: elachlan commentedCan I get some input on whether we want to go with the for loop instead of each?
#5 - Do you know how much time it will save doing it this way?
Comment #10
ksenzeeI happen to hate jQuery.each with a hot hot hate, and I have never understood the widespread reluctance to use a simple venerable for loop, but as it happens modern browsers have built in each handling so jQuery.each isn't nearly as slow for them. I wouldn't hold up the patch for it either way. The refactoring is more important.
Comment #11
elachlan CreditAttribution: elachlan commented#10 - if you re-roll it without the for loop I will include it. There have been a few changes to the javascript in the master branch, so be sure to check that first.
We can then add it later pending input from other maintainers.
Comment #12
elachlan CreditAttribution: elachlan commentedSetting the priority to Major. I will re-roll this patch soon, unless someone does it first.
Comment #13
elachlan CreditAttribution: elachlan commentedRe-worked the patch. Uses jQuery.each
I am happy to move away from it if quicksketch agrees to it. He knows more than me about it.
Comment #15
elachlan CreditAttribution: elachlan commentedSorry, bad patch format.
Comment #16
elachlan CreditAttribution: elachlan commentedComment #18
elachlan CreditAttribution: elachlan commentedWhite space issues.
Comment #19
elachlan CreditAttribution: elachlan commentedComment #21
elachlan CreditAttribution: elachlan commentedComment #23
elachlan CreditAttribution: elachlan commentedI think it is failing because it's applying to 7.x-1.13 and not the master branch.
Trying again anyway.
Comment #25
Dave ReidYou should create a 7.x-1.x-dev release for this project so that you can properly assign versions to things in development here in the issue queue.
Comment #26
elachlan CreditAttribution: elachlan commentedAdded the new branch for 7.x-1.x and dev release.
Comment #27
elachlan CreditAttribution: elachlan commented#23: extlink-1329786-23.patch queued for re-testing.
Comment #29
elachlan CreditAttribution: elachlan commented#23: extlink-1329786-23.patch queued for re-testing.
Comment #30
elachlan CreditAttribution: elachlan commentedCommited to Git. Needs port to 8.x
Comment #31
elachlan CreditAttribution: elachlan commentedBy having the backwards compatibility section we add 0.05% to the page execution time.
This doesn't sound like much, but it is 45% of the execution time for external links. Is there a better way we can do it?
Comment #32
DamienMcKennaMoving to the correct branch.
Comment #33
elachlan CreditAttribution: elachlan commentedIt looks like this was already done as apart of the conversion.
Comment #34
acrosmanAgreed, it looks like this patch is already in the 8.x branch.
Comment #35
rootwork