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 everything! | |||||
| // https://we.phorge.it/T15316 | |||||
| if (is_object($href) && !($href instanceof PhutilURI)) { | |||||
| phlog(sprintf( | |||||
avivey: `pht()`, not `sprintf`. | |||||
| 'Received unexpected type for href: %s. '. | |||||
Done Inline Actions✅ Lint: Partial Catch read, and ignored. valerio.bozzolan: ✅ Lint: Partial Catch read, and ignored. | |||||
| 'Please paste this log as comment in https://we.phorge.it/T15316', | |||||
| get_class($href))); | |||||
| } | |||||
| $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 everything! | |||||
| // https://we.phorge.it/T15316 | |||||
| if (is_object($image_href) && !($image_href instanceof PhutilURI)) { | |||||
| phlog(sprintf( | |||||
aviveyUnsubmitted Done Inline Actionspht avivey: `pht` | |||||
| 'Received unexpected type for imageHref: %s. '. | |||||
Done Inline Actions✅ Lint: Partial Catch read, and ignored. valerio.bozzolan: ✅ Lint: Partial Catch read, and ignored. | |||||
| 'Please paste this log as comment in https://we.phorge.it/T15316', | |||||
| get_class($image_href))); | |||||
| } | |||||
| $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))) { | ||||
| $image_href = ($this->imageHref) ? $this->imageHref : $this->href; | $image_href = ($this->imageHref) ? $this->imageHref : $this->href; | ||||
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 = phutil_tag( | $image = phutil_tag( | ||||
| 'a', | 'a', | ||||
| array( | array( | ||||
| 'href' => $image_href, | 'href' => $image_href, | ||||
| ), | ), | ||||
| $image); | $image); | ||||
| } | } | ||||
| ▲ Show 20 Lines • Show All 221 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, ''); | ||||
| } | } | ||||
| } | } | ||||
Done Inline Actions✅ Lint: Read, and ignored. valerio.bozzolan: ✅ Lint: Read, and ignored. | |||||
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… | |||||
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… | |||||
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… | |||||
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… | |||||
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… | |||||
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()? | |||||
Not Done Inline Actionseither way is fine. avivey: either way is fine. | |||||
pht(), not sprintf.