[5.4] Error handling: Adding new shouldUseException() (#44098)

To make an easy transition from the legacy error handling with "getError" and "setError" methods, this PR introduces new methods "shouldUseExceptions" and "setUseExceptions" to the "LegacyErrorHandlingTrait". The latter allows to set a flag if exceptions should be used or not, and the former allows to check if that flag is set or not. The plan is to add this to 5.4 and then to remove all of this from 7.0 again.

Author: Hackwar

administrator/components/com_content/src/View/Articles/HtmlView.php

@@ -111,28 +111,28 @@ public function display($tpl = null)
     {
         /** @var ArticlesModel $model */
         $model = $this->getModel();
+        $model->setUseExceptions(true);
+
+        try {
+            $this->items         = $model->getItems();
+            $this->pagination    = $model->getPagination();
+            $this->state         = $model->getState();
+            $this->filterForm    = $model->getFilterForm();
+            $this->activeFilters = $model->getActiveFilters();
+            $this->vote          = PluginHelper::isEnabled('content', 'vote');
+            $this->hits          = ComponentHelper::getParams('com_content')->get('record_hits', 1) == 1;
+
+            if (!\count($this->items) && $this->isEmptyState = $model->getIsEmptyState()) {
+                $this->setLayout('emptystate');
+            }
 
-        $this->items         = $model->getItems();
-        $this->pagination    = $model->getPagination();
-        $this->state         = $model->getState();
-        $this->filterForm    = $model->getFilterForm();
-        $this->activeFilters = $model->getActiveFilters();
-        $this->vote          = PluginHelper::isEnabled('content', 'vote');
-        $this->hits          = ComponentHelper::getParams('com_content')->get('record_hits', 1) == 1;
-
-        if (!\count($this->items) && $this->isEmptyState = $model->getIsEmptyState()) {
-            $this->setLayout('emptystate');
-        }
-
-        if (ComponentHelper::getParams('com_content')->get('workflow_enabled')) {
-            PluginHelper::importPlugin('workflow');
-
-            $this->transitions = $model->getTransitions();
-        }
+            if (ComponentHelper::getParams('com_content')->get('workflow_enabled')) {
+                PluginHelper::importPlugin('workflow');
 
-        // Check for errors.
-        if (\count($errors = $model->getErrors()) || $this->transitions === false) {
-            throw new GenericDataException(implode("\n", $errors), 500);
+                $this->transitions = $model->getTransitions();
+            }
+        } catch (\Exception $e) {
+            throw new GenericDataException($e->getMessage(), 500, $e);
         }
 
         // We don't need toolbar in the modal window.

libraries/src/MVC/Model/BaseDatabaseModel.php

@@ -263,6 +263,10 @@ public function getTable($name = '', $prefix = '', $options = [])
         }
 
         if ($table = $this->_createTable($name, $prefix, $options)) {
+            if ($this->shouldUseExceptions()) {
+                $table->setUseExceptions(true);
+            }
+
             return $table;
         }
 

libraries/src/Object/LegacyErrorHandlingTrait.php

@@ -19,7 +19,7 @@
  *
  * @since       4.3.0
  *
- * @deprecated  4.3 will be removed in 6.0
+ * @deprecated  4.3 will be removed in 7.0
  *              Will be removed without replacement
  *              Throw an Exception instead of setError
  */
@@ -36,6 +36,15 @@ trait LegacyErrorHandlingTrait
     protected $_errors = [];
     // phpcs:enable PSR2.Classes.PropertyDeclaration
 
+    /**
+     * Use exceptions rather than getError/setError.
+     *
+     * @var          boolean
+     * @since        __DEPLOY_VERSION__
+     * @deprecated   7.0
+     */
+    private bool $useExceptions = false;
+
     /**
      * Get the most recent error message.
      *
@@ -46,7 +55,7 @@ trait LegacyErrorHandlingTrait
      *
      * @since   1.7.0
      *
-     * @deprecated  3.1.4 will be removed in 6.0
+     * @deprecated  3.1.4 will be removed in 7.0
      *              Will be removed without replacement
      *              Catch thrown Exceptions instead of getError
      */
@@ -78,7 +87,7 @@ public function getError($i = null, $toString = true)
      *
      * @since   1.7.0
      *
-     * @deprecated  3.1.4 will be removed in 6.0
+     * @deprecated  3.1.4 will be removed in 7.0
      *              Will be removed without replacement
      *              Catch thrown Exceptions instead of getErrors
      */
@@ -96,12 +105,44 @@ public function getErrors()
      *
      * @since   1.7.0
      *
-     * @deprecated  3.1.4 will be removed in 6.0
+     * @deprecated  3.1.4 will be removed in 7.0
      *              Will be removed without replacement
      *              Throw an Exception instead of using setError
      */
     public function setError($error)
     {
+        if ($this->useExceptions && \is_string($error)) {
+            throw new \Exception($error, 500);
+        }
+
         $this->_errors[] = $error;
     }
+
+    /**
+     * If true then subclasses should throw exceptions rather than use getError and setError.
+     *
+     * @return  boolean
+     *
+     * @since        __DEPLOY_VERSION__
+     * @deprecated   7.0
+     */
+    public function shouldUseExceptions(): bool
+    {
+        return $this->useExceptions;
+    }
+
+    /**
+     * If true then subclasses should throw exceptions rather than use getError and setError.
+     *
+     * @param   boolean   $value  The value to set for the field.
+     *
+     * @return  void
+     *
+     * @since        __DEPLOY_VERSION__
+     * @deprecated   7.0
+     */
+    public function setUseExceptions(bool $value): void
+    {
+        $this->useExceptions = $value;
+    }
 }

tests/Unit/Libraries/Cms/MVC/Model/FormModelTest.php

@@ -93,6 +93,7 @@ public function testFailedCheckin()
         $table->method('hasField')->willReturn(true);
         $table->method('checkIn')->willReturn(false);
         $table->method('getColumnAlias')->willReturn('checked_out');
+        $table->method('getError')->willReturn('ERROR MESSAGE');
 
         $mvcFactory = $this->createStub(MVCFactoryInterface::class);
         $mvcFactory->method('createTable')->willReturn($table);
@@ -106,6 +107,10 @@ public function getForm($data = [], $loadData = true)
         $model->setCurrentUser(new User());
 
         $this->assertFalse($model->checkin(1));
+
+        $this->expectException(\Exception::class);
+        $model->setUseExceptions(true);
+        $model->checkin(1);
     }
 
     /**
@@ -298,6 +303,7 @@ public function testFailedCheckout()
         $table->method('hasField')->willReturn(true);
         $table->method('checkIn')->willReturn(false);
         $table->method('getColumnAlias')->willReturn('checked_out');
+        $table->method('getError')->willReturn('ERROR MESSAGE');
 
         $mvcFactory = $this->createStub(MVCFactoryInterface::class);
         $mvcFactory->method('createTable')->willReturn($table);
@@ -315,6 +321,10 @@ public function getForm($data = [], $loadData = true)
         $model->setCurrentUser($user);
 
         $this->assertFalse($model->checkout(1));
+
+        $this->expectException(\Exception::class);
+        $model->setUseExceptions(true);
+        $model->checkout(1);
     }
 
     /**

tests/Unit/Libraries/Cms/Object/CMSObjectTest.php

@@ -102,6 +102,7 @@ public function testGetProperties()
                 '_privateproperty1' => 'valuep1',
                 'property1'         => 'value1',
                 'property2'         => 5,
+                'useExceptions'     => false,
             ],
             $object->getProperties(false),
             'Should get all properties, including private ones'