[5.3] Error handling: Adding new ExceptionTrait

[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 horribly complex solution.

ExceptionTrait

To make an easy transition, this PR introduces a new trait, which allows to set a flag if exceptions should be used or not. This trait can be used in all classes which also use the LegacyErrorHandlingTrait. 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 a lot 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 (is_callable([$this, 'shouldUseExceptions']) && $this->shouldUseExceptions()) {
                throw new \Exception($table->getError());
            }

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

            return false;
        }

We could of course add some code to the ExceptionTrait to simply replace the setError(), which automatically either sets the error or throws an exception. However that means that we are still in the same situation as before, that we have to refactor all error handling to use exceptions the moment we want to remove the trait again, with the difference that now we have one more trait to remove than before. d'oh.
So instead we use this more complex code and when we are finally at the point where we want to remove all the legacy error handling, we only have to look for all occurences of calls to the ExceptionTrait and delete the code around the exception. The above code would then look like this:

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

Another example would be this example of the refactored code from AdminModel::save():

        // Allow an exception to be thrown.
        try {
            // Load the row if saving an existing record.
            if ($pk > 0) {
                $table->load($pk);
                $isNew = false;
            }

            // Bind the data.
            if (!$table->bind($data)) {
                if (is_callable([$this, 'shouldUseExceptions']) && $this->shouldUseExceptions()) {
                    throw new \Exception($table->getError());
                }

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

                return false;
            }

            // Prepare the row for saving
            $this->prepareTable($table);

            // Check the data.
            if (!$table->check()) {
                if (is_callable([$this, 'shouldUseExceptions']) && $this->shouldUseExceptions()) {
                    throw new \Exception($table->getError());
                }

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

                return false;
            }

            // Trigger the before save event.
            $beforeSaveEvent = new Model\BeforeSaveEvent($this->event_before_save, [
                'context' => $context,
                'subject' => $table,
                'isNew'   => $isNew,
                'data'    => $data,
            ]);
            $result = $dispatcher->dispatch($this->event_before_save, $beforeSaveEvent)->getArgument('result', []);

            if (\in_array(false, $result, true)) {
                if (is_callable([$this, 'shouldUseExceptions']) && $this->shouldUseExceptions()) {
                    throw new \Exception($table->getError());
                }

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

                return false;
            }

            // Store the data.
            if (!$table->store()) {
                if (is_callable([$this, 'shouldUseExceptions']) && $this->shouldUseExceptions()) {
                    throw new \Exception($table->getError());
                }

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

                return false;
            }

            // Clean the cache.
            $this->cleanCache();

            // Trigger the after save event.
            $dispatcher->dispatch($this->event_after_save, new Model\AfterSaveEvent($this->event_after_save, [
                'context' => $context,
                'subject' => $table,
                'isNew'   => $isNew,
                'data'    => $data,
            ]));
        } catch (\Exception $e) {
            if (is_callable([$this, 'shouldUseExceptions']) && $this->shouldUseExceptions()) {
                throw $e;
            }

            $this->setError($e->getMessage());

            return false;
        }

This would shrink to

        // Allow an exception to be thrown.
        try {
            // Load the row if saving an existing record.
            if ($pk > 0) {
                $table->load($pk);
                $isNew = false;
            }

            // Bind the data.
            $table->bind($data);

            // Prepare the row for saving
            $this->prepareTable($table);

            // Check the data.
            $table->check();

            // Trigger the before save event.
            $beforeSaveEvent = new Model\BeforeSaveEvent($this->event_before_save, [
                'context' => $context,
                'subject' => $table,
                'isNew'   => $isNew,
                'data'    => $data,
            ]);
            $result = $dispatcher->dispatch($this->event_before_save, $beforeSaveEvent)->getArgument('result', []);

            if (\in_array(false, $result, true)) {
                throw new \Exception($table->getError());
            }

            // Store the data.
            $table->store();

            // Clean the cache.
            $this->cleanCache();

            // Trigger the after save event.
            $dispatcher->dispatch($this->event_after_save, new Model\AfterSaveEvent($this->event_after_save, [
                'context' => $context,
                'subject' => $table,
                'isNew'   => $isNew,
                'data'    => $data,
            ]));
        } catch (\Exception $e) {
            throw $e;
        }

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;

Inheritance of the exception setting

Since we have the legacy error handling not only in the models, but also in the tables, the question is how to migrate both and how to set that correctly. The solution is to have the object created inside another object to inherit the setting from the parent. In reality in most cases this would be the table object in the model. For that, the getTable() method checks if the table has the setUseException() method available and then hands in the setting from its own class. The way the code in this PR is written, this even makes it possible to only convert the model and defer the conversion of the table to later. If you look at the models in this PR, you can see that the model keeps the legacy error handling of the table and converts it to exceptions. At the same time, when the table also supports the exception method, the exceptions from the table are simply passed through the model to the calling code. This is in line with how it is now, except that right now we are manually handing the error from the table to the model to...

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, which would also include this ExceptionTrait. 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 the ExceptionTrait. At 7.0 we would be removing the legacy error handling, but keep the ExceptionTrait. The ExceptionTrait 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 about moving the logic into the already existing LegacyErrorHandlingTrait?

[Hackwar]

I decided against that, since I didn't want to push this on all existing code. If we put this in the existing trait or add this trait to the base model class, you can't differentiate between code which has been actively updated to run with exceptions or code which just inherited this by accident. That is why I have all the checks for the $this->shouldUseException() method.