[5.4] 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

For simplicity to test this and to show how this already improves existing error handling, please test this in the backend list view for articles.

Trigger an error by modifying the SQL query in the Articles model. For this, edit administrator/components/com_content/src/Model/ArticlesModel.php and look around line 210. You can see all the different fields that the model is reading from the database. Modify a random field by adding or removing a character. This makes the SQL query invalid and when now visiting the Content > Articles view in the backend, you get an error.

Actual result BEFORE applying this Pull Request

You get an error message like count(): Argument #1 ($value) must be of type Countable|array, bool given, which is not really helpfull...

Expected result AFTER applying this Pull Request

You get an error message like Unknown column '***' in 'SELECT' , which shows that there is an error in your SQL.

You can also switch between the two behaviors by editing line 114 in administrator/components/com_content/src/View/Articles/HtmlView.php and switching the $model->setUseExceptions(true); from true to false.

Link to documentations

Please select:

• [ ] Documentation link for docs.joomla.org:

• [X] No documentation changes for docs.joomla.org needed

• [X] Pull Request link for manual.joomla.org: https://github.com/joomla/Manual/pull/455

• [ ] 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.

[richard67]

@Hackwar Could this be extended by unit tests so we have unit tests for both cases, with the flag off and with the flag on?

[HLeithner]

This pull request has been automatically rebased to 6.0-dev.

[exlemor]

I have tested this item :white_check_mark: successfully on 9643c10734a0a59a84b9ce3d9e51ff78cb8d712c

I have successfully tested this! :) cool.

Thanks @Hackwar

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44098.}

[dautrich]

I have tested this item :white_check_mark: successfully on 9643c10734a0a59a84b9ce3d9e51ff78cb8d712c

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44098.}

[alikon]

RTC

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44098.}

[richard67]

Thanks all (author and testers).