Differential D12692 Diff 30472 src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php

Changeset View

Standalone View

src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php

<?php		<?php

abstract class PhabricatorObjectRemarkupRule extends PhutilRemarkupRule {		abstract class PhabricatorObjectRemarkupRule extends PhutilRemarkupRule {

const KEY_RULE_OBJECT = 'rule.object';		const KEY_RULE_OBJECT = 'rule.object';
const KEY_MENTIONED_OBJECTS = 'rule.object.mentioned';		const KEY_MENTIONED_OBJECTS = 'rule.object.mentioned';

abstract protected function getObjectNamePrefix();		abstract protected function getObjectNamePrefix();
abstract protected function loadObjects(array $ids);		abstract protected function loadObjects(array $ids);

public function getPriority() {		public function getPriority() {
		joshuaspenceUnsubmitted Not Done Inline Actions Oh this isn't used... I was planning to add strings which had been matched as a URL (but then rejected because the URL didn't match an object) so that we could reject them in the `markupObjectEmbed` and `markupObjectReference` methods. But I don't think that we need to do this. joshuaspence: Oh this isn't used... I was planning to add strings which had been matched as a URL (but then…
return 450.0;		return 350.0;
}		}

protected function getObjectNamePrefixBeginsWithWordCharacter() {		protected function getObjectNamePrefixBeginsWithWordCharacter() {
$prefix = $this->getObjectNamePrefix();		$prefix = $this->getObjectNamePrefix();
return preg_match('/^\w/', $prefix);		return preg_match('/^\w/', $prefix);
}		}

protected function getObjectIDPattern() {		protected function getObjectIDPattern() {
Show All 18 Lines	protected function loadHandles(array $objects) {
}		}
return $result;		return $result;
}		}

protected function getObjectHref($object, $handle, $id) {		protected function getObjectHref($object, $handle, $id) {
return $handle->getURI();		return $handle->getURI();
}		}

protected function renderObjectRefForAnyMedia (		protected function renderObjectRefForAnyMedia(
$object,		$object,
$handle,		$handle,
$anchor,		$anchor,
$id) {		$id) {

$href = $this->getObjectHref($object, $handle, $id);		$href = $this->getObjectHref($object, $handle, $id);
$text = $this->getObjectNamePrefix().$id;		$text = $this->getObjectNamePrefix().$id;

if ($anchor) {		if ($anchor) {
$href = $href.'#'.$anchor;		$href = $href.'#'.$anchor;
$text = $text.'#'.$anchor;		$text = $text.'#'.$anchor;
}		}

▲ Show 20 Lines • Show All 41 Lines • ▼ Show 20 Lines	abstract class PhabricatorObjectRemarkupRule extends PhutilRemarkupRule {
}		}

protected function renderObjectEmbed($object, $handle, $options) {		protected function renderObjectEmbed($object, $handle, $options) {
$name = $handle->getFullName();		$name = $handle->getFullName();
$href = $handle->getURI();		$href = $handle->getURI();
$status_closed = PhabricatorObjectHandleStatus::STATUS_CLOSED;		$status_closed = PhabricatorObjectHandleStatus::STATUS_CLOSED;
$attr = array(		$attr = array(
'phid' => $handle->getPHID(),		'phid' => $handle->getPHID(),
'closed' => ($handle->getStatus() == $status_closed),		'closed' => ($handle->getStatus() == $status_closed),
);		);

return $this->renderHovertag($name, $href, $attr);		return $this->renderHovertag($name, $href, $attr);
}		}

protected function renderObjectTagForMail(		protected function renderObjectTagForMail(
$text,		$text,
$href,		$href,
Show All 25 Lines	return id(new PHUITagView())
->setHref($href)		->setHref($href)
->setType(PHUITagView::TYPE_OBJECT)		->setType(PHUITagView::TYPE_OBJECT)
->setPHID(idx($attr, 'phid'))		->setPHID(idx($attr, 'phid'))
->setClosed(idx($attr, 'closed'))		->setClosed(idx($attr, 'closed'))
->render();		->render();
}		}

public function apply($text) {		public function apply($text) {
$text = preg_replace_callback(		$text = preg_replace_callback(
$this->getObjectEmbedPattern(),		$this->getObjectEmbedPattern(),
		joshuaspenceUnsubmitted Not Done Inline Actions We could maybe add `markupObjectHyperlinkEmbed()`, but I don't think it would be used very often as the users who are more likely to use hyperlinks instead of object references probably don't know about the embed syntax anyway. joshuaspence: We could maybe add `markupObjectHyperlinkEmbed()`, but I don't think it would be used very…
array($this, 'markupObjectEmbed'),		array($this, 'markupObjectEmbed'),
$text);		$text);

$text = preg_replace_callback(		$text = preg_replace_callback(
$this->getObjectReferencePattern(),		$this->getObjectReferencePattern(),
array($this, 'markupObjectReference'),		array($this, 'markupObjectReference'),
$text);		$text);

return $text;		return $text;
}		}

private function getObjectEmbedPattern() {		private function getObjectEmbedPattern() {
$prefix = $this->getObjectNamePrefix();		$prefix = $this->getObjectNamePrefix();
$prefix = preg_quote($prefix);		$prefix = preg_quote($prefix);
$id = $this->getObjectIDPattern();		$id = $this->getObjectIDPattern();

return '(\B{'.$prefix.'('.$id.')([,\s](?:[^}\\\\]\|\\\\.)*)?}\B)u';		$base_uri = PhabricatorEnv::getEnvConfig('phabricator.base-uri');
		$base_uri = new PhutilURI($base_uri);

		return
		'(\B{'.
		'(?:'.preg_quote($base_uri).'/)?'.
		epriestleyAuthorUnsubmitted Not Done Inline Actions I suspect we need to match "a bunch of stuff that looks like it's probably a URI here", and then check if it's a URI we care about in the callback, so that these work: URIs with uppercase letters. http vs https. All URIs in `phabricator.allowed-uris`. Probably something like "some non-whitespace stuff", then "://", then "some more stuff with no whitespace", then "a slash"? epriestley: I suspect we need to match "a bunch of stuff that looks like it's probably a URI here", and…
		$prefix.'('.$id.')
		([,\s](?:[^}\\\\]\|\\\\.)*)?'.
		'}\B)u';
}		}

private function getObjectReferencePattern() {		private function getObjectReferencePattern() {
$prefix = $this->getObjectNamePrefix();		$prefix = $this->getObjectNamePrefix();
$prefix = preg_quote($prefix);		$prefix = preg_quote($prefix);

$id = $this->getObjectIDPattern();		$id = $this->getObjectIDPattern();

// If the prefix starts with a word character (like "D"), we want to		// If the prefix starts with a word character (like "D"), we want to
// require a word boundary so that we don't match "XD1" as "D1". If the		// require a word boundary so that we don't match "XD1" as "D1". If the
// prefix does not start with a word character, we want to require no word		// prefix does not start with a word character, we want to require no word
// boundary for the same reasons. Test if the prefix starts with a word		// boundary for the same reasons. Test if the prefix starts with a word
// character.		// character.
if ($this->getObjectNamePrefixBeginsWithWordCharacter()) {		if ($this->getObjectNamePrefixBeginsWithWordCharacter()) {
$boundary = '\\b';		$boundary = '\\b';
} else {		} else {
$boundary = '\\B';		$boundary = '\\B';
}		}

// The "(?<![#-])" prevents us from linking "#abcdef" or similar, and		// The "(?<![#-])" prevents us from linking "#abcdef" or similar, and
// "ABC-T1" (see T5714).		// "ABC-T1" (see T5714).
		//
// The "\b" allows us to link "(abcdef)" or similar without linking things		// The "\b" allows us to link "(abcdef)" or similar without linking things
// in the middle of words.		// in the middle of words.
		return
return '((?<![#-])'.$boundary.$prefix.'('.$id.')(?:#([-\w\d]+))?(?!\w))u';		'((?<![#-=/\?&])'.$boundary.
		epriestleyAuthorUnsubmitted Done Inline Actions We can fix some of those by adding `=` and `/` to this blacklist of lookbehind characters. epriestley: We can fix some of those by adding `=` and `/` to this blacklist of lookbehind characters.
		epriestleyAuthorUnsubmitted Done Inline Actions Probably also `?` and `&` for `a.html?T1` and `a.html?a=1&T1=true`. epriestley: Probably also `?` and `&` for `a.html?T1` and `a.html?a=1&T1=true`.
		'([a-zA-Z]+://\w+\.\w+/)?'.
		$prefix.'('.$id.')'.
		'(?:#([-\w\d]+))?(?!\w))u';
}		}


/**		/**
		joshuaspenceUnsubmitted Not Done Inline Actions This was copied from `PhutilRemarkupHyperlinkRule`. joshuaspence: This was copied from `PhutilRemarkupHyperlinkRule`.
		epriestleyAuthorUnsubmitted Not Done Inline Actions If you prefer less magic, you can remove MAGIC_BYTE from the regex -- matching strings which contain a MAGIC_BYTE will be rejected by `isFlatText()` for not being flat text. The MAGIC_BYTE stuff predates the concept of "flat text". But this is also fine as-is. epriestley: If you prefer less magic, you can remove MAGIC_BYTE from the regex -- matching strings which…
* Extract matched object references from a block of text.		* Extract matched object references from a block of text.
*		*
* This is intended to make it easy to write unit tests for object remarkup		* This is intended to make it easy to write unit tests for object remarkup
* rules. Production code is not normally expected to call this method.		* rules. Production code is not normally expected to call this method.
*		*
* @param string Text to match rules against.		* @param string Text to match rules against.
* @return wild Matches, suitable for writing unit tests against.		* @return wild Matches, suitable for writing unit tests against.
*/		*/
Show All 30 Lines	foreach ($sets as $type => $matches) {
$formatted[] = $format;		$formatted[] = $format;
}		}
$results[$type] = $formatted;		$results[$type] = $formatted;
}		}

return $results;		return $results;
}		}

public function markupObjectEmbed($matches) {		public function markupObjectEmbed(array $matches) {
if (!$this->isFlatText($matches[0])) {		if (!$this->isFlatText($matches[0])) {
return $matches[0];		return $matches[0];
}		}

return $this->markupObject(array(		return $this->markupObject(array(
'type' => 'embed',		'type' => 'embed',
'id' => $matches[1],		'id' => $matches[1],
'options' => idx($matches, 2),		'options' => idx($matches, 2),
'original' => $matches[0],		'original' => $matches[0],
));		));
}		}

public function markupObjectReference($matches) {		public function markupObjectReference(array $matches) {
if (!$this->isFlatText($matches[0])) {		if (!$this->isFlatText($matches[0])) {
return $matches[0];		return $matches[0];
}		}

		if ($matches[1]) {
		$uri = new PhutilURI($matches[1]);

		$base_uri = PhabricatorEnv::getEnvConfig('phabricator.base-uri');
		$base_uri = new PhutilURI($base_uri);
		trigger_error($uri->getDomain());
		joshuaspenceUnsubmitted Not Done Inline Actions Err, ignore this :P joshuaspence: Err, ignore this :P
		if (strtolower($uri->getDomain()) != strtolower($base_uri->getDomain())) {
		return $matches[0];
		}
		}

return $this->markupObject(array(		return $this->markupObject(array(
'type' => 'ref',		'type' => 'ref',
'id' => $matches[1],		'id' => $matches[2],
'anchor' => idx($matches, 2),		'anchor' => idx($matches, 3),
'original' => $matches[0],		'original' => $matches[0],
));		));
}		}

private function markupObject(array $params) {		private function markupObject(array $params) {
if (!$this->shouldMarkupObject($params)) {		if (!$this->shouldMarkupObject($params)) {
return $params['original'];		return $params['original'];
}		}

$regex = trim(		$regex = trim(
PhabricatorEnv::getEnvConfig('remarkup.ignored-object-names'));		PhabricatorEnv::getEnvConfig('remarkup.ignored-object-names'));
if ($regex && preg_match($regex, $params['original'])) {		if ($regex && preg_match($regex, $params['original'])) {
return $params['original'];		return $params['original'];
}		}

$engine = $this->getEngine();		$engine = $this->getEngine();
$token = $engine->storeText('x');		$token = $engine->storeText('x');

$metadata_key = self::KEY_RULE_OBJECT.'.'.$this->getObjectNamePrefix();		$metadata_key = self::KEY_RULE_OBJECT.'.'.$this->getObjectNamePrefix();
$metadata = $engine->getTextMetadata($metadata_key, array());		$metadata = $engine->getTextMetadata($metadata_key, array());

$metadata[] = array(		$metadata[] = array(
		epriestleyAuthorUnsubmitted Not Done Inline Actions I think a link like `http://secure.phabricator.com/T1` should display like this in the grey bubble: [ http://secure.phabricator.com/T1 ] ...not like this: [ T1 ] That is, we should recognize it and link it up, but visually retain the fact that it's a URL reference, not a monogram reference, so the actual bubble text is the URL, not just the monogram. This is probably a little worse for new/lazy users, but gets us most of the actual core value (mentions, hovercards, etc) and I think it's better for experienced users (specifically, it can be frustrating when you type text which includes a URL and your meaning is altered by summarization of the token, and retraining the URL avoids that). Generally, this reduces the frustration the rule can cause in cases where we get things wrong. epriestley: I think a link like `http://secure.phabricator.com/T1` should display like this in the grey…
		joshuaspenceUnsubmitted Not Done Inline Actions The idea is that the domain checking happens a few lines before this: if (strtolower($uri->getDomain()) != strtolower($base_uri->getDomain())) { return $matches[0]; } joshuaspence: The idea is that the domain checking happens a few lines before this: ```lang=php if…
'token' => $token,		'token' => $token,
) + $params;		) + $params;

$engine->setTextMetadata($metadata_key, $metadata);		$engine->setTextMetadata($metadata_key, $metadata);

		joshuaspenceUnsubmitted Not Done Inline Actions This is a little odd and I wanted to just do `$this->apply($path)` unconditionally... but the issue was that I couldn't return whether `$path` had actually been matched by `$this->getObjectEmbedPattern()` or `$this->getObjectReferencePattern()`. joshuaspence: This is a little odd and I wanted to just do `$this->apply($path)` unconditionally... but the…
return $token;		return $token;
}		}

public function didMarkupText() {		public function didMarkupText() {
$engine = $this->getEngine();		$engine = $this->getEngine();
		epriestleyAuthorUnsubmitted Not Done Inline Actions Do we need to do this at all? Can we just return $matches[0] and let the other rule pick it up later? epriestley: Do we need to do this at all? Can we just return $matches[0] and let the other rule pick it up…
		joshuaspenceUnsubmitted Not Done Inline Actions I don't think we can do that... my concern was that the following would happen: `http://phabricator.local/?id=T1` would be picked up by `markupObjectHyperlinkReference()` and then rejected because the path of the URI doesn't match an object. By returning `$matches[0]`, the text would then be passed to `markupObjectReference`... transforming it into `http://phabricator.local?id=` and `T1`. `PhutilRemarkupHyperlinkRule` would then turn `http://phabricator.local?id=` into a hyperlink. You end up with "http://phabricator.local?id= T1" (without the whitespace). The intention was to short-circuit this straight to `PhutilRemarkupHyperlinkRule`. joshuaspence: I don't think we can do that... my concern was that the following would happen: # `http…
		joshuaspenceUnsubmitted Not Done Inline Actions I don't understand why `return $rule->apply($matches[0])` doesn't work here. joshuaspence: I don't understand why `return $rule->apply($matches[0])` doesn't work here.
$metadata_key = self::KEY_RULE_OBJECT.'.'.$this->getObjectNamePrefix();		$metadata_key = self::KEY_RULE_OBJECT.'.'.$this->getObjectNamePrefix();
$metadata = $engine->getTextMetadata($metadata_key, array());		$metadata = $engine->getTextMetadata($metadata_key, array());

if (!$metadata) {		if (!$metadata) {
return;		return;
}		}


▲ Show 20 Lines • Show All 52 Lines • Show Last 20 Lines