Changeset View
Standalone View
src/view/phui/PHUIObjectItemView.php
| Show First 20 Lines • Show All 77 Lines • ▼ Show 20 Lines | public function setObject($object) { | ||||
| $this->object = $object; | $this->object = $object; | ||||
| return $this; | return $this; | ||||
| } | } | ||||
| public function getObject() { | public function getObject() { | ||||
| return $this->object; | return $this->object; | ||||
| } | } | ||||
| /** | |||||
| * Set the href attribute | |||||
| * | |||||
| * @param string|PhutilURI|null $href | |||||
| * @return self | |||||
| */ | |||||
| public function setHref($href) { | public function setHref($href) { | ||||
| // We have not a very clear idea about what this method should receive | |||||
| // We suspect that PhutilURI should be allowed... but let's log stuff! | |||||
| // https://we.phorge.it/T15316 | |||||
| try { | |||||
| self::assertValidHref($href, 'href'); | |||||
avivey: `pht()`, not `sprintf`. | |||||
| } catch (Exception $ex) { | |||||
Lint: Partial Catch Try/catch block catches "Exception", but does not catch "Throwable". In PHP7 and newer, some runtime exceptions will escape this block. Lint: Partial Catch: Try/catch block catches "Exception", but does not catch "Throwable". In PHP7 and newer, some… | |||||
Done Inline Actions✅ Lint: Partial Catch read, and ignored. valerio.bozzolan: ✅ Lint: Partial Catch read, and ignored. | |||||
| phlog($ex); | |||||
| } | |||||
| $this->href = $href; | $this->href = $href; | ||||
| return $this; | return $this; | ||||
| } | } | ||||
| /** | |||||
| * Get the href attribute | |||||
| * | |||||
| * @see PHUIObjectItemView::setHref() | |||||
| * @return string|PhutilURI|null | |||||
| */ | |||||
| public function getHref() { | public function getHref() { | ||||
| return $this->href; | return $this->href; | ||||
| } | } | ||||
| public function setHeader($header) { | public function setHeader($header) { | ||||
| $this->header = $header; | $this->header = $header; | ||||
| return $this; | return $this; | ||||
| } | } | ||||
| Show All 32 Lines | public function addByline($byline) { | ||||
| return $this; | return $this; | ||||
| } | } | ||||
| public function setImageURI($image_uri) { | public function setImageURI($image_uri) { | ||||
| $this->imageURI = $image_uri; | $this->imageURI = $image_uri; | ||||
| return $this; | return $this; | ||||
| } | } | ||||
| /** | |||||
| * Set the image href attribute | |||||
| * | |||||
| * @param string|PhutilURI|null $image_href | |||||
| * @return self | |||||
| */ | |||||
| public function setImageHref($image_href) { | public function setImageHref($image_href) { | ||||
| // We have not a very clear idea about what this method should receive | |||||
| // We suspect that PhutilURI should be allowed... but let's log stuff! | |||||
| // https://we.phorge.it/T15316 | |||||
| try { | |||||
| self::assertValidHref($image_href, 'image_href'); | |||||
Done Inline Actionspht avivey: `pht` | |||||
| } catch (Exception $ex) { | |||||
Lint: Partial Catch Try/catch block catches "Exception", but does not catch "Throwable". In PHP7 and newer, some runtime exceptions will escape this block. Lint: Partial Catch: Try/catch block catches "Exception", but does not catch "Throwable". In PHP7 and newer, some… | |||||
Done Inline Actions✅ Lint: Partial Catch read, and ignored. valerio.bozzolan: ✅ Lint: Partial Catch read, and ignored. | |||||
| phlog($ex); | |||||
| } | |||||
| $this->imageHref = $image_href; | $this->imageHref = $image_href; | ||||
| return $this; | return $this; | ||||
| } | } | ||||
| public function getImageURI() { | public function getImageURI() { | ||||
| return $this->imageURI; | return $this->imageURI; | ||||
| } | } | ||||
| ▲ Show 20 Lines • Show All 506 Lines • ▼ Show 20 Lines | if ($this->getImageURI()) { | ||||
| $image = phutil_tag( | $image = phutil_tag( | ||||
| 'div', | 'div', | ||||
| array( | array( | ||||
| 'class' => 'phui-oi-image-icon', | 'class' => 'phui-oi-image-icon', | ||||
| ), | ), | ||||
| $this->getImageIcon()); | $this->getImageIcon()); | ||||
| } | } | ||||
| if ($image && (phutil_nonempty_string($this->href) || | if ($image && (phutil_nonempty_stringlike($this->href) || | ||||
| phutil_nonempty_string($this->imageHref))) { | phutil_nonempty_stringlike($this->imageHref))) { | ||||
valerio.bozzolanAuthorUnsubmitted Done Inline ActionsNOTE: This was really the only needed fix ↑ All the rest is not strictly necessary.
This allows to receive also objects with a __toString() instead of being very string and only accepting native strings. valerio.bozzolan: NOTE: This was really the only needed fix ↑ All the rest is not strictly necessary.
This… | |||||
| $image_href = ($this->imageHref) ? $this->imageHref : $this->href; | $image_href = ($this->imageHref) ? $this->imageHref : $this->href; | ||||
| $image = phutil_tag( | $image = phutil_tag( | ||||
| 'a', | 'a', | ||||
| array( | array( | ||||
| 'href' => $image_href, | 'href' => $image_href, | ||||
| ), | ), | ||||
| $image); | $image); | ||||
| } | } | ||||
| ▲ Show 20 Lines • Show All 222 Lines • ▼ Show 20 Lines | private function renderHandleIcon(PhabricatorObjectHandle $handle, $label) { | ||||
| if (strlen($label)) { | if (strlen($label)) { | ||||
| $options['sigil'] = 'has-tooltip'; | $options['sigil'] = 'has-tooltip'; | ||||
| $options['meta'] = array('tip' => $label, 'align' => 'E'); | $options['meta'] = array('tip' => $label, 'align' => 'E'); | ||||
| } | } | ||||
| return javelin_tag('span', $options, ''); | return javelin_tag('span', $options, ''); | ||||
| } | } | ||||
| /** | |||||
| * Receive a href attribute and check if it has expected values | |||||
| * | |||||
| * TODO: Feel free to remove after 2023, if no more new reports arrive. | |||||
Lint: TODO Comment This comment has a TODO. Lint: TODO Comment: This comment has a TODO. | |||||
Done Inline Actions✅ Lint: Read, and ignored. valerio.bozzolan: ✅ Lint: Read, and ignored. | |||||
| * | |||||
| * https://we.phorge.it/T15316 | |||||
| * | |||||
| * @param mixed $href | |||||
| * @param string $variable_name | |||||
| */ | |||||
| private static function assertValidHref($href, $variable_name) { | |||||
| // We have not a very clear idea about what this method should receive | |||||
| // We suspect that PhutilURI should be allowed... but let's log stuff! | |||||
| if (is_object($href) && !($href instanceof PhutilURI)) { | |||||
| throw new Exception(pht( | |||||
aviveyUnsubmitted Not Done Inline ActionsRather then having throw-try-catch-log-ignore, just phlog the exception here and don't throw anything. You'll need to change the name of the method accordingly. avivey: Rather then having throw-try-catch-log-ignore, just phlog the exception here and don't throw… | |||||
valerio.bozzolanAuthorUnsubmitted Done Inline ActionsI understand (really: my previous diff version was exactly that) but I then examined Phorge and I was not able to find 1 usage of "phlog(new Exception". So I don't think we should introduce this approach today here. I think it was more a debugging trick than something that Evan would recommend. There are lot of "assertSomething" instead with catch. Even if the previous diff (whith your tip) is shorter, this is probably still "more OK". Sharing this, last word to you valerio.bozzolan: I understand (really: my previous diff version was exactly that) but I then examined Phorge and… | |||||
aviveyUnsubmitted Not Done Inline ActionsYes, it's not happening anywhere, but neither is "throw-catch-log", which is obviously worse (less obvious intention). I'm also not clear why you'd want the exception anyway - phlog already adds the complete trace of where it happened. avivey: Yes, it's not happening anywhere, but neither is "throw-catch-log", which is obviously worse… | |||||
valerio.bozzolanAuthorUnsubmitted Done Inline ActionsOK I will rewrite this method, thanks Premising that phlog() does not share the stack trace in the log without an exception: it only shares the log line without context. The stack trace is shared only if the user knows how to enable DarkConsole, if it was enabled, and if the user knows how to reproduce that problem to trigger it again and catch the stack trace. valerio.bozzolan: OK I will rewrite this method, thanks
Premising that `phlog()` does not share the stack trace… | |||||
aviveyUnsubmitted Not Done Inline Actionsyes, that's fine. I wouldn't expect anyone who isn't actively working on phorge to check the server logs for problems to report. avivey: yes, that's fine. I wouldn't expect anyone who isn't actively working on phorge to check the… | |||||
valerio.bozzolanAuthorUnsubmitted Done Inline ActionsSorry for clarification: Would you love to remove the shared stack trace, and just use phlog()? Thank you valerio.bozzolan: Sorry for clarification:
Would you love to remove the shared stack trace, and just use phlog()? | |||||
aviveyUnsubmitted Not Done Inline Actionseither way is fine. avivey: either way is fine. | |||||
| 'The variable %s received an unexpected type: %s. '. | |||||
| 'Please share this stack trace as comment in Task %s', | |||||
| $variable_name, | |||||
| get_class($href), | |||||
| 'https://we.phorge.it/T15316')); | |||||
| } | |||||
| } | |||||
| } | } | ||||
pht(), not sprintf.