show any ajax request errors to the user in the table

[camallen]

The ajaxError event is testing the url matches the stored settings url using the String.match function which expects a regular expression as it's parameter. Instead use the equality operator to test the strings match, thus allowing the ajax error response message to be shown on the table.

[TheSin-]

does === cover it or should the match just be fixed to be a regex? I'm only asking cause I haven't run into this yet, and I'd like to test/understand it. though I do agree using match on a string won't work, I'm just not convinced === is right either here. Does either url use the protocol? are they url or uri? etc etc

[camallen]

Hey @TheSin- ,

The url and settings.url instances are just Strings.

The code is just checking that the url String is the same, i.e. request matches the expected one in settings. Which, they should be as they are the same string copied around. So i just did a === comparison instead of trying the regex way as it seemed like overkill.

[TheSin-]

right I assumed just strings, but could url be http://example.com/ and settings.url be example.com or one a url and the other a uri? Is there a point where === wouldn't work and something like /url/ would be preferable? Again I haven't been in that part of the code yet, I might try and setup a test on this today if monday would leave me alone for a bit ;) Chances are I'll merge your changes today but I just want to make sure.

[camallen]

My understanding is that the settings.url of the ajax request is derived from the url method of the pager instance. So they should not differ as on each ajax request the ajaxError.pager event is setup with the current url and the ajax instance url as well so they shouldn't differ.

I left the check in there as i wasn't 100% on the edge cases.

You can test it easily by just getting your server to return a fail header and the ajax error event should run.

[TheSin-]

In all my tests they were identical, and they are the uri not the url, but they do not include the protocol unless it's actually specified in the ajaxUrl.

This seems like a good find and fix, I'm going to accept it.

[Mottie]

I'm probably going to have to revert this change as the settings.url won't exactly equal the url if there are extra parameters at the end. So, using this ajaxUrl:

ajaxUrl : 'fake{page}.json#{filterList:filter}&{sortList:column}',

and a log of those two values, you'll see:

url = fake0.json#filter&column
settings.url = fake0.json

so a match might be the best solution. I'm open to any feedback. :)

[TheSin-]

this was my concern and I'm glad you caught it Rob.

[camallen]

Hi,

I can't reproduce the same results. I get the same value for url and settings.url when my server returns an error header on an ajax page request.

I haven't traced the execution of this for a while but my original understanding was the url was set on the ajaxObject prior to the remote request see lines 291 - 314:

// line 306
c.ajaxObject.url = url;
// line 314
$.ajax(c.ajaxObject);

Thus they "should" always be the same in the ajaxError.pager function.

Perhaps I am missing something..

[Mottie]

I get it when I test it locally (in Firefox; Chrome doesn't work locally).

I take this pager ajax demo: http://mottie.github.io/tablesorter/docs/example-pager-ajax.html

change the ajaxUrl to 'assets/x{page}.json#{filterList:filter}&{sortList:column}'

and add this within the error event:

$doc.bind('ajaxError.pager', function(e, xhr, settings, exception) {
    console.log(url, settings.url);

the log shows:

assets/x0.json#filter&column assets/x0.json

and the table is showing the error row with "Not connected, verify Network" - after I changed it to match the urls.

[camallen]

Pretty sure you can't use the '#' char for query params delimiting as the '#' char refers to the fragment part of a URL and the '?' char delimits the query part (http://dev.w3.org/html5/spec-LC/urls.html).

In my tests using the '?' char instead of the '#' i got the same URL in the error function. E.g.:

'assets/x{page}.json#{filterList:filter}&{sortList:column}'
should become
'assets/x{page}.json?{filterList:filter}&{sortList:column}'

Perhaps the query params are being stripped by the JQuery ajax command and thus the settings.url is different to the derived URL from the getAjaxUrl(table, c) function?

[Mottie]

You're right, I should be using the ? to delimit search parameters. But the fact is still that the URLs won't exactly match if it contains a hash portion =(

[TheSin-]

And I do agree with that, there is a chance that # is used since it's not illegal char, it means to load the page at that tag, so it is possible which means the compare would fail in some rare cases, it's best to find a way so all cases work. That being said it will be a rare case I don't think we need to revert the change, I think this change passes more then the old way, thus isn't still an improvement, though we should add a comment about the potential fail and work on a 100% fix for it in the future. At least that is my opinion.

[camallen]

Hi @Mottie & @TheSin-,

I agree that the '#' char can be a valid URL component for ajax requests but it's poor practice (as the original intention was to signify the fragment in the document).

But who am I to tell people what to have in their URL's! Instead how about an explicit encode of any '#' char in the URL returned from c.ajaxUrl on Line 319.

Perhaps:

var url = (c.ajaxUrl) ? c.ajaxUrl
  // allow using "{page+1}" in the url string to switch to a non-zero based index
  .replace(/\{page([\-+]\d+)?\}/, function(s,n){ return c.page + (n ? parseInt(n, 10) : 0); })
  .replace(/\{size\}/g, c.size) : '',

Becomes:

var url = (c.ajaxUrl) ? c.ajaxUrl
  // allow using "{page+1}" in the url string to switch to a non-zero based index
  .replace(/\{page([\-+]\d+)?\}/, function(s,n){ return c.page + (n ? parseInt(n, 10) : 0); })
  .replace(/\{size\}/g, c.size)
  // manually encode any '#' char with the encoded value(%23), http://www.w3schools.com/tags/ref_urlencode.asp
  .replace(/#/g, "%23") : '',

In some quick manual tests this seems to allow the settings.url to be the same as the url derived from the getAjaxUrl(table, c) with the encoded '#' symbol.

Thoughts?

[TheSin-]

I completely agree it's poor practice, but as I have learn from coding for the company I work for currently, you must always code for the lowest common denominator if you want to have full support. In my case the bar is set very very low ;)

I'like to say that using a percent code (or urlencoding) wouldn't be a good solution but since I still dont' know the full scope of those vars it's hard for me to comment. But at least now we have options that we can test with. The real question is... is that the only char that could cause this issue?

[camallen]

How about this (http://dev.w3.org/html5/spec-LC/urls.html#url-manipulation-and-creation):

To fragment-escape a string input, a user agent must run the following steps:
Let input be the string to be escaped.
Let position point at the first character of input.
Let output be an empty string.
Loop: If position is past the end of input, then jump to the step labeled end.
If the character in input pointed to by position is in the range U+0000 to U+0020 or is one of the following characters:
U+0022 QUOTATION MARK character (")
U+0023 NUMBER SIGN character (#)
U+0025 PERCENT SIGN character (%)
U+003C LESS-THAN SIGN character (<)
U+003E GREATER-THAN SIGN character (>)
U+005B LEFT SQUARE BRACKET character ([)
U+005C REVERSE SOLIDUS character ()
U+005D RIGHT SQUARE BRACKET character (])
U+005E CIRCUMFLEX ACCENT character (^)
U+007B LEFT CURLY BRACKET character ({)
U+007C VERTICAL LINE character (|)
U+007D RIGHT CURLY BRACKET character (})
...then append the percent-encoded form of the character to output. [RFC3986]

Otherwise, append the character itself to output.
This escapes any ASCII characters that are not valid in the URI production without being escaped.
Advance position to the next character in input.
Return to the step labeled loop.
End: Return output

Also this:

If data for a URI component would conflict with a reserved
character's purpose as a delimiter, then the conflicting data must be
percent-encoded before the URI is formed.

From, http://tools.ietf.org/html/rfc3986#section-2.2

[TheSin-]

what about a regex instead? something to just make sure the start of c.ajaxObject.url = url so like /^c.ajaxObject.url/, then it doesn't matter what the end of the url is, even if it has hash tags on the end. etc etc.

[Mottie]

The match I had in there before worked fine. Is it really a problem to keep it as a match?

[camallen]

Sorry @Mottie, the original match didn't work!

Please note: the original intention of my pull request is to display error notifications on failing ajax requests.

As per my initial comments on the pull request, using the url.match(settings.url) expects a regex as the only parameter to the String.match function. So if you just pass the settings.url (string type) then this test will never pass, the error will just silently fail and no indicator will ever be displayed.

Originally when looking at the code, I questioned why there is a URL match in the error object but decided to leave it alone and just make the code work. Now looking again (noting the many issues with the match function and valid URL's, etc), I dont' think you need to double check the URL if the Ajax error event is fired. This event should only fire if the ajax request fails and the URL should be the one loaded here, $.ajax(c.ajaxObject);. The only way i can see you would need to check the URL match would be multiple bindings of the event. However on any return (success / error) of the ajax request you unbind the error so you 'should' never have multiple bindings of the error function.

Accordingly, I think the URL check is unnecessary defensive programming. If the ajax event fails then just show an error (as stated above, the attempted request URL should be the one that was loaded, minus invalid url chars, etc).

Perhaps I am not fully understanding the relevance of the URL match defence before showing the error on the table.

[Mottie]

LOL I was having trouble remembering why I did that too... then I found out it was written by someone else - see pull request #183. I guess it wouldn't hurt to remove it ;)