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 $href string|PhutilURI|null | |||||
| */ | |||||
| 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)) { | |||||
| 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 38 Lines | final class PHUIObjectItemView extends AphrontTagView { | ||||
| } | } | ||||
| public function setImageHref($image_href) { | public function setImageHref($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; | ||||
Done Inline Actionspht avivey: `pht` | |||||
| } | } | ||||
Done Inline Actions✅ Lint: Partial Catch read, and ignored. valerio.bozzolan: ✅ Lint: Partial Catch read, and ignored. | |||||
| public function setImageIcon($image_icon) { | public function setImageIcon($image_icon) { | ||||
| if (!$image_icon instanceof PHUIIconView) { | if (!$image_icon instanceof PHUIIconView) { | ||||
| $image_icon = id(new PHUIIconView()) | $image_icon = id(new PHUIIconView()) | ||||
| ->setIcon($image_icon); | ->setIcon($image_icon); | ||||
| } | } | ||||
| $this->imageIcon = $image_icon; | $this->imageIcon = $image_icon; | ||||
| return $this; | return $this; | ||||
| ▲ Show 20 Lines • Show All 499 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.