Catch exceptions from scripts and let the user know where the error came from #699

Merged
merged 3 commits into from May 22, 2012

Conversation

Projects
None yet
5 participants
Contributor

metaturso commented May 16, 2012

Any exception coming from the listener is now caught and replaced by a \RuntimeException, this exception has a message that references the one provided from the intercepted exception.

Note:

  • What about passing $previousException parameter to the runtime exception constructor?
  • The feature has only been tested using the Composer\Test\Script\EventDispatcherTest test case as I am still not using Composer ATM, I'd appreciate if anyone of you could test it before pulling please.

metaturso added some commits May 15, 2012

@metaturso metaturso Wrapped the listener invocation a try/catch block that catches any
exception and throws a \RuntimeException.
Added a test case for the EventDispatcher.

Note:
In order to test the doDispatch method I had to use a stub EventDispatcher
with a getListeners that returned a preconfigured array. IMHO there should
be a way to inject the listeners into the EventDispatcher.
f626ccb
@metaturso metaturso Added exception class and message to the error string. f8b2f20

This pull request passes (merged f8b2f20 into 799a478).

@stloyd stloyd commented on an outdated diff May 16, 2012

tests/Composer/Test/Script/EventDispatcherTest.php
@@ -0,0 +1,58 @@
+<?php
+/*
+ * This file is part of Composer.
+ *
+ * (c) Nils Adermann <naderman@naderman.de>
+ * Jordi Boggiano <j.boggiano@seld.be>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Composer\Test\Script;
+
+use Exception;
@stloyd

stloyd May 16, 2012

Contributor

This should be removed and below use: \Exception.

@metaturso metaturso Minor changes to the EventDispatcherTest
 - Removed Exception class import
 - Added FQN at L60
 - Added documentation
 - Added @group event-dispatcher
3a31b59

This pull request passes (merged 3a31b59 into 799a478).

@stloyd stloyd commented on the diff May 17, 2012

src/Composer/Script/EventDispatcher.php
@@ -91,7 +91,15 @@ protected function doDispatch(Event $event)
throw new \UnexpectedValueException('Method '.$callable.' is not callable, can not call '.$event->getName().' script');
}
- $className::$methodName($event);
+ try {
+ $className::$methodName($event);
+ } catch (\Exception $e) {
+ $message = "%s terminated with an exception.\n[%s] %s";
+ throw new \RuntimeException(sprintf($message,
@stloyd

stloyd May 17, 2012

Contributor

IMO you should use same convention for this message like is already used in composer.

@metaturso

metaturso May 17, 2012

Contributor

@stloyd could you please give me a hint about Composer conventions? I haven't had the time to read thoroughly the docs and the code since I knew about this project two days ago (:

Can you suggest a message that adheres to Composer conventions? I'd very much like to add it ASAP.

@till

till May 18, 2012

Contributor

This looks good to me – I'd probably use PHP_EOL instead of \n – but besides it looks all OK? :)

Seldaek merged commit 3a31b59 into composer:master May 22, 2012

Owner

Seldaek commented May 22, 2012

Thanks, merged

Owner

Seldaek commented May 22, 2012

BTW I changed it to output some warning instead of throwing a new exception, because that removes the backtrace of the exception and hinders debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment