Posted by jimkeller on August 4, 2011 at 1:24am
10 followers
| Project: | Link |
| Version: | 6.x-2.9 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
URLs such as http://twitter.com/#!/TED_TALKS are considered invalid by the parser. Patch against 6.x-2.9 attached.
| Attachment | Size |
|---|---|
| link_hash_bang.patch | 985 bytes |
Comments
#1
Worked for me, thanks!
Flagging as 'needs review' so a committer can look at it.
#2
I can confirm that the patch submitted in the original post works as described.
#3
since D7 final release has priority in the moment, I only can promise that we will review all helpful D6 patches after D7 release to find its way into next D6 release.
Thanks for all input and effort - much appreciated!
INFO: Drupal 5 issues support dropped from now on. Files stay online for any use.
#4
The provided patch would not apply using
git apply -v link_hash_bang.patchand returned the messageerror: No changes. However, the patch did apply usingpatch -p0 < link_hash_bang.patch. Please re-roll the patch using git. http://drupal.org/node/707484#5
dkingofpa, since you already did a succesful patch, why don't you diff/patch against the release you used and provide the reroll by yourself? It would only be 2 mouseclicks from your point now ...
#6
I can attest that the patch works as described.
#7
Duplicate issue http://drupal.org/node/401138
#8
@jwant: Since you have already closed #401138: Allow exclamation in URL I will contribute here, but in case of duplicate issues, it's common practice to close the newer issue in favor of the older. That way, the initial reporter gets the credits he deserves.
The proposed patch fixes exclamations in the anchor part, but not in the query part. Here is a patch that supports both.
#9
Hi,
Thanks for the info. Close new issues in favor of the older...noted.
Thanks again, jwant.
#10
You know, I'm happier when people provide patches that come with tests. :)
#11
jcfiala, I understand and agree.
I just had a quick look at link.validate.test and it looks like we could easily add some URLs in
function testValidateExternalLinks(). For instance, adding 'http://example.com/?exclamationmark!' and 'http://example.com/#!/username' should be enough to test if exclamation marks are allowed in the query and anchor part. However, that's not all there is to it.My patch merely adds one character to a much longer list of special characters (like
$-_.+*'(),) that are all allowed (according to http://tools.ietf.org/html/rfc1738). Most of those characters are not covered by tests at the moment. We would be fooling ourselves if we just add tests for exclamation marks in queries and anchors and consider it done.That said, adding tests for all special characters seems beyond the scope of this issue. I suggest that we get the fix from #8 committed and open a new issue for tests for all special characters.
#12
Hmm.. patch didn't work.
output was:
Checking patch link.module...
error: while searching for:
$directories = "(?:\/[a-z0-9". $LINK_ICHARS ."_\-\.~+%=&,$'#!():;*@\[\]]*)*";
// Yes, four backslashes == a single backslash.
$query = "(?:\/?\?([?a-z0-9". $LINK_ICHARS ."+_|\-\.~\/\\\\%=&,$'():;*@\[\]{} ]*))";
$anchor = "(?:#[a-z0-9". $LINK_ICHARS ."_\-\.~+%=&,$'():;*@\[\]\/\?]*)";
// The rest of the path for a standard URL.
$end = $directories .'?'. $query .'?'. $anchor .'?'.'$/i';
error: patch failed: link.module:991
error: link.module: patch does not apply
#13
Forgot to change the status.
#14
I rolled my patch against the 8.x-1.x branch; against which branch did you try to apply it?
#15
@jcfiala, that's what I thought by looking on the code of this patch ... it can't apply.
#16
Here's a patch for 6.x-2.x-dev which has moved the code from link.module into link.inc
#17
And here's one where I've added a hash bang url to the valid examples test file, though I have no idea how testing works in drupal/php.