Fix EZP-27814: Support MySQL error codes in DB exceptions

[peterkeung]

Bug report: https://jira.ez.no/browse/EZP-27814

Since eZ Publish 4.5, we can catch DB exceptions: https://github.com/ezsystems/ezpublish-legacy/blob/master/doc/features/4.5/ezdb_exception_based_error_handling.txt

However, the error code in the DB exception is always 0. This pull request logs the MySQL error code in the exception code so that you can specifically catch error codes (such as deadlocks) and act accordingly (such as retrying the transaction).

One of the reasons this is important is that the legacy API is prone to DB deadlock errors on concurrent object creation.

You can reproduce this quite easily by running multiple versions of this script at the same time:

<?php
for( $i = 0; $i < 100; ++$i )
{
    $params = array();
    $params['class_identifier'] = 'folder';
    $params['parent_node_id'] = 62;
    $params['attributes'] = array( 'name' => 'Test folder script 1' );
 
    $contentObject = eZContentFunctions::createAndPublishObject( $params );
}

By adding the DB exception code, we can catch a deadlock error and retry the creation rather than having the script crash.

[andrerom]

break, could this be done another way?

[peterkeung]

What's here now is essentially a copy of ezcBaseException, but with support for the error code / number. Thus, the intention is that there is no BC break at all.

[emodric]

It's a break if someone typehinted against ezcBaseExtension.

Can you not implement a custom base exception (e.g. eZBaseException) extending from ezcBaseExtension with added features?

[pkamps]

It's unlikely somebody would typehint against 'ezcBaseException'. It's much more likely that somebody would typehint against 'eZDBException'

[emodric]

Sure, but that doesn't change the fact that it's still a BC break which IMO should not be made lightly.

[peterkeung]

OK, I've rewritten it so that we continue to extend ezcBaseException but just set the $message and $code properties, since the ezcBaseException constructor removed $code support.

[emodric]

This should probably be htmlspecialchars($message, ENT_QUOTES, 'UTF-8') to properly encode quotes and since UTF-8 is the default charset only since PHP 5.4.

[peterkeung]

It was originally just a copy of ezcBaseException, but I've made the change now :)

[emodric]

Apart from the comment about htmlspecialchars, this looks good 👍

[adamwojs]

+1

By adding the DB exception code, we can catch a deadlock error and retry the creation rather than having the script crash.

Maybe one sugestion for future improvment: I think that introduce const representing error codes, will allow to avoid hard coding values in error code checking. Maybe those const are already defined somewhere ?

[peterkeung]

About the constants, in the context of MySQL (https://dev.mysql.com/doc/refman/5.7/en/error-messages-server.html), I don't think there is such a list of them in PHP.

[moismailzai]

+1

[dfearnley]

+1

[ernestob]

This looks good 👍

[andrerom]

@peterkeung Didn't see this before after merge, but it might be a minor issue not to call parent::__construct( $message ) as Exception::__construct() then won't get called.

Would you be able to do a followup fix?

[peterkeung]

@andrerom That was the entire challenge of preserving the use of ezcBaseException (because with ezcBaseException as the parent, the $code cannot be set through the constructor), but I think we are good here, because:

• The exception is thrown
• You can catch eZDBException, ezcBaseException, or Exception
• By setting the $this->message and $this->code properties, we are making the necessary information available to be acted upon

Thus, we don't need to call parent::__construct( $message) unless I'm missing something?

[andrerom]

@peterkeung I was thinking about these parts:

		$this->file = __FILE__; // of throw clause
		$this->line = __LINE__; // of throw clause
		$this->trace = debug_backtrace();
		$this->string = StringFormat($this);

[peterkeung]

@andrerom (Disclaimer: this is beyond my exception knowledge, so I apologize if I have a fundamental misunderstanding.) In my tests, I still got the expected results with $e->getFile(), $e->getLine(), $e->getTrace(), $e->getTraceAsString() in the caught exception object. We couldn't just move those lines into eZDBException or we'd get wrong info.

[andrerom]

In my tests, I still got the expected results with $e->getFile(), $e->getLine(), $e->getTrace(), $e->getTraceAsString() in the caught exception object.

ok then we are good, my doubt was if these would be wrong now.

We couldn't just move those lines into eZDBException or we'd get wrong info.

was not implying that, only that we'd somehow call Exception::__construct().
But if the info is right, then no need. done.

[gggeek]

Nitpick: it is bad practice to html-encode data on data-creation. Data should be escaped when it is displayed.