Fix EZP-23913: Override legacy fatal error message

[thiagocamposviana]

The message "Fatal error: The web server did not finish its request" cannot be overridden in eZ Publish:

https://github.com/ezsystems/ezpublish-legacy/blob/master/kernel/private/classes/ezpkernelweb.php#L213

See more at: https://jira.ez.no/browse/EZP-23913

This pull request allows this to be overridden, the developer can now define a error.ini (only in global escope, ie, settings/override/error.ini.append.php) variable like:

[ErrorSettings-kernel]
FatalErrorHandler=CustomFatalErrorHandlerClass::FatalErrorHandlerFunction

[thiagocamposviana]

If you see https://github.com/ezsystems/ezpublish-legacy/pull/1138/files?diff=split

You will notice that I just create one if statement that checks for the existence of such variable.

[peterkeung]

No BC break

+1

[brookinsconsulting]

+1 We love this solution!

[lendormi]

+1

[trolek]

Shouldn't you also examine the setting for is_callable?

[yannickroger]

Missing empty line at the end of the file.

[yannickroger]

+1

[crevillo]

just a thought. as long as we already have a dedicate error.ini file, couldn't we move that setting there too?

[yannickroger]

@crevillo sounds like a good idea

[thiagocamposviana]

I have updated the code and now it should a setting defined in the error.ini and also check if the method is callable.

[crevillo]

nitpick: space after the if

[thiagocamposviana]

Hi Carlos, I am not sure about what you mean, there is no trailing spaces there.

[crevillo]

i'm sayin that the line should be like if ( instead of if(

[thiagocamposviana]

Ok, thanks.

[thiagocamposviana]

Updated code, fixed if statement.

[crevillo]

+1. useful thing!.

[yannickroger]

ping @andrerom @bdunogier @lolautruche @glye

[andrerom]

+/-0 this won't work with siteaccess settings, will only be global, because of how early this is registered (it has to be early, so I'm not saying it should be moved).

[thiagocamposviana]

@andrerom

Do you think it should be necessary to put a note in the .ini comment? Something like:

# Allow the default eZ Publish 500 error message to be overridden
# Note: this won't work with siteaccess settings, will only be global, because of how early this is registered
# FatalErrorHandler=CustomFatalErrorHandlerClass::FatalErrorHandlerFunction

[andrerom]

Something like that would help a bit.

[thiagocamposviana]

I confirmed the limitation and put a note about it.

[thiagocamposviana]

I am not sure why CI build failed, the current code shouldn't be executed at all because it will require that the ini variable to be defined and also the class::method exists, also about the code itself, I checked eZExecution and it adds the fatal error handlers like this:

static function addFatalErrorHandler( $handler )
    {
        self::registerShutdownHandler();
        self::$fatalErrorHandlers[] = $handler;
    }

And call them like this:

$handlers = eZExecution::fatalErrorHandlers();
        foreach ( $handlers as $handler )
        {
            if ( is_callable( $handler ) )
                call_user_func( $handler );
            else

                eZDebug::writeError('Could not call fatal error handler, is it a static public function?', __METHOD__ );
        }

So, it should work, as long as I am passing a string and call_user_func accepts strings.

[thiagocamposviana]

Checked https://travis-ci.org/ezsystems/ezpublish-legacy/jobs/47844272

The command "php -d date.timezone=$TEST_TIMEZONE tests/runtests.php --dsn "$DB://${DB_USER}@127.0.0.1/$DB_NAME" tests extension/ezoe" exited with 1.

Seems like an unrelated error is affecting the Travis CI build?

[bdunogier]

+1 with the principle since it is backward compatible. We could even have a default implement in the LegacyBundle ;-)

Nitpick: given that the modified code doesn't use any variable from the laaaarge constructor's scope, you could move this to a protected method. It wouldn't hurt, and would make this step self-contained.

[thiagocamposviana]

Hi Bertrand

Do you recommend a name for the method?

[thiagocamposviana]

I have updated the code and moved it to its own method.

[bdunogier]

Do you recommend a name for the method?

registerFatalErrorHandler ?

[thiagocamposviana]

I have seem that setupFatalErrorHandler might be a better option because I have seen some code using setup, is this pull request good enough to be merged?

[yannickroger]

How do we handle improvement PR on legacy now ? ping @andrerom @bdunogier

[brookinsconsulting]

We on the 'eZ Publish Pull Request Team' want this feature merged so it can be included in the next 'eZ Publish 5.x Community Build' (which is being driven now by community support). So we are strongly compelled to encourage this legacy improvement PR from mugo.ca to be merged. We can't 👍 enough this PR!

[yannickroger]

So if everybody is still +1, I'll merge it ;)

[peterkeung]

+1 here

[brookinsconsulting]

+1 Yes please and thank you :)

[bdunogier]

I don't see anything wrong with it. What do you think, @andrerom ?

[yannickroger]

Thanks @thiagocamposviana