Skip to content

Commit

Permalink
allow to pass SystemFacade to the constructor
Browse files Browse the repository at this point in the history
  • Loading branch information
oscarotero committed Jun 21, 2017
1 parent bb329be commit 99bab7b
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 21 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

## 0.4.1 - UNRELEASED

### Fixed

* Fixed shutdown errors handling

## [0.4.0]

### Added
Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ $response = $dispatcher->dispatch(new ServerRequest());

## Options

#### `__construct(Whoops\Run $whoops = null)`
#### `__construct(Whoops\Run $whoops = null, Whoops\Util\SystemFacade $system = null)`

Allows to provide a custom `Whoops\Run` instance. If it's not defined, creates an instance automatically.
Allows to provide a custom `Whoops\Run` instance. If it's not defined, creates an instance automatically. You can provide also the `SystemFacade` used by the `Run` instance, in order to implement a special behaviour with fatal errors.

#### `catchErrors(true)`

Expand Down
45 changes: 29 additions & 16 deletions src/Whoops.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ class Whoops implements MiddlewareInterface
*/
private $whoops;

/**
* @var SystemFacade|null
*/
private $system;

/**
* @var bool Whether catch errors or not
*/
Expand All @@ -29,10 +34,12 @@ class Whoops implements MiddlewareInterface
* Set the whoops instance.
*
* @param Run|null $whoops
* @param SystemFacade|null $systemFacade
*/
public function __construct(Run $whoops = null)
public function __construct(Run $whoops = null, SystemFacade $system = null)
{
$this->whoops = $whoops;
$this->system = $system;
}

/**
Expand Down Expand Up @@ -63,7 +70,7 @@ public function process(ServerRequestInterface $request, DelegateInterface $dele
$level = ob_get_level();

$method = Run::EXCEPTION_HANDLER;
$whoops = $this->whoops ?: self::getWhoopsInstance($request);
$whoops = $this->whoops ?: $this->getWhoopsInstance($request);

$whoops->allowQuit(false);
$whoops->writeToOutput(false);
Expand All @@ -72,6 +79,21 @@ public function process(ServerRequestInterface $request, DelegateInterface $dele
//Catch errors means register whoops globally
if ($this->catchErrors) {
$whoops->register();

$shutdown = function () use ($whoops) {
$whoops->allowQuit(true);
$whoops->writeToOutput(true);
$whoops->sendHttpCode(true);

This comment has been minimized.

Copy link
@shadowhand

shadowhand Jun 22, 2017

Member

I think allowQuit and sendHttpCode would still need to be false to properly capture the error, no?

This comment has been minimized.

Copy link
@oscarotero

oscarotero Jun 22, 2017

Author Member

The error cannot be captured (or at least I was unable to capture it). Fatal errors are a special case in which the exceptions are not propagated so cannot be captured in the try/catch of the middleware. This is just a way to, at least, show something.

This comment has been minimized.

Copy link
@shadowhand

shadowhand Jun 22, 2017

Member

Couldn't it be "caught" by capturing the output buffer?

This comment has been minimized.

Copy link
@oscarotero

oscarotero Jun 22, 2017

Author Member

No. handleShutdown() returns nothing, and there's no way to capture the output because all buffers are closed before send the new output.
If you have any better idea, feel free to work in a pull request.

This comment has been minimized.

Copy link
@shadowhand

shadowhand Jun 22, 2017

Member

Dang. Well I guess this is fine. The only reasonable solution involves requiring PHP 7, in which case the current master code will work just fine.

this is fine.

This comment has been minimized.

Copy link
@oscarotero

oscarotero Jun 26, 2017

Author Member

FYI, my intention is to release the 1.x version when PSR-15 is accepted and then, a 2.x version supporting only php7. You can contribute in 2.0 with that better solution.


$method = Run::SHUTDOWN_HANDLER;
$whoops->$method();
};

if ($this->system) {
$this->system->registerShutdownFunction($shutdown);
} else {
register_shutdown_function($shutdown);
}
}

try {
Expand Down Expand Up @@ -104,23 +126,14 @@ public function process(ServerRequestInterface $request, DelegateInterface $dele
*
* @return Run
*/
private static function getWhoopsInstance(ServerRequestInterface $request)
private function getWhoopsInstance(ServerRequestInterface $request)
{
$system = new SystemFacade();
$whoops = new Run($system);

//E_ERROR in PHP 5.x
if (!class_exists('Throwable')) {
$system->registerShutdownFunction(function () use ($whoops) {
$whoops->allowQuit(true);
$whoops->writeToOutput(true);
$whoops->sendHttpCode(true);

$method = Run::SHUTDOWN_HANDLER;
$whoops->$method();
});
if (!$this->system) {
$this->system = new SystemFacade();
}

$whoops = new Run($this->system);

switch (self::getPreferredFormat($request)) {
case 'json':
$handler = new JsonResponseHandler();
Expand Down
6 changes: 3 additions & 3 deletions tests/WhoopsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,18 @@ function () {
$this->assertNotFalse(strpos($response->getBody(), 'Error Processing Request'));
}

public function testFatalError()
public function testStandardError()
{
$response = Dispatcher::run([
new Whoops(),
function () {
new UnexistingClass();
$a = $b; //undefined variable
},
]);

$this->assertEquals(500, $response->getStatusCode());
$this->assertEquals('text/plain', $response->getHeaderLine('Content-Type'));
$this->assertNotFalse(strpos($response->getBody(), "Class 'Middlewares\\Tests\\UnexistingClass' not found"));
$this->assertNotFalse(strpos($response->getBody(), 'Undefined variable: b'));
}

public function testNotError()
Expand Down

1 comment on commit 99bab7b

@oscarotero
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shadowhand Implemented the option "b" (passing the SystemFacade to the constructor) and option "c" (execute register_shutdown_function directly) as fallback.

Please sign in to comment.