Project

General

Profile

Actions

Defect #1591

closed

http links containing parentheses fail to reder correctly

Added by ChunChang (Nagaharu) Lo about 16 years ago. Updated about 16 years ago.

Status:
Closed
Priority:
Normal
Category:
-
Target version:
Start date:
2008-07-07
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed
Affected version:


Files

patch_links_with_parenthesis.patch (5.01 KB) patch_links_with_parenthesis.patch Auto link + parenthesis Pierre Paysant-Le Roux, 2008-07-10 18:31
urls_fix.patch (4.37 KB) urls_fix.patch Paul Rivier, 2008-08-26 12:01

Related issues

Has duplicate Redmine - Defect #1831: Textile bug in forming URL with last ')' symbolClosed2008-08-29

Actions
Actions #1

Updated by Pierre Paysant-Le Roux about 16 years ago

Here is a patch that should solve the problem. The regexp come from Rails with some modifications.

Actions #2

Updated by Jean-Philippe Lang about 16 years ago

Thanks for the patch.
But it would be nice to fix RedCloth#inline_textile_link as well.

Actions #3

Updated by Jean-Philippe Lang about 16 years ago

This patch doesn't work well with links inside parenthesis. Eg. (http://www.foo.bar/Test_foobar).
Trailing parenthesis is included in the url.

Actions #4

Updated by Paul Rivier about 16 years ago

  • Assignee set to Paul Rivier

Jean-Philippe Lang wrote:

This patch doesn't work well with links inside parenthesis. Eg. (http://www.foo.bar/Test_foobar).
Trailing parenthesis is included in the url.

This is not an easy problem. Should we consider that parenthesis inside url should alway be balanced ? I don't think we should.
Then should we treat the particular case of a url with a preceding ( and a trailing ) ?

Actions #5

Updated by Paul Rivier about 16 years ago

Paul Rivier wrote:

This is not an easy problem. Should we consider that parenthesis inside url should alway be balanced ? I don't think we should.
Then should we treat the particular case of a url with a preceding ( and a trailing ) ?

here is a naive patch for the later.

   def inline_auto_link(text)
        text.gsub!(AUTO_LINK_RE) do
          all, a, b, c, d = $&, $1, $2, $3, $4
          if a =~ /<a\s/i || a =~ /![<>=]?/
          else
################## PATCH BELOW
            if ( a[-1] == ?( and c[-1]==?) ) # looks like url is put inside parenthesis
              c=c[0..-2]
              d=")"+d
            end
################## PATCH ABOVE
            text = b + c
            %(#{a}<a class="external" href="#{b=="www."?"http://www.":b}#{c}">#{text}</a>#{d})
          end
        end
      end
Actions #6

Updated by Paul Rivier about 16 years ago

Jean-Philippe Lang wrote:

Thanks for the patch.
But it would be nice to fix RedCloth#inline_textile_link as well.

What's the policy for this kind of "bundled-librairy" related problem ? Should we patch against bundled files ? Should we try to monkey-patch from outside ?

Actions #7

Updated by Jean-Philippe Lang about 16 years ago

Should we consider that parenthesis inside url should alway be balanced ? I don't think we should.

I don't know which is the best solution but I'd really like that something like this works: (see: http://example.net).
I think we see this more often than a link with a single closing parenthesis at the end.
WDYT ?

Actions #8

Updated by Jean-Philippe Lang about 16 years ago

Should we patch against bundled files ?

Yes, you can.
redcloth.rb is already patched: http://www.redmine.org/repositories/changes/redmine/trunk/lib/redcloth.rb

Actions #9

Updated by Paul Rivier about 16 years ago

Jean-Philippe Lang wrote:

I don't know which is the best solution but I'd really like that something like this works: (see: http://example.net).
I think we see this more often than a link with a single closing parenthesis at the end.
WDYT ?

For sure I'd like this as well, but we first have to formulate clearly what we want. For the moment, it's like :

 "If the url ends a sentence in parenthesis, don't include the closing parenthesis in the URL" 

That would involve matching the whole sentence, before url to see if it starts with '(' and after to see if there isn't an other closing ')' later, like in "(see: http://fr.example.net/wiki/clavier_(musique) to read more)". I think we will have hard time doing so :/

Maybe we can do that :

  "If the url last char is closing ')' that is unbalanced in the url itself, take the ')' as external to the url

This seems doable, does not require to scan far around the url itself, and should handle :

WDYT ?

Actions #10

Updated by Jean-Philippe Lang about 16 years ago

"If the url last char is closing ')' that is unbalanced in the url itself, take the ')' as external to the url

This is fine for me.

Actions #11

Updated by Paul Rivier about 16 years ago

Jean-Philippe Lang wrote:

"If the url last char is closing ')' that is unbalanced in the url itself, take the ')' as external to the url

This is fine for me.

so please find attached patch against current trunk. It's fairly minor, and does not change the (already fairly complex) regexp for url except concerning the parenthesis. Tests included.

Actions #12

Updated by Paul Rivier about 16 years ago

jean philippe, are you reasonably happy with this patch ? If not, could you explain how you think it should be improved please.

Actions #13

Updated by Paul Rivier about 16 years ago

  • Status changed from New to 7
  • Assignee changed from Paul Rivier to Jean-Philippe Lang

What's up with this patch please ?
Jean Philippe, you are the new assignee, as you may want to apply it or not, or close the issue.

Actions #14

Updated by Jean-Philippe Lang about 16 years ago

  • Status changed from 7 to Closed
  • Target version set to 0.8
  • Affected version (unused) set to 0.7.3
  • Resolution set to Fixed
  • Affected version set to 0.7.3

Paul, I didn't see your new patch, sorry.
Anyway, everything looks fine and it's now committed in r1871.

Thanks !

Actions

Also available in: Atom PDF