Page MenuHomePhabricator

CodeReview autolinking
Closed, DeclinedPublicFeature

Description

Phabricator needs to link r12345 to CodeReview; either autolinking or editing existing comments replacing with real links. These are very common in the bugzilla database.

I ran into this on T12334 , and needed to manually construct the URL to see the patch that was applied, in order to answer the query on T74941.

Not providing links reduces the usefulness of old bugs, especially for newcomers who do not know all of the previous SCMs, migrations and how to manually jump across development systems which were not fully migrated.

Note there are two sets of Ids which need to be linked: MediaWiki and pywikibot, both with multiple branches.

https://www.mediawiki.org/wiki/Special:Code

Event Timeline

jayvdb raised the priority of this task from to Needs Triage.
jayvdb updated the task description. (Show Details)
jayvdb changed Security from none to None.
jayvdb subscribed.
Qgil triaged this task as Low priority.Nov 30 2014, 11:50 AM
Qgil added a project: Phabricator.
Aklapper lowered the priority of this task from Low to Lowest.Mar 3 2015, 12:39 PM
Aklapper subscribed.
Aklapper changed the subtype of this task from "Task" to "Feature Request".EditedJul 8 2023, 3:58 PM

I guess it makes more sense to change the rendering instead of changing ~12800 database entries[1]. However...

  • if we added a new file like src/applications/maniphest/remarkup/ManiphestMWCodeReviewRemarkupRule.php
    • with getObjectNamePrefix() returning 'r'
    • and getObjectIDPattern() returning '1?\d{0,5}'
    • and getObjectHref() returning something like 'https://static-codereview.wikimedia.org/MediaWiki/' . $uri . 'html',
  • and we ran arc liberate and added that file to the return array of src/applications/maniphest/application/PhabricatorManiphestApplication.php::getRemarkupRules() so it'll be applied,
  • we'd still have src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php defining abstract protected function loadObjects(array $ids), thus our new file must also implement loadObjects. But what to load?
  • In any case, if (empty($objects[$spec['id']])) will be true in PhabricatorObjectRemarkupRule::didMarkupText() as any object (e.g. r12345) does not exist in Phabricator itself (while objects like T12345 exist), and that code says For objects that are invalid or which the user can't see, just render the original text.

Looks like auto-linking to external stuff isn't that trivial to implement.
But was an interesting afternoon investigation and might also be useful for T687.

[1] SELECT COUNT(mtc.content) FROM phabricator_maniphest.maniphest_transaction_comment mtc INNER JOIN phabricator_maniphest.maniphest_transaction ta INNER JOIN phabricator_maniphest.maniphest_task t WHERE ta.objectPHID = t.phid AND mtc.phid = ta.commentPHID AND t.id < 75682 AND mtc.content REGEXP " r[0-9][0-9]+";

I checked descriptions of open tasks only found via SELECT CONCAT("https://phabricator.wikimedia.org/T", id) FROM maniphest_task WHERE (description REGEXP '\\sr[0-9]{2,5}+\\s' OR description REGEXP '\\sr10[0-9]{4,4}+\\s' OR description REGEXP '\\sr11[0-5]{4,4}+\\s') AND id < 75682 AND (status="open" OR status="progress" OR status="stalled"); which were 24 results. Manually edited half of them, the other half were false positives, e.g. some use r12345 as link text to Gerrit, and some were already links (using [[ r123 | https://foo ]] syntax).

Given my previous comment I'm going to decline this task.

Doesn't your previous comment only mean that you would have to subclass PhutilRemarkupRule directly rather than PhabricatorObjectRemarkupRule? I note that Wikimedia Phabricator already has custom code to allow embedding videos from Wikimedia Commons, so it clearly is possible to add markup rules for external stuff. (https://phabricator.wikimedia.org/source/phabricator/browse/wmf%252Fstable/src/infrastructure/markup/rule/WikimediaCommonsRemarkupRule.php)