From a2f909f0bd0a195e06730e955e4d5a29bb3ad61b Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Sun, 8 Nov 2015 21:54:47 +1100 Subject: [PATCH] Improve XHPAST handling of syntax errors Summary: Currently, a bunch of developers are using #xhpast for writing custom linter rules. As such, we end up with a fair few `XHPASTSyntaxErrorException` in our PHP error logs. I think that throwing an exception is not quite correct in this case because it is somewhat expected that invalid PHP may be entered. Instead, catch the exception and show the user a helpful message. Test Plan: This doesn't quite work yet... the stream and tree views render as blank but the exceptions still propogate to the error logs. Mostly, I'm not sure how the exception should be rendered for display. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14028 --- .../autopatches/20151108.xhpast.stderr.sql | 5 +++++ .../PhabricatorXHPASTViewController.php | 1 - .../PhabricatorXHPASTViewFrameController.php | 2 +- ...habricatorXHPASTViewFramesetController.php | 22 +++++++++---------- .../PhabricatorXHPASTViewPanelController.php | 8 +++---- .../PhabricatorXHPASTViewRunController.php | 20 ++++++++++------- .../PhabricatorXHPASTViewStreamController.php | 12 +++++++--- .../PhabricatorXHPASTViewTreeController.php | 13 +++++++---- .../PhabricatorXHPASTViewParseTree.php | 5 ++++- 9 files changed, 54 insertions(+), 34 deletions(-) create mode 100644 resources/sql/autopatches/20151108.xhpast.stderr.sql diff --git a/resources/sql/autopatches/20151108.xhpast.stderr.sql b/resources/sql/autopatches/20151108.xhpast.stderr.sql new file mode 100644 index 0000000000..1721505658 --- /dev/null +++ b/resources/sql/autopatches/20151108.xhpast.stderr.sql @@ -0,0 +1,5 @@ +ALTER TABLE {$NAMESPACE}_xhpastview.xhpastview_parsetree + ADD returnCode INT NOT NULL AFTER input; + +ALTER TABLE {$NAMESPACE}_xhpastview.xhpastview_parsetree + ADD stderr longtext NOT NULL AFTER stdout; diff --git a/src/applications/phpast/controller/PhabricatorXHPASTViewController.php b/src/applications/phpast/controller/PhabricatorXHPASTViewController.php index 7d3ccf8187..2ef34fb36f 100644 --- a/src/applications/phpast/controller/PhabricatorXHPASTViewController.php +++ b/src/applications/phpast/controller/PhabricatorXHPASTViewController.php @@ -3,7 +3,6 @@ abstract class PhabricatorXHPASTViewController extends PhabricatorController { public function buildStandardPageResponse($view, array $data) { - $page = $this->buildStandardPageView(); $page->setApplicationName('XHPASTView'); diff --git a/src/applications/phpast/controller/PhabricatorXHPASTViewFrameController.php b/src/applications/phpast/controller/PhabricatorXHPASTViewFrameController.php index 2c5a43687c..89f5b8fd7d 100644 --- a/src/applications/phpast/controller/PhabricatorXHPASTViewFrameController.php +++ b/src/applications/phpast/controller/PhabricatorXHPASTViewFrameController.php @@ -14,7 +14,7 @@ final class PhabricatorXHPASTViewFrameController phutil_tag( 'iframe', array( - 'src' => '/xhpast/frameset/'.$id.'/', + 'src' => "/xhpast/frameset/{$id}/", 'frameborder' => '0', 'style' => 'width: 100%; height: 800px;', '', diff --git a/src/applications/phpast/controller/PhabricatorXHPASTViewFramesetController.php b/src/applications/phpast/controller/PhabricatorXHPASTViewFramesetController.php index de446b5e44..6f186fb3f8 100644 --- a/src/applications/phpast/controller/PhabricatorXHPASTViewFramesetController.php +++ b/src/applications/phpast/controller/PhabricatorXHPASTViewFramesetController.php @@ -10,17 +10,15 @@ final class PhabricatorXHPASTViewFramesetController public function handleRequest(AphrontRequest $request) { $id = $request->getURIData('id'); - $response = new AphrontWebpageResponse(); - $response->setFrameable(true); - $response->setContent(phutil_tag( - 'frameset', - array('cols' => '33%, 34%, 33%'), - array( - phutil_tag('frame', array('src' => "/xhpast/input/{$id}/")), - phutil_tag('frame', array('src' => "/xhpast/tree/{$id}/")), - phutil_tag('frame', array('src' => "/xhpast/stream/{$id}/")), - ))); - - return $response; + return id(new AphrontWebpageResponse()) + ->setFrameable(true) + ->setContent(phutil_tag( + 'frameset', + array('cols' => '33%, 34%, 33%'), + array( + phutil_tag('frame', array('src' => "/xhpast/input/{$id}/")), + phutil_tag('frame', array('src' => "/xhpast/tree/{$id}/")), + phutil_tag('frame', array('src' => "/xhpast/stream/{$id}/")), + ))); } } diff --git a/src/applications/phpast/controller/PhabricatorXHPASTViewPanelController.php b/src/applications/phpast/controller/PhabricatorXHPASTViewPanelController.php index 8f824dc03f..7238b36381 100644 --- a/src/applications/phpast/controller/PhabricatorXHPASTViewPanelController.php +++ b/src/applications/phpast/controller/PhabricatorXHPASTViewPanelController.php @@ -14,6 +14,7 @@ abstract class PhabricatorXHPASTViewPanelController $this->id = $data['id']; $this->storageTree = id(new PhabricatorXHPASTViewParseTree()) ->load($this->id); + if (!$this->storageTree) { throw new Exception(pht('No such AST!')); } @@ -65,10 +66,9 @@ li span { '', $content); - $response = new AphrontWebpageResponse(); - $response->setFrameable(true); - $response->setContent($content); - return $response; + return id(new AphrontWebpageResponse()) + ->setFrameable(true) + ->setContent($content); } } diff --git a/src/applications/phpast/controller/PhabricatorXHPASTViewRunController.php b/src/applications/phpast/controller/PhabricatorXHPASTViewRunController.php index dd9cf85433..dc2224d291 100644 --- a/src/applications/phpast/controller/PhabricatorXHPASTViewRunController.php +++ b/src/applications/phpast/controller/PhabricatorXHPASTViewRunController.php @@ -13,17 +13,21 @@ final class PhabricatorXHPASTViewRunController $resolved = $future->resolve(); // This is just to let it throw exceptions if stuff is broken. - $parse_tree = XHPASTTree::newFromDataAndResolvedExecFuture( - $source, - $resolved); + try { + XHPASTTree::newFromDataAndResolvedExecFuture($source, $resolved); + } catch (XHPASTSyntaxErrorException $ex) { + // This is possibly expected. + } list($err, $stdout, $stderr) = $resolved; - $storage_tree = new PhabricatorXHPASTViewParseTree(); - $storage_tree->setInput($source); - $storage_tree->setStdout($stdout); - $storage_tree->setAuthorPHID($viewer->getPHID()); - $storage_tree->save(); + $storage_tree = id(new PhabricatorXHPASTViewParseTree()) + ->setInput($source) + ->setReturnCode($err) + ->setStdout($stdout) + ->setStderr($stderr) + ->setAuthorPHID($viewer->getPHID()) + ->save(); return id(new AphrontRedirectResponse()) ->setURI('/xhpast/view/'.$storage_tree->getID().'/'); diff --git a/src/applications/phpast/controller/PhabricatorXHPASTViewStreamController.php b/src/applications/phpast/controller/PhabricatorXHPASTViewStreamController.php index 3fe1046f10..f5cedf225a 100644 --- a/src/applications/phpast/controller/PhabricatorXHPASTViewStreamController.php +++ b/src/applications/phpast/controller/PhabricatorXHPASTViewStreamController.php @@ -6,11 +6,17 @@ final class PhabricatorXHPASTViewStreamController public function handleRequest(AphrontRequest $request) { $storage = $this->getStorageTree(); $input = $storage->getInput(); + $err = $storage->getReturnCode(); $stdout = $storage->getStdout(); + $stderr = $storage->getStderr(); - $tree = XHPASTTree::newFromDataAndResolvedExecFuture( - $input, - array(0, $stdout, '')); + try { + $tree = XHPASTTree::newFromDataAndResolvedExecFuture( + $input, + array($err, $stdout, $stderr)); + } catch (XHPASTSyntaxErrorException $ex) { + return $this->buildXHPASTViewPanelResponse($ex->getMessage()); + } $tokens = array(); foreach ($tree->getRawTokenStream() as $id => $token) { diff --git a/src/applications/phpast/controller/PhabricatorXHPASTViewTreeController.php b/src/applications/phpast/controller/PhabricatorXHPASTViewTreeController.php index 1b4eec6441..c15bdc2928 100644 --- a/src/applications/phpast/controller/PhabricatorXHPASTViewTreeController.php +++ b/src/applications/phpast/controller/PhabricatorXHPASTViewTreeController.php @@ -10,18 +10,23 @@ final class PhabricatorXHPASTViewTreeController public function handleRequest(AphrontRequest $request) { $storage = $this->getStorageTree(); $input = $storage->getInput(); + $err = $storage->getReturnCode(); $stdout = $storage->getStdout(); + $stderr = $storage->getStderr(); - $tree = XHPASTTree::newFromDataAndResolvedExecFuture( - $input, - array(0, $stdout, '')); + try { + $tree = XHPASTTree::newFromDataAndResolvedExecFuture( + $input, + array($err, $stdout, $stderr)); + } catch (XHPASTSyntaxErrorException $ex) { + return $this->buildXHPASTViewPanelResponse($ex->getMessage()); + } $tree = phutil_tag('ul', array(), $this->buildTree($tree->getRootNode())); return $this->buildXHPASTViewPanelResponse($tree); } protected function buildTree($root) { - try { $name = $root->getTypeName(); $title = $root->getDescription(); diff --git a/src/applications/phpast/storage/PhabricatorXHPASTViewParseTree.php b/src/applications/phpast/storage/PhabricatorXHPASTViewParseTree.php index fa9adb62a2..d4432af496 100644 --- a/src/applications/phpast/storage/PhabricatorXHPASTViewParseTree.php +++ b/src/applications/phpast/storage/PhabricatorXHPASTViewParseTree.php @@ -3,16 +3,19 @@ final class PhabricatorXHPASTViewParseTree extends PhabricatorXHPASTViewDAO { protected $authorPHID; - protected $input; + protected $returnCode; protected $stdout; + protected $stderr; protected function getConfiguration() { return array( self::CONFIG_COLUMN_SCHEMA => array( 'authorPHID' => 'phid?', 'input' => 'text', + 'returnCode' => 'sint32', 'stdout' => 'text', + 'stderr' => 'text', ), ) + parent::getConfiguration(); } -- 2.51.2