Page MenuHomePhabricator

Full Phabricator links are being expanded weirdly in gerrit
Closed, ResolvedPublic

Tokens
"Like" token, awarded by Krenair."Like" token, awarded by Aklapper."Grey Medal" token, awarded by Dzahn."Orange Medal" token, awarded by Krinkle."Like" token, awarded by polybuildr."Like" token, awarded by Yurik.
Assigned To
Authored By
MZMcBride, Nov 26 2014

Description

https://gerrit.wikimedia.org/r/#/c/175938/

If you put a link in the form of https://phabricator.wikimedia.org/T12345 it gets expanded as

<p>( Just testing. <a href="https://phabricator.wikimedia.org/&lt;a href=" https:="" phabricator.wikimedia.org="" t12345"="">T12345</a>" target="_blank"&gt;https://phabricator.wikimedia.org/<a href="https://phabricator.wikimedia.org/T12345">T12345</a> )</p>

Or something similarly silly. Bad regex, I imagine.


See also

Details

Related Gerrit Patches:

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Thanks man, much appreciated

Change 226234 had a related patch set uploaded (by MZMcBride):
Don't match Phabricator task IDs inside URLs

https://gerrit.wikimedia.org/r/226234

@chasemp: Could you backport https://gerrit-review.googlesource.com/#/c/68377/ to our Gerrit installation?

From my point of view, it's not worth it.
Although the change itself is simple, there have been quite some changes between WMF version of its-base (the relevant project) and upstream.
WMF is talking since some time about upgrading gerrit.
If gerrit got upgraded, one could bump the plugin for free ... and the gerrit upgrade would solve many other issues too.

@chasemp: Could you backport https://gerrit-review.googlesource.com/#/c/68377/ to our Gerrit installation?

From my point of view, it's not worth it.
Although the change itself is simple, there have been quite some changes between WMF version of its-base (the relevant project) and upstream.
WMF is talking since some time about upgrading gerrit.
If gerrit got upgraded, one could bump the plugin for free ... and the gerrit upgrade would solve many other issues too.

I have really no insight in gerrit in that way @demon or @greg would make this call

Change 256663 had a related patch set uploaded (by Thiemo Mättig (WMDE)):
Avoid breaking full phabricator URLs

https://gerrit.wikimedia.org/r/256663

Change 226234 abandoned by Ricordisamoa:
Don't match Phabricator task IDs inside URLs

Reason:
:(

https://gerrit.wikimedia.org/r/226234

Dzahn removed a subscriber: Dzahn.Dec 5 2015, 12:47 AM

I tried to link to T119209#1912745 from https://gerrit.wikimedia.org/r/#/c/254440/ and caused weirdness in the escaping code. Basically, I tried to work around the lack of fix for T76459: Allow links to phabricator comments from gerrit messages. and failed.

mmodell added a subscriber: mmodell.Jan 2 2016, 6:18 PM

The way gerrit matches links is very limited, there is no straightforward way to fix it.

scfc added a subscriber: scfc.Jan 2 2016, 7:06 PM

@mmodell: If you think that Thiemo's patch does not resolve this task, please explain in the review.

demon moved this task from Bugs & stuff to Local hacks on the Gerrit board.Jul 25 2016, 5:32 PM

Now that Gerrit has been upgraded to version 2.12.2, can we fix this task?

@MZMcBride hi, yes I'm working on a fix and I think I found a good fix. I'm uploading a patch now.

It is based on @thiemowmde suggestion here https://gerrit.wikimedia.org/r/#/c/242237/2/modules/gerrit/templates/gerrit.config.erb

and worked reliably and didn't break anything from my testing.

Testing on http://gerrit-test.wmflabs.org/

Change 302129 had a related patch set uploaded (by Paladox):
Gerrit: Avoid breaking full phabricator URLs

https://gerrit.wikimedia.org/r/302129

Change 302129 merged by Dzahn:
Gerrit: Avoid breaking full phabricator URLs

https://gerrit.wikimedia.org/r/302129

Krinkle removed a subscriber: Krinkle.Aug 2 2016, 10:01 PM

This should now be partially fixed. The only way it breaks now is if you link to a comment which I have a patch for/

Change 302229 had a related patch set uploaded (by Paladox):
Gerrit: Support having phab commits as links

https://gerrit.wikimedia.org/r/302229

Change 302229 merged by Dzahn:
Gerrit: Support linking to phabricator comments

https://gerrit.wikimedia.org/r/302229

Dzahn added a comment.Aug 16 2016, 4:04 AM

I merged Paladox' change https://gerrit.wikimedia.org/r/#/c/302229/16 and just linked from there to
T75997#2518198 directly.

Dzahn closed this task as Resolved.Aug 16 2016, 4:04 AM
Dzahn assigned this task to Paladox.
Dzahn removed a project: Patch-For-Review.

T75997#2518198 works, but https://phabricator.wikimedia.org/T75997#2518198 continues to fail at https://gerrit.wikimedia.org/r/302229. Should I re-open this task or file a new task?

Maybe re open this task please. But not much people should notice unless they link to phabricator comments

jayvdb reopened this task as Open.Aug 16 2016, 8:32 AM

T75997#2518198 works, but https://phabricator.wikimedia.org/T75997#2518198 continues to fail at https://gerrit.wikimedia.org/r/302229. Should I re-open this task or file a new task?

As title and description says, the task was about full links, so in fact the task shouldn't have been closed as it hasn't been fixed.

As suggested for a long, long time now (see https://gerrit.wikimedia.org/r/256663) you should change (?!\") to (?![#\"]) or even (?![#\"<]). I strongly believe this will solve most, if not all of the remaining issues.

It will break most things actually. It fixes full links but then breaks the short links. As I have tested that.

What exactly does "break" mean? How does the addition of a # character break inputs that do not contain this character?

The current regular expression (which replaces "\\bT(\\d+)(#\\d+)?\\b(?!\")" with https://phabricator.wikimedia.org/T$1$2) destroys some links because the HTML <a href="https://phabricator.wikimedia.org/T1#2">…</a> contains two possible substrings that seem to fit: But the obvious pick T1#2 is not used any more (finally!) because it is followed by a double quote, which is excluded by the regular expression. So it picks T1 and still destroys the existing HTML.

Exactly this is fixed by replacing (?!\") with (?![#\"]).

It will then still destroy HTML like <a href="…">https://phabricator.wikimedia.org/T1#2</a> and possibly make the whole situation worse because the final HTML Tidy step can not work this out any more. Which is what (?![#\"<]) will fix.

This may lead to a situation where some specific T… numbers in specific contexts are not replaced any more. Which is, in my humble opinion, acceptable. What I find unacceptable is messed up HTML.

Over at https://gerrit.wikimedia.org/r/288462, Krinkle used "%54" instead of "T" as a workaround. Example: https://phabricator.wikimedia.org/%54135170#2532152. Hackish, but effective!

Paladox added a comment.EditedAug 24 2016, 9:23 AM

@thiemowmde hi yes that's what I mean some plain T 1 for example links will break, I found they broke in the comment box but not in commit msg.

I'm wondering is there a way we can do it your way but also fix it so plain T 1 work please.

(Remove space between T and 1)

I can test any proposed solutions, since I have a test instance. But we doint want to be breaking things when it only breaks if you link to the full link with a comment Id.

Again: What does "break" mean? What exactly do you input? What exactly do you get?

@thiemowmde

You said this

This may lead to a situation where some specific T… numbers in specific contexts are not replaced any more. Which is, in my humble opinion, acceptable. What I find unacceptable is messed up HTML.

Which is true.

Is there something wrong with the question I'm now asking for the third time? Can you please, please, please explain your test scenario, what exactly you entered (please copy and paste it), what you expected and what erroneous output you got instead (again, please copy the resulting HTML source and paste it here)? Thank you very much.

Paladox added a comment.EditedAug 24 2016, 10:44 AM

@thiemowmde I have told you.

When you use your change, it causes the T1 links for example to not work in the comment box but work in commit msg. IE T1 will no longer link, whereas the full link will.

See http://gerrit-test.wmflabs.org/gerrit/#/c/16/ please

Oh wait, it seems to work now since upgrading to gerrit 2.12.4.

Change 306413 had a related patch set (by Paladox) published:
Fix phabricator expanding links

https://gerrit.wikimedia.org/r/306413

Again, what exactly do you enter in the comment box? Only the two characters T1 and nothing else? Other characters or words before or after? What happens if the comment contains spaces before and after the ticket number, e.g. xx T1 xx? Newlines?

So under which exact circumstances does it stop working when you change nothing but what I suggested above (and still strongly suggest): Expand (?!\") to (?![#\"<]). No other change.

On http://gerrit-test.wmflabs.org/gerrit/#/c/16/ I do see two comments with ticket numbers that are not part of a pasted http://… link, one on PS5 and one on PS28, and they are all blue. Which of these turn black with the proposed change?

@Paladox: Noone can look over your shoulder. So as long as you do not provide an absolutely clear list of numbered steps and avoid any paraphrasing and vagueness, things will remain unclear...

Paladox added a comment.EditedAug 25 2016, 12:48 AM

@thiemowmde hi, ok done I deployed your change on the test server, please take a look at http://gerrit-test.wmflabs.org/gerrit/#/c/16/ (look at the comment box scroll to bottom and expand comment with the plain T numbers please to see the problem.

also sorry for late reply and deploy.

the commit msg works but in the comment box it dosent please could you recheck since I deployed the change you wanted please?

it could be because plain T numbers doint include the < so they are ignored but the full links work because they include <

we should go with the patch I created since it may not be perfect but at least it makes an improvement. it makes the links look nice instead of horrible and expanded. we can also improve it further.

Qgil removed a subscriber: Qgil.Aug 25 2016, 9:13 AM
thiemowmde added a comment.EditedAug 25 2016, 3:24 PM

I agree that https://gerrit.wikimedia.org/r/306413 should be merged as it is because it is already an improvement.

@Paladox, your change on the test system helped a lot, thank you very much. Note that one of the comments contains a plain T1 in the middle of a sentence, and this one is linked. The ones that are not linked are directly followed by </p> in the HTML source, which explains everything.

Please replace (?![#\"<]) with (?![#\"]|</a>) on the test system.

Paladox added a comment.EditedAug 25 2016, 4:13 PM

@thiemowmde no thank you since your working on a fix and I test it :)

Paladox added a comment.EditedAug 25 2016, 4:37 PM

@thiemowmde I get this error
fatal: bad config file line 121 in /var/www/html/review_site/etc/gerrit

That line is

match = "\\bT(\\d+)(#\\d+)?\\b(?![#\"]|<\/a>)"

I have nothing in the logs to say what piece is failing, only the line that is causing the failures. Maybe it is because the regex is invalid or incorrect?

Paladox added a comment.EditedAug 26 2016, 10:28 AM

I fixed it by doing

match = "\\bT(\\d+)(#\\d+)?\\b(?![#\"]|<\\/a>)"

per @thiemowmde I am now doing

match = "\\bT(\\d+)(#\\d+)?\\b(?![#\"]|</a>)"

and works :) just waiting for patch to be merged. I have tested it and works. See http://gerrit-test.wmflabs.org/gerrit/#/c/16/ please.

@thiemowmde also tested and said it looked good :)

Paladox added a comment.EditedAug 26 2016, 10:58 AM

@MZMcBride this change https://gerrit.wikimedia.org/r/#/c/306413/ done by @thiemowmde should now fix

https://phabricator.wikimedia.org/T75997#2518198

see http://gerrit-test.wmflabs.org/gerrit/#/c/16/ please :)

Change 306413 merged by Dzahn:
Gerrit: Fix phabricator expanding links

https://gerrit.wikimedia.org/r/306413

Paladox closed this task as Resolved.Aug 31 2016, 12:25 AM

@MZMcBride hi, the fix has been deployed, does
https://gerrit.wikimedia.org/r/306413 that look better for you please? (Look at bottom)

Paladox removed a subscriber: gerritbot.

Woo! Thank you all for the hard work here. :-)

Change 256663 merged by Dzahn:
Gerit: Rewrite outdated comment about Gerrit-Phabricator linking

https://gerrit.wikimedia.org/r/256663