[5.3] Error handling: Adding new shouldUseException()

[Hackwar]

Summary of Changes

Since forever we have the legacy error handling with getError() and setError(), implemented with the LegacyErrorHandlingTrait and since forever this has actually been wrong, since the right solution would be exceptions. In an easy world we would simply switch all code over from the legacy method to using exceptions in the next major version, but since that breaks basically every website out there, we need a transition period here. That is the reason for this more complex solution.

shouldUseException()

To make an easy transition, this PR introduces new methods to the LegacyErrorHandlingTrait, which allows to set a flag if exceptions should be used or not. The plan is to add this to 5.3 and then to remove all of this from 7.0 again.

Changed error handling

This transition method requires more code right now. Old error handling often enough looked like this:

        if ($errorConditionEvaluatingToFalse) {
            $this->setError($table->getError());

            return false;
        }

The new code would look similar to this:

        if ($errorConditionEvaluatingToFalse) {
            if ($this->shouldUseExceptions()) {
                throw new \Exception($table->getError());
            }

            $this->setError($table->getError());

            return false;
        }

To improve the transition from the old setError() to throwing exceptions, this is also introduces a change to setError(). setError() throws an exception as well when the setUseExceptions() is set to true. The longterm goal is to rewrite all the code from using the legacy error handling to using exceptions and when the major version to switch this around has come, we only need to delete lines instead of coming up with new code to have the right exceptions. In the end it should look like this:

        if ($errorConditionEvaluatingToFalse) {
            throw new \Exception($table->getError());
        }

Using the new exception handling

The new error handling would have to be enabled by the code calling the model/table. In most cases this would be the view. The code right now looks for example like this:

        $this->item  = $this->get('Item');
        $this->print = $app->getInput()->getBool('print', false);
        $this->state = $this->get('State');
        $this->user  = $user;

        // Check for errors.
        if (\count($errors = $this->get('Errors'))) {
            throw new GenericDataException(implode("\n", $errors), 500);
        }

and would have to be refactored like this in the future:

        $model = $this->getModel();
        $model->setUseException(true);
        $this->item  = $this->get('Item');
        $this->print = $app->getInput()->getBool('print', false);
        $this->state = $this->get('State');
        $this->user  = $user;

Third party code

Third party extensions can include the trait and then call the $this->setUseException(true); in their constructor in order to force the exception behavior for the base core classes. In the rest of their code, they can directly use exception handling and thus be done with this basically with one refactoring.

Future plans on how to remove the trait

As explained, we want to remove the whole legacy error handling at some point. The proposal would be to allow third party developers to migrate to exception error handling between 5.3 and 7.0 with the help of this code. At 7.0 we would be removing the legacy error handling mostly. The LegacyErrorHandlingTrait would be changed to only be a placeholder which doesn't do anything. That means that in terms of error handling an extension coded properly for 6.0 will also work in 7.0 and only in 8.0 you would have to have the call to $model->setUseException() removed.
This is only a proposal to make it easier, but we could also just remove this right away with 7.0...

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Link to documentations

Please select:

• Documentation link for docs.joomla.org:

• No documentation changes for docs.joomla.org needed

• Pull Request link for manual.joomla.org:

• No documentation changes for manual.joomla.org needed

[laoneo]

What would also be good if you can change one occurrence in a core component to this. So extension developers get an idea what to do.

[Hackwar]

We talked about this in the last maintainers meeting and I'm going to refactor this in another way. Please give me some time. I will probably create a new PR.

[Hackwar]

This is now ready. One way to test the difference would be to intentionally break the query in the model and then see the differing behavior for before and after this PR. This is only one example on how to do this. Refactoring all core code would have to come later.