nojs/ajax replacement

Works for me. Committed!

I 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.

Works for me. This simple patch has worked well in CTools and makes just as much sense in 7.

Committed to CVS HEAD. Might want to open a ticket for CTools on D7 -- they can undo that little change. :)

Moving back to CTools 7.x-1.x to see if it needs fixing there. Re-fix if it doesn't need to be.

Crap. Let's fix this non-sense, please.

/nojs/g will replace *all* occurrences of "nojs", even if they are part of another word.

url = url.replace("/nojs/?$", "/ajax");

?

Aaaand patchie.

-  this.url = element_settings.url.replace(/nojs/g, 'ajax');
+  this.url = element_settings.url.replace(/\/nojs\/?$/g, '/ajax');

At first sight, this is removing the / after nojs.

Yep, it is. So what? That's only if the slash is at the end (aka it's meaningless)

/a/nojs/b/c will become /a/nojsb/c, which *is* a problem.

Excuse 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')))

Indeed, 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.

The JS in #13 didn't replace properly in my browser.

this.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')))

Cool, 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.

Whoops, this one instead.

I tested #16 and it worked even with foo/nonojshark/bar and didn't replace as expected.

The 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'));

Drupal 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.

Oh 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.

Wait no we cant, blah. It messes up 'bar/nojshark/foo'. One sec.

Fine, I just removed the line beginning anchor from my last patch. :P

Here, I think the {1} was redundant.

Speaking of redundant... :)

So, Roger's last one would fail on '/a/nojsb/c', so this patch SHOULD be the one.

Meh, how many times can I screw up one patch? Hopefully Damien is counting for me ;)

#27 looks ok :)

It should be '/ajax$1' instead of '/ajax$2'.

#29: 565808_8.patch queued for re-testing.

Yup, you got it! (Apparently I still had one bad patch left in me ;P)

Let's get this in, and fix the mess.

subscribing.

Committed to CVS HEAD. Thanks.

Let's make sure this gets backported to Ctools then.

Patch for CTools 6.x-1.x that uses the same new replacement in all places

Missed one in modal.js

Thanks for the backport! Committed.

I 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.

subscribing

My 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(\/|\?|&|$)/

Tagging...

Just 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

Patch 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)

Changing status...? EDIT - oh thanks Dave, you got in before me :)

+++ misc/ajax.js	25 Aug 2010 16:58:37 -0000
@@ -107,7 +107,7 @@ Drupal.ajax = function (base, element, e
   // Replacing 'nojs' with 'ajax' in the URL allows for an easy method to let
   // the server detect when it needs to degrade gracefully.
-  this.url = element_settings.url.replace(/\/nojs(\/|$)/g, '/ajax$1');
+  this.url = element_settings.url.replace(/\/nojs(\/|$|\?|&)/g, '/ajax$1');

Since 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.

Added explanation in comments before regex to describe the four scenarios being checked:

// There are four scenarios to check for:
// 1. /nojs/
// 2. /nojs$ - The end of a URL string.
// 3. /nojs? - Followed by a query (with cleanurls enabled).
//      eg: path/nojs?destination=foobar
// 4. /nojs& - Followed by a query (without cleanurls enabled).
//      eg: ?q=path/nojs&destination=foobar

Can 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.

The CTools patch could add a single javascript function and call that, so that the regex only exists in one place. Yay abstraction. =)

Removed 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.

changing status

Whoops apparently it pays to review your work *before* you upload... plz ignore last ctools patch

also uploading the right file is important...

The 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?

Made the simple wording changes recommended by Dave Reid. The patches still applied

D6 patch was meant for ctools, bot didn't know

Thanks andrewlevine.
Sorry I was slow to pick up on this, I've been dealing with back to school crazies :P

I think we're missing one other condition with 'mypath/nojs#fragment'. If someone can re-roll I can quickly review today.

Go 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?

No, 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.

Rerolled with support for a fragment.

CTools patch failing testing is not relevant. Back to needs review.

Patch in #65 satisfies all the conditions I think we could encounter. Tested manually with JS and it replaces as desired.

Looks good to me. Committed to CVS HEAD.

Should we move this to CTools now?

Moving to CTools.

Please 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!

Posted the D6 patch in #970536: nojs/ajax replacement fails in various situations, CTools queue.