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 | |||||
| // So, let's log alien stuff for some time | |||||
| // https://we.phorge.it/T15316 | |||||
| self::requireValidHref($href, 'href'); | |||||
avivey: `pht()`, not `sprintf`. | |||||
| $this->href = $href; | $this->href = $href; | ||||
Done Inline Actions✅ Lint: Partial Catch read, and ignored. valerio.bozzolan: ✅ Lint: Partial Catch read, and ignored. | |||||
| 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 | |||||
| // So, let's log alien stuff for some time | |||||
| // https://we.phorge.it/T15316 | |||||
| self::requireValidHref($image_href, 'image_href'); | |||||
Done Inline Actionspht avivey: `pht` | |||||
| $this->imageHref = $image_href; | $this->imageHref = $image_href; | ||||
Done Inline Actions✅ Lint: Partial Catch read, and ignored. valerio.bozzolan: ✅ Lint: Partial Catch read, and ignored. | |||||
| return $this; | return $this; | ||||
| } | } | ||||
| public function getImageURI() { | public function getImageURI() { | ||||
| return $this->imageURI; | return $this->imageURI; | ||||
| } | } | ||||
| public function setImageIcon($image_icon) { | public function setImageIcon($image_icon) { | ||||
| ▲ Show 20 Lines • Show All 505 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, ''); | ||||
| } | } | ||||
| /** | |||||
| * Receive a href attribute and check if it has expected values | |||||
| * | |||||
| * TODO: Feel free to remove after 2023, if no more new reports arrive. | |||||
Done Inline Actions✅ Lint: Read, and ignored. valerio.bozzolan: ✅ Lint: Read, and ignored. | |||||
| * | |||||
| * https://we.phorge.it/T15316 | |||||
| * | |||||
| * @param mixed $href Value to be checked | |||||
| * @param string $variable_name Human reference | |||||
| */ | |||||
| private static function requireValidHref($href, $variable_name) { | |||||
| // We have not a very clear idea about what a "href" should be | |||||
| if (is_object($href) && !($href instanceof PhutilURI)) { | |||||
| // We log stuff with a kind stack trace | |||||
| phlog(new Exception(pht( | |||||
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. | |||||
| '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.