Feature RTC Unit/System Tests PR-5.4-dev Pending

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
17 Sep 2024

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.4 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.4 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:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org: joomla/Manual#455

  • No documentation changes for manual.joomla.org needed

avatar Hackwar Hackwar - open - 17 Sep 2024
avatar Hackwar Hackwar - change - 17 Sep 2024
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 17 Sep 2024
Category Libraries
avatar laoneo
laoneo - comment - 4 Oct 2024

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.

avatar Hackwar
Hackwar - comment - 4 Oct 2024

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.

avatar Hackwar Hackwar - change - 4 Oct 2024
Labels Added: PR-5.3-dev
avatar joomla-cms-bot joomla-cms-bot - change - 4 Oct 2024
Category Libraries Administration com_content Libraries
avatar Hackwar Hackwar - change - 4 Oct 2024
Title
[5.3] Error handling: Adding new ExceptionTrait
[5.3] Error handling: Adding new shouldUseException()
avatar Hackwar Hackwar - edited - 4 Oct 2024
avatar Hackwar Hackwar - change - 4 Oct 2024
The description was changed
avatar Hackwar Hackwar - edited - 4 Oct 2024
avatar joomla-cms-bot joomla-cms-bot - change - 4 Oct 2024
Category Libraries Administration com_content Administration com_content Libraries Unit Tests
avatar Hackwar
Hackwar - comment - 5 Oct 2024

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.

avatar HLeithner HLeithner - change - 31 Oct 2024
Labels Added: Unit/System Tests
avatar richard67
richard67 - comment - 5 Feb 2025

@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?

avatar HLeithner
HLeithner - comment - 4 Mar 2025

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

avatar HLeithner HLeithner - change - 4 Mar 2025
Title
[5.3] Error handling: Adding new shouldUseException()
[6.0] Error handling: Adding new shouldUseException()
avatar HLeithner HLeithner - edited - 4 Mar 2025
avatar Hackwar Hackwar - change - 29 Apr 2025
Labels Added: Feature PR-6.0-dev
Removed: PR-5.3-dev
avatar Hackwar Hackwar - change - 30 Apr 2025
Title
[6.0] Error handling: Adding new shouldUseException()
[5.4] Error handling: Adding new shouldUseException()
avatar Hackwar Hackwar - edited - 30 Apr 2025
avatar Hackwar Hackwar - change - 30 Apr 2025
Labels Added: PR-5.4-dev
avatar Hackwar Hackwar - change - 2 May 2025
The description was changed
avatar Hackwar Hackwar - edited - 2 May 2025
avatar Hackwar Hackwar - change - 2 May 2025
The description was changed
avatar Hackwar Hackwar - edited - 2 May 2025
avatar richard67 richard67 - change - 11 May 2025
Labels Removed: PR-6.0-dev
avatar richard67 richard67 - change - 11 May 2025
Build 5.3-dev 5.4-dev
avatar exlemor exlemor - test_item - 18 Jun 2025 - Tested successfully
avatar exlemor
exlemor - comment - 18 Jun 2025

I have tested this item ✅ successfully on 9643c10

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.

avatar dautrich dautrich - test_item - 19 Jun 2025 - Tested successfully
avatar dautrich
dautrich - comment - 19 Jun 2025

I have tested this item ✅ successfully on 9643c10


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

avatar alikon alikon - change - 19 Jun 2025
Status Pending Ready to Commit
avatar alikon
alikon - comment - 19 Jun 2025

RTC


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

avatar richard67 richard67 - change - 28 Jun 2025
The description was changed
avatar richard67 richard67 - edited - 28 Jun 2025
avatar richard67 richard67 - change - 28 Jun 2025
Labels Added: RTC
avatar richard67 richard67 - change - 28 Jun 2025
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2025-06-28 09:49:44
Closed_By richard67
avatar richard67 richard67 - close - 28 Jun 2025
avatar richard67 richard67 - merge - 28 Jun 2025
avatar richard67
richard67 - comment - 28 Jun 2025

Thanks all (author and testers).

Add a Comment

Login with GitHub to post a comment