ruckus / ruckusing-migrations

Database migrations for PHP ala ActiveRecord Migrations with support for MySQL, Postgres, SQLite
Other
506 stars 95 forks source link

Exception handlers set no exit code #95

Open woru opened 11 years ago

woru commented 11 years ago

Exception handlers set no exit code, which causes problems when migrations are called from bash scripts.

Fix:

Index: ruckusing-migrations/lib/Ruckusing/Exception.php
===================================================================
--- ruckusing-migrations/lib/Ruckusing/Exception.php    (revision 6344)
+++ ruckusing-migrations/lib/Ruckusing/Exception.php    (working copy)
@@ -79,6 +79,7 @@
     public static function errorHandler($code, $message, $file, $line)
     {
         file_put_contents('php://stderr', "\n" . basename($file) . "({$line}) : {$message}\n\n");
+        die(1);
     }

     /**
@@ -89,6 +90,7 @@
     public static function exceptionHandler($exception)
     {
         file_put_contents('php://stderr', "\n" . basename($exception->getFile()) . "({$exception->getLine()}) : {$exception->getMessage()}\n\n");
+        die(1);
     }

 }
ruckus commented 11 years ago

Very good point. Thank you for the heads up!

On Mar 20, 2013, at 8:01 AM, woru notifications@github.com wrote:

Exception handlers set no exit code, which causes problems when migrations are called from bash scripts.

Fix:

Index: ruckusing-migrations/lib/Ruckusing/Exception.php

--- ruckusing-migrations/lib/Ruckusing/Exception.php (revision 6344) +++ ruckusing-migrations/lib/Ruckusing/Exception.php (working copy) @@ -79,6 +79,7 @@ public static function errorHandler($code, $message, $file, $line) { file_put_contents('php://stderr', "\n" . basename($file) . "({$line}) : {$message}\n\n");

  •  die(1);

    }

    /** @@ -89,6 +90,7 @@ public static function exceptionHandler($exception) { file_put_contents('php://stderr', "\n" . basename($exception->getFile()) . "({$exception->getLine()}) : {$exception->getMessage()}\n\n");

  •  die(1);

    }

    } — Reply to this email directly or view it on GitHub.

salimane commented 11 years ago

Please what was this trying to fix exactly ? can we have a way to reproduce this ? This changes introduces #96 . Thanks

woru commented 11 years ago

'success' is displayed even though migrations failed

salimane commented 11 years ago

Here is a sample code I tested with pure php

salimane at salimane-zenbook  in ~
⚛ cat a.php                 
<?php

echo a;

salimane at salimane-zenbook  in ~
⚛ php a.php                 

Notice: Use of undefined constant a - assumed 'a' in /home/salimane/a.php on line 3

Call Stack:
    0.0226     373800   1. {main}() /home/salimane/a.php:0

PHP Notice:  Use of undefined constant a - assumed 'a' in /home/salimane/a.php on line 3
PHP Stack trace:
PHP   1. {main}() /home/salimane/a.php:0

salimane at salimane-zenbook  in ~
⚛ php a.php && echo "success"

Notice: Use of undefined constant a - assumed 'a' in /home/salimane/a.php on line 3

Call Stack:
    0.0228     373800   1. {main}() /home/salimane/a.php:0

PHP Notice:  Use of undefined constant a - assumed 'a' in /home/salimane/a.php on line 3
PHP Stack trace:
PHP   1. {main}() /home/salimane/a.php:0
success

salimane at salimane-zenbook  in ~
⚛ 

Pure php is also showing that behavior.

Thanks

woru commented 11 years ago

You example is invalid. Undefined variable results in a notice not error so result code 'success' is acceptable. Try: a.php: <?php throw new Exception("a");

$ php a.php; echo $?

returns error code 255 != 0 so it works as expected.

I would add a check: if ($code != E_WARNING && $code != E_NOTICE) { exit(1); } in error handler and it should be fine (at least as far as the issue #96 is concerned)

salimane commented 11 years ago

please could you send a pull request.. Thanks