BUG Debug error handler breaks error_get_last #2583

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Contributor

tractorcow commented Oct 22, 2013

I've tracked down a critical issue in Debug which makes error recovery impossible in CLI.

In certain situations it's necessary to have error recovery behaviour (through either register_shutdown_function or detecting errors with error_get_last). This is also critical to the successful recovery for ErrorControlChain, which relies on both to correctly respond to errors.

The problem is that error_get_last is documented as returning null if the error handler (set by set_error_handler, as the Debug class does) does not return false.

Checking Debug.php I find that fatalHandler does not return false in cli_mode. See https://github.com/silverstripe/silverstripe-framework/blob/3.1/dev/Debug.php#L338 and https://github.com/silverstripe/silverstripe-framework/blob/3.1/dev/Debug.php#L426.

Debug::showError doesn't return anything; which is not the same as returning false. This means that the error is printed out to the screen (which is great, thanks) but then error_get_last lies about there not being an error. That bastard!

I think this is a serious issue with 3.1 as this currently means that error recover is difficult without completely disabling the Debug error handler and rolling your own.

This fix adds a return false; to the end of Debug::showError, meaning that all error handling paths return false explicitly.

Can someone please explain if the current behaviour is in place intentionally? If so, can it please be explained how an error handled by fatalError in CLI be detected?

Contributor

tractorcow commented Oct 22, 2013

Also as an aside ,why doesn't ErrorControlChain work with exceptions? I had to do the following in my task code.

    public function run($request) {

        // Bootstrap import process
        $import = new Import();
        $import->write();
        $self = $this;

        $this->message('Beginning import chain');

        // Operate import in chain
        $chain = new ErrorControlChain();
        $chain
            ->then(function() use ($self, $import, $chain) {
                try {
                    $self->importFiles($import);
                } catch (Exception $ex) {
                    $self->setException($ex);
                    $chain->setErrored(true);
                }
            })
            ->thenIfErrored(function() use ($self, $import) {
                $self->failTask($import);
            })
            ->thenWhileGood(function() use ($self, $import) {
                $self->finishTask($import);
            })
            ->execute();

        // Continue to handle exceptions
        $chain->setErrored(false);
        if($ex = $this->getException()) {
            throw $ex;
        }
    }
Owner

chillu commented Oct 22, 2013

@hafriedlander I think this is one for you

simonwelsh added the 3.1 label Mar 15, 2014

Contributor

tractorcow commented Apr 6, 2014

@hafriedlander do you mind looking at this old bug? :)

tractorcow added the easy label Apr 28, 2014

Owner

hafriedlander commented Jul 5, 2014

Because noticeHandler calls showHandler, this patch would make notices start calling the underlying handler (i.e. spit out a warning into the page). Need to only return false in fatalHandler.

@tractorcow @tractorcow tractorcow BUG Debug error handler breaks error_get_last
193b7bc
Contributor

tractorcow commented Jul 6, 2014

noticeHandler calls ini_set('display_errors', 0); which should bypass that. Is that not sufficient? It is also returning false by default in live mode. If you want to change the framework so that error_get_lasts ONLY reports fatal errors, then you'll need to change all of these return falses. Otherwise we should probably be reasonably consistent between live and dev mode.

Contributor

tractorcow commented Sep 24, 2014

Ping!

Owner

wilr commented Feb 8, 2015

I agree, in live mode this returns false so consistency seems to trumps concerns. In 3 from 8534982

wilr closed this Feb 8, 2015

tractorcow deleted the tractorcow:pulls/3.1-fix-debug-error-handling branch Feb 8, 2015

Contributor

tractorcow commented Feb 8, 2015

Only took two years thanks :)

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