From 94f6b6ca4e693efea8a924df63fd62cd85e9d0bb Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 4 Feb 2013 17:06:34 -0800 Subject: [PATCH] Fix every HTML issue I could find Summary: I attempted to test every interface. I probably missed some stuff, but I at least have some level of confidence that the `phutil_tag` branch is fairly stable. Fixed these issues: [1] Fixed a Herald issue with object links in transcripts. Some actions return links; this was previously hard-coded. [2] DarkConsole refactoring created an issue where the "`" event handler registered too many times. [3] Fixed a bug where `strlen($value)` was being checked, but fields may now return array(). Possibly we should implement phutil_is_empty_html() or similar. [4] Fixed a undefined variable issue for image edit transactions. [5] Fixed an issue with rendering participant transactions. This required phutil_safe_html() because `pht()` can't handle `array()` for `%s`. [6] Fixed an issue where feed was entirely overescaped by reverting an overly ambitious render_tag -> tag. [7] Fixed an issue with strict tables and inserting `''` instead of `0` into an integer column. [8] Fixed an issue where • was shown escaped. [9] Fixed an issue where "no data" was overescaped. [10] Fixed an issue with strict tables and inserting `''` instead of `0` into an integer column. [11] Fixed an issue with strict tables and inserting `''`. [12] Fixed an issue with missing space after ":" for mini panels. Encountered (but did not fix) these issues: [X1] "e" works incorrectly on comments you are not allowed to edit. Did not fix. [X2] Coverage currently interacts incorrectly with "--everything" for Phutil tests. Test Plan: - Viewed Differential. - Created a diff via copy/paste. - Viewed standalone diff. - Jumped to diff via changeset table. - Created a revision. - Updated revision. - Added a comment. - Edited revision dependencies. - Edited revision tasks. - Viewed MetaMTA transcripts. - Viewed Herald transcripts [1]. - Downloaded raw diff. - Flagged / unflagged revision. - Added/edited/deleted inline comment. - Collapsed/expanded file. - Did show raw left. - Did show raw right. - Checked previews for available actions. - Clicked remarkup buttons - Used filetree view. - Used keyboard: F, j, k, J, K, n, p, t, h, "?" [2] [X1]. - Created a meme. - Uploaded a file via drag and drop. - Viewed a revision with no reviewers. - Viewed a revision with >100 files. - Viewed various other revisions [3]. - Viewed an image diff. - Added image diff inline comments. - Viewed Maniphest. - Ran various queries. - Created task. - Created similar task. - Added comments to tasks. - Ran custom query. - Saved custom query. - Edited custom queries. - Drag-reordered tasks. - Batch edited tasks. - Exported tasks to excel. - Looked at reports (issue in T2311 notwithstanding). - Viewed Diffusion. - Browsed Git, SVN, HG repositories. - Looked at history, browse, change, commit views. - Viewed audit. - Performed various audit searches. - Viewed Paste. - Performed paste searches. - Created, edited, forked paste. - Viewed Phriction. - Edited a page. - Viewed edit history. - Used search typeahead to search for user / application. - Used search to search for text. - Viewed Phame. - Viewed Blog, Post. - Viewed live post. - Published/unpublished post. - Previewed post. - Viewed Pholio. - Edited/commented mock. - Viewed ponder. - Viewed question. - Added answer/comment. - Viewed Diviner. - Viewed Conpherence [4] [5]. - Made Conpherence updates. - Viewed calendar. - Created status. - Viewed status. - Viewed Feed [6]. - Viewed Projects. - Viewed project detail. - Edited project. - Viewed Owners. - Viewed package detail. - Edited package [7]. - Viewed flags. - Edited flag. - Deleted flag. - Viewed Herald. - Viewed rules. - Created rule. - Edited rule. - Viewed edit log. - Viewed transcripts. - Inspected a transcript. - Viewed People. - Viewed list. - Administrated user. - Checked username/delete stuff. - Looked at create/import LDAP/activity logs. - Looked at a user profile. - Looked at user about page. - Looked at Repositories. - Edited repository. - Edited arcanist project. - Looked at daemons. - Looked at all daemons [8]. - Viewed combined log. - Looked at configuration. - Edited configuration. - Looked at setup issues [9]. - Looked at current settings. - Looked at application list. - Installed / uninstalled applications [10]. - Looked at mailing lists. - Created a mailing list. - Edited a mailing list. - Looked at sent mail. - Looked at received mail. - Looked at send/receive tests. - Looked at settings. - Clicked through all the panels. - Looked at slowvote. - Created a slowvote [11]. - Voted in a slowvote. - Looked at Macro. - Created a macro. - Edited a macro. - Commented on a macro. - Looked at Countdown. - Created a Countdown. - Looked at it. - Looked at Drydock. - Poked around a bit. - Looked at Fact. - Poked around a bit. - Looked at files. - Looked at a file. - Uploaded a file. - Looked at Conduit. - Made a Conduit call. - Looked at UIExamples. - Looked at PHPAST. - Looked at PHIDs. - Looked at notification menu. - Looked at notification detail. - Logged out. - Logged in. - Looked at homepage [12]. - Ran `arc unit --everything --no-coverage` [X2]. Reviewers: vrana, btrahan Reviewed By: vrana CC: aran Maniphest Tasks: T2432 Differential Revision: https://secure.phabricator.com/D4807 --- .../config/editor/PhabricatorConfigEditor.php | 4 +- .../controller/ConpherenceController.php | 4 +- .../view/ConpherenceTransactionView.php | 1 + .../view/PhabricatorDaemonLogListView.php | 6 +-- ...rentialChangesetFileTreeSideNavBuilder.php | 11 +++-- .../view/DifferentialRevisionDetailView.php | 2 +- .../PhabricatorDirectoryMainController.php | 2 +- .../feed/view/PhabricatorFeedStoryView.php | 4 +- .../controller/HeraldTranscriptController.php | 28 ++++++------ .../PhabricatorOwnersEditController.php | 5 ++- .../PhabricatorSlowvoteCreateController.php | 2 +- .../PhabricatorApplicationTransaction.php | 2 +- .../layout/PhabricatorObjectItemListView.php | 3 +- .../application/core/behavior-dark-console.js | 45 ++++++++++--------- 14 files changed, 65 insertions(+), 54 deletions(-) diff --git a/src/applications/config/editor/PhabricatorConfigEditor.php b/src/applications/config/editor/PhabricatorConfigEditor.php index a2ca330fdf..cb21eb2a0d 100644 --- a/src/applications/config/editor/PhabricatorConfigEditor.php +++ b/src/applications/config/editor/PhabricatorConfigEditor.php @@ -18,7 +18,7 @@ final class PhabricatorConfigEditor switch ($xaction->getTransactionType()) { case PhabricatorConfigTransaction::TYPE_EDIT: return array( - 'deleted' => (bool)$object->getIsDeleted(), + 'deleted' => (int)$object->getIsDeleted(), 'value' => $object->getValue(), ); } @@ -54,7 +54,7 @@ final class PhabricatorConfigEditor $v['value']); } - $object->setIsDeleted($v['deleted']); + $object->setIsDeleted((int)$v['deleted']); $object->setValue($v['value']); break; } diff --git a/src/applications/conpherence/controller/ConpherenceController.php b/src/applications/conpherence/controller/ConpherenceController.php index 6ca418661d..3050ceb224 100644 --- a/src/applications/conpherence/controller/ConpherenceController.php +++ b/src/applications/conpherence/controller/ConpherenceController.php @@ -155,7 +155,9 @@ abstract class ConpherenceController extends PhabricatorController { $item->addClass('conpherence-selected'); $item->addClass('hide-unread-count'); } - $nav->addCustomBlock($item->render()); + + // TODO: [HTML] Clean this up when we clean up HTML stuff in Conpherence. + $nav->addCustomBlock(phutil_safe_html($item->render())); } if (empty($conpherences) || $read) { $nav->addCustomBlock($this->getNoConpherencesBlock()); diff --git a/src/applications/conpherence/view/ConpherenceTransactionView.php b/src/applications/conpherence/view/ConpherenceTransactionView.php index c44b80faa5..bcb3278fa7 100644 --- a/src/applications/conpherence/view/ConpherenceTransactionView.php +++ b/src/applications/conpherence/view/ConpherenceTransactionView.php @@ -35,6 +35,7 @@ final class ConpherenceTransactionView extends AphrontView { ->setEpoch($transaction->getDateCreated()) ->setContentSource($transaction->getContentSource()); + $content = null; $content_class = null; switch ($transaction->getTransactionType()) { case ConpherenceTransactionType::TYPE_TITLE: diff --git a/src/applications/daemon/view/PhabricatorDaemonLogListView.php b/src/applications/daemon/view/PhabricatorDaemonLogListView.php index c60b584209..190e17c3cd 100644 --- a/src/applications/daemon/view/PhabricatorDaemonLogListView.php +++ b/src/applications/daemon/view/PhabricatorDaemonLogListView.php @@ -47,17 +47,17 @@ final class PhabricatorDaemonLogListView extends AphrontView { case PhabricatorDaemonLog::STATUS_RUNNING: $style = 'color: #00cc00'; $title = 'Running'; - $symbol = '•'; + $symbol = "\xE2\x80\xA2"; break; case PhabricatorDaemonLog::STATUS_DEAD: $style = 'color: #cc0000'; $title = 'Died'; - $symbol = '•'; + $symbol = "\xE2\x80\xA2"; break; case PhabricatorDaemonLog::STATUS_EXITED: $style = 'color: #000000'; $title = 'Exited'; - $symbol = '•'; + $symbol = "\xE2\x80\xA2"; break; case PhabricatorDaemonLog::STATUS_UNKNOWN: default: // fallthrough diff --git a/src/applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php b/src/applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php index 6ee62cc218..f2b1e59f40 100644 --- a/src/applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php +++ b/src/applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php @@ -116,10 +116,13 @@ final class DifferentialChangesetFileTreeSideNavBuilder { } $tree->destroy(); - $filetree = - '
'. - implode("\n", $filetree). - '
'; + $filetree = phutil_tag( + 'div', + array( + 'class' => 'phabricator-filetree', + ), + $filetree); + $nav->addLabel(pht('Changed Files')); $nav->addCustomBlock($filetree); $nav->setActive(true); diff --git a/src/applications/differential/view/DifferentialRevisionDetailView.php b/src/applications/differential/view/DifferentialRevisionDetailView.php index 55204ed32b..75fe45bfaf 100644 --- a/src/applications/differential/view/DifferentialRevisionDetailView.php +++ b/src/applications/differential/view/DifferentialRevisionDetailView.php @@ -80,7 +80,7 @@ final class DifferentialRevisionDetailView extends AphrontView { foreach ($this->auxiliaryFields as $field) { $value = $field->renderValueForRevisionView(); - if (strlen($value)) { + if ($value !== null) { $label = rtrim($field->renderLabelForRevisionView(), ':'); $properties->addProperty($label, $value); } diff --git a/src/applications/directory/controller/PhabricatorDirectoryMainController.php b/src/applications/directory/controller/PhabricatorDirectoryMainController.php index eca0c28b1a..e61cf16b48 100644 --- a/src/applications/directory/controller/PhabricatorDirectoryMainController.php +++ b/src/applications/directory/controller/PhabricatorDirectoryMainController.php @@ -345,7 +345,7 @@ final class PhabricatorDirectoryMainController array( ), array( - phutil_tag('strong', array(), $title.':'), + phutil_tag('strong', array(), $title.': '), $body ))); $this->minipanels[] = $panel; diff --git a/src/applications/feed/view/PhabricatorFeedStoryView.php b/src/applications/feed/view/PhabricatorFeedStoryView.php index bf23a53890..9900b5e9a7 100644 --- a/src/applications/feed/view/PhabricatorFeedStoryView.php +++ b/src/applications/feed/view/PhabricatorFeedStoryView.php @@ -111,7 +111,7 @@ final class PhabricatorFeedStoryView extends PhabricatorFeedView { require_celerity_resource('phabricator-feed-css'); - return phutil_tag( + return phutil_render_tag( 'div', array( 'class' => $this->oneLine @@ -119,7 +119,7 @@ final class PhabricatorFeedStoryView extends PhabricatorFeedView { : 'phabricator-feed-story', 'style' => $image_style, ), - array($head, $body, $foot)); + $this->renderSingleView(array($head, $body, $foot))); } } diff --git a/src/applications/herald/controller/HeraldTranscriptController.php b/src/applications/herald/controller/HeraldTranscriptController.php index 62af029da8..7008b49c6a 100644 --- a/src/applications/herald/controller/HeraldTranscriptController.php +++ b/src/applications/herald/controller/HeraldTranscriptController.php @@ -476,21 +476,21 @@ final class HeraldTranscriptController extends HeraldController { $rows = array(); foreach ($data as $name => $value) { - if (!is_scalar($value) && !is_null($value)) { - $value = implode("\n", $value); - } + if (!($value instanceof PhutilSafeHTML)) { + if (!is_scalar($value) && !is_null($value)) { + $value = implode("\n", $value); + } - if (strlen($value) > 256) { - $value = phutil_tag( - 'textarea', - array( - 'class' => 'herald-field-value-transcript', - ), - $value); - } else if ($name === 'Object Link') { - // The link cannot be escaped - } else { - $value = phutil_escape_html($value); + if (strlen($value) > 256) { + $value = phutil_tag( + 'textarea', + array( + 'class' => 'herald-field-value-transcript', + ), + $value); + } else { + $value = phutil_escape_html($value); + } } $rows[] = array( diff --git a/src/applications/owners/controller/PhabricatorOwnersEditController.php b/src/applications/owners/controller/PhabricatorOwnersEditController.php index a5aece08d1..1be18ddd8c 100644 --- a/src/applications/owners/controller/PhabricatorOwnersEditController.php +++ b/src/applications/owners/controller/PhabricatorOwnersEditController.php @@ -32,7 +32,10 @@ final class PhabricatorOwnersEditController $package->setName($request->getStr('name')); $package->setDescription($request->getStr('description')); $old_auditing_enabled = $package->getAuditingEnabled(); - $package->setAuditingEnabled($request->getStr('auditing') === 'enabled'); + $package->setAuditingEnabled( + ($request->getStr('auditing') === 'enabled') + ? 1 + : 0); $primary = $request->getArr('primary'); $primary = reset($primary); diff --git a/src/applications/slowvote/controller/PhabricatorSlowvoteCreateController.php b/src/applications/slowvote/controller/PhabricatorSlowvoteCreateController.php index 5dcb904707..3f3d5f1b6d 100644 --- a/src/applications/slowvote/controller/PhabricatorSlowvoteCreateController.php +++ b/src/applications/slowvote/controller/PhabricatorSlowvoteCreateController.php @@ -23,7 +23,7 @@ final class PhabricatorSlowvoteCreateController if ($request->isFormPost()) { $poll->setQuestion($request->getStr('question')); $poll->setResponseVisibility($request->getInt('response_visibility')); - $poll->setShuffle($request->getBool('shuffle', false)); + $poll->setShuffle((int)$request->getBool('shuffle', false)); $poll->setMethod($request->getInt('method')); if (!strlen($poll->getQuestion())) { diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 72e7ac08ad..00bbde7e3d 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -143,7 +143,7 @@ abstract class PhabricatorApplicationTransaction foreach ($phids as $phid) { $links[] = $this->renderHandleLink($phid); } - return implode(', ', $links); + return phutil_safe_html(implode(', ', $links)); } public function getIcon() { diff --git a/src/view/layout/PhabricatorObjectItemListView.php b/src/view/layout/PhabricatorObjectItemListView.php index 76b9c7655f..4ae41564ca 100644 --- a/src/view/layout/PhabricatorObjectItemListView.php +++ b/src/view/layout/PhabricatorObjectItemListView.php @@ -53,8 +53,7 @@ final class PhabricatorObjectItemListView extends AphrontView { $string = nonempty($this->noDataString, pht('No data.')); $items = id(new AphrontErrorView()) ->setSeverity(AphrontErrorView::SEVERITY_NODATA) - ->appendChild(phutil_escape_html($string)) - ->render(); + ->appendChild(phutil_escape_html($string)); } $pager = null; diff --git a/webroot/rsrc/js/application/core/behavior-dark-console.js b/webroot/rsrc/js/application/core/behavior-dark-console.js index 99370a9342..50a3471997 100644 --- a/webroot/rsrc/js/application/core/behavior-dark-console.js +++ b/webroot/rsrc/js/application/core/behavior-dark-console.js @@ -40,6 +40,8 @@ JX.behavior('dark-console', function(config, statics) { statics.visible = config.visible; statics.selected = config.selected; + install_shortcut(); + return statics.root; } @@ -202,29 +204,30 @@ JX.behavior('dark-console', function(config, statics) { JX.DOM.setContent(statics.el.panel, div); } - // Install keyboard shortcut. - var desc = 'Toggle visibility of DarkConsole.'; - new JX.KeyboardShortcut('`', desc) - .setHandler(function(manager) { - statics.visible = !statics.visible; - - if (statics.visible) { - JX.DOM.show(root); - if (statics.req.current) { - draw_request(statics.req.current); + function install_shortcut() { + var desc = 'Toggle visibility of DarkConsole.'; + new JX.KeyboardShortcut('`', desc) + .setHandler(function(manager) { + statics.visible = !statics.visible; + + if (statics.visible) { + JX.DOM.show(root); + if (statics.req.current) { + draw_request(statics.req.current); + } + } else { + JX.DOM.hide(root); } - } else { - JX.DOM.hide(root); - } - // Save user preference. - new JX.Request('/~/', JX.bag) - .setData({visible: statics.visible ? 1 : 0}) - .send(); + // Save user preference. + new JX.Request('/~/', JX.bag) + .setData({visible: statics.visible ? 1 : 0}) + .send(); - // Force resize listeners to take effect. - JX.Stratcom.invoke('resize'); - }) - .register(); + // Force resize listeners to take effect. + JX.Stratcom.invoke('resize'); + }) + .register(); + } }); -- 2.51.2