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.
line 40 of ajax-responder.js states
url = url.replace("/nojs/", "/ajax/");
However, if I've setup a url where nojs is at the end (i.e. myurl/nojs) then it is not picked up by .replace() and is not changed.
I propose the following change:
url = url.replace(/nojs/g, "ajax");
This would make 'nojs' replaced globally in the url with 'ajax'. There may be edge cases where this is not desirable, but since it is not case insensitive and does handle the case where we have /nojs, I think it's okay. Can anyone refute this? Is there something I've missed?
Comments
Comment #1
merlinofchaos CreditAttribution: merlinofchaos commentedWorks for me. Committed!
Comment #3
Dave ReidI had problems that paths like 'foo/bar/nojs' didn't work on D7 (had to use 'foo/bar/nojs/' with explicit trailing slash) so this needs to be up-ported as well. Tested and works great.
Comment #4
zroger CreditAttribution: zroger commentedWorks for me. This simple patch has worked well in CTools and makes just as much sense in 7.
Comment #5
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Might want to open a ticket for CTools on D7 -- they can undo that little change. :)
Comment #6
Dave ReidMoving back to CTools 7.x-1.x to see if it needs fixing there. Re-fix if it doesn't need to be.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedCrap. Let's fix this non-sense, please.
/nojs/g
will replace *all* occurrences of "nojs", even if they are part of another word.Comment #8
cha0s CreditAttribution: cha0s commentedurl = url.replace("/nojs/?$", "/ajax");
?
Comment #9
cha0s CreditAttribution: cha0s commentedAaaand patchie.
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedAt first sight, this is removing the / after nojs.
Comment #11
cha0s CreditAttribution: cha0s commentedYep, it is. So what? That's only if the slash is at the end (aka it's meaningless)
Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commented/a/nojs/b/c
will become/a/nojsb/c
, which *is* a problem.Comment #13
cha0s CreditAttribution: cha0s commentedExcuse me? Test the patch. Thanks in advance.
Or alternatively, run this code in your browser: javascript:(alert('/a/nojs/b/c'.replace(/\/nojs\/?$/g, '/ajax')))
Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedIndeed, I read that wrong. But this regexp is still wrong: we want to replace every part of the path containing "nojs", whether it is in the end or in the middle of the path.
Comment #15
Dave ReidThe JS in #13 didn't replace properly in my browser.
Comment #16
zroger CreditAttribution: zroger commentedthis.url = element_settings.url.replace(/\/nojs(\/?)/g, '/ajax$1');
This matches /nojs with an optional trailing slash. The optional trailing slash is captured and re-used in the replacement.
Test with nojs in the middle of a url:
javascript:(alert('/a/nojs/b/c'.replace(/\/nojs(\/?)/g, '/ajax$1')))
Test with nojs at the end of the url, no trailing slash:
javascript:(alert('/a/nojs'.replace(/\/nojs(\/?)/g, '/ajax$1')))
Comment #17
cha0s CreditAttribution: cha0s commentedCool, thank you Roger López. :) For some reason I had assumed that nojs was only occuring at the end of the path. If it can occur everywhere, let's check for the leading slash as well, in the case of paths 'nojs/foo/bar', for completion.
Comment #18
cha0s CreditAttribution: cha0s commentedWhoops, this one instead.
Comment #19
Dave ReidI tested #16 and it worked even with foo/nonojshark/bar and didn't replace as expected.
Comment #20
cha0s CreditAttribution: cha0s commentedThe patch I posted works on that string too, there is a problem however, that isn't addressed in either patch.
My latest patch will fail on 'nojshark/bar', and Roger's will faill on 'nojs/foo/bar'. I added the anchor to the beginning of the regex in my latest patch, but it isn't enough.
So, I see what's happening here, the '$' anchor was removed from my regex without me noticing in the last couple of patches. This has been put back in.
I'm providing test expressions, though I don't think we can simpletest JS. Please run these trough firebug's console before commenting about how effective the regex is...
alert('nojs/foo'.replace(/(\/|^)nojs(\/|$){1}/g, '$1ajax$2'));
alert('nojshark/foo'.replace(/(\/|^)nojs(\/|$){1}/g, '$1ajax$2'));
alert('nonojshark/foo'.replace(/(\/|^)nojs(\/|$){1}/g, '$1ajax$2'));
alert('bar/nojs/foo'.replace(/(\/|^)nojs(\/|$){1}/g, '$1ajax$2'));
alert('bar/nonojshark/foo'.replace(/(\/|^)nojs(\/|$){1}/g, '$1ajax$2'));
alert('bar/nojs'.replace(/(\/|^)nojs(\/|$){1}/g, '$1ajax$2'));
alert('bar/nojs/'.replace(/(\/|^)nojs(\/|$){1}/g, '$1ajax$2'));
alert('bar/nonojs'.replace(/(\/|^)nojs(\/|$){1}/g, '$1ajax$2'));
Comment #21
merlinofchaos CreditAttribution: merlinofchaos commentedDrupal cannot handle wildcards in the first position of a URL, so missing ^nojs does not matter in the slightest. We should consider that an invalid place to put your ajax/nojs anyway.
Comment #22
cha0s CreditAttribution: cha0s commentedOh ok... if we don't have to handle that case (I was trying to be safe rather than sorry ;) then we cn just use Roger's patch straight up. I created a patch for that one.
Comment #23
cha0s CreditAttribution: cha0s commentedWait no we cant, blah. It messes up 'bar/nojshark/foo'. One sec.
Comment #24
cha0s CreditAttribution: cha0s commentedFine, I just removed the line beginning anchor from my last patch. :P
Comment #25
cha0s CreditAttribution: cha0s commentedHere, I think the {1} was redundant.
Comment #26
cha0s CreditAttribution: cha0s commentedSpeaking of redundant... :)
So, Roger's last one would fail on '/a/nojsb/c', so this patch SHOULD be the one.
Comment #27
cha0s CreditAttribution: cha0s commentedMeh, how many times can I screw up one patch? Hopefully Damien is counting for me ;)
Comment #28
Damien Tournoud CreditAttribution: Damien Tournoud commented#27 looks ok :)
Comment #29
Alexander N CreditAttribution: Alexander N commentedIt should be '/ajax$1' instead of '/ajax$2'.
Comment #31
scor CreditAttribution: scor commented#29: 565808_8.patch queued for re-testing.
Comment #32
cha0s CreditAttribution: cha0s commentedYup, you got it! (Apparently I still had one bad patch left in me ;P)
Comment #33
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's get this in, and fix the mess.
Comment #34
rfaysubscribing.
Comment #35
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #36
Dave ReidLet's make sure this gets backported to Ctools then.
Comment #37
Dave ReidPatch for CTools 6.x-1.x that uses the same new replacement in all places
Comment #38
Dave ReidMissed one in modal.js
Comment #39
merlinofchaos CreditAttribution: merlinofchaos commentedThanks for the backport! Committed.
Comment #41
Dave ReidI need to re-open this because urls like path/nojs?destination=foobar do not get replaced properly (with both Ctools and D7 core). I'm going to also bet if you have clean URLs disabled the similar ?q=path/nojs&destination=foobar will not get processed as well.
Comment #42
rfaysubscribing
Comment #43
Dave ReidMy initial thought is to use
/nojs\b/
which will match on word boundries and we don't even have to use $1 for replacement. The only downside is that it matches paths like /foo/nojs-bar, but otherwise it works for *all* possible cases, including query string and query string without clean URLs. Otherwise we'll just have to use/nojs(\/|\?|&|$)/
Comment #44
Dave ReidTagging...
Comment #45
tnightingale CreditAttribution: tnightingale commentedJust ran into a similar issue to Dave Reid, wanting to use a URL ending in /nojs?destination...
Are there any cases not covered by
/nojs(\/|\?|&|$)/
?Seems pretty reasonable to me
Comment #46
tnightingale CreditAttribution: tnightingale commentedPatch for CTools 6.x-1.x using /nojs(\/|$\?|&)/g
Throwing in a patch for D7 too though I suspect it is too late for D7 patches eh? (I'm kind of new to all this :P)
Comment #47
Dave ReidComment #48
tnightingale CreditAttribution: tnightingale commentedChanging status...? EDIT - oh thanks Dave, you got in before me :)
Comment #49
sunSince we are not able to test JS yet, and even if we could, we need to explain the non-trivial parts in comments.
- /nojs? made me think.
- /nojs& made me go nuts, until I noticed the term "destination" somewhere skimming this issue.
Powered by Dreditor.
Comment #50
tnightingale CreditAttribution: tnightingale commentedAdded explanation in comments before regex to describe the four scenarios being checked:
Comment #51
Dave ReidCan we remove the empty comment line (
//
) and just make that an empty line (up for a quick re-roll thegreat)? Otherwise this looks good to me. Side note, the ctools patch has to fix this in 4 different places. The D7 patch only has to fix it in one place though.Comment #52
merlinofchaos CreditAttribution: merlinofchaos commentedThe CTools patch could add a single javascript function and call that, so that the regex only exists in one place. Yay abstraction. =)
Comment #53
tnightingale CreditAttribution: tnightingale commentedRemoved empty comment and rerolled.
Created a function (urlReplaceNojs... not feeling very imaginative today :P) for ctools and replaced the replace() calls in all four places.
Comment #54
tnightingale CreditAttribution: tnightingale commentedchanging status
Comment #55
tnightingale CreditAttribution: tnightingale commentedWhoops apparently it pays to review your work *before* you upload... plz ignore last ctools patch
Comment #56
tnightingale CreditAttribution: tnightingale commentedalso uploading the right file is important...
Comment #57
Dave ReidThe comments will need to be cleaned up in #54. We use 'clean URLs' and not 'cleanurls'. Should use 'E.g.' and not 'eg'. thegreat, up for a re-roll?
Comment #58
andrewlevine CreditAttribution: andrewlevine commentedMade the simple wording changes recommended by Dave Reid. The patches still applied
Comment #60
andrewlevine CreditAttribution: andrewlevine commentedD6 patch was meant for ctools, bot didn't know
Comment #61
tnightingale CreditAttribution: tnightingale commentedThanks andrewlevine.
Sorry I was slow to pick up on this, I've been dealing with back to school crazies :P
Comment #62
Dave ReidI think we're missing one other condition with 'mypath/nojs#fragment'. If someone can re-roll I can quickly review today.
Comment #63
merlinofchaos CreditAttribution: merlinofchaos commentedGo ahead, though we need to make sure the fragments are even visible. Those aren't transmitted to the server, usually, so I don't think they'll be seen usually?
Comment #64
Dave ReidNo, they're not transmitted to the server, but we need to make sure they're still included in links in a page, which is when this is executed.
Comment #65
carlos8f CreditAttribution: carlos8f commentedRerolled with support for a fragment.
Comment #67
merlinofchaos CreditAttribution: merlinofchaos commentedCTools patch failing testing is not relevant. Back to needs review.
Comment #68
Dave ReidPatch in #65 satisfies all the conditions I think we could encounter. Tested manually with JS and it replaces as desired.
Comment #69
Dries CreditAttribution: Dries commentedLooks good to me. Committed to CVS HEAD.
Should we move this to CTools now?
Comment #70
carlos8f CreditAttribution: carlos8f commentedMoving to CTools.
Comment #71
rfayPlease create a new issue in CTools and link it here. It makes our history too difficult to follow when an issue that was committed to core suddenly disappears into another queue. Thanks!
Comment #72
carlos8f CreditAttribution: carlos8f commentedPosted the D6 patch in #970536: nojs/ajax replacement fails in various situations, CTools queue.