From cce411541811039e857ddacd2cbb37e68865db95 Mon Sep 17 00:00:00 2001
From: iglocska <andras.iklody@gmail.com>
Date: Thu, 28 Nov 2024 17:47:43 +0100
Subject: [PATCH] fix: [error handling] better error handling for bookmarks,
 fixes #188

- show why something failed
- actually fail if a field is missing for bookmarks
---
 src/Controller/Component/CRUDComponent.php    |  2 +-
 .../Component/RestResponseComponent.php       |  8 ++++++--
 src/Controller/UserSettingsController.php     |  5 +++--
 src/Model/Table/UserSettingsTable.php         | 19 +++++++++++++++++--
 4 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/src/Controller/Component/CRUDComponent.php b/src/Controller/Component/CRUDComponent.php
index e7092a1..ed374fe 100644
--- a/src/Controller/Component/CRUDComponent.php
+++ b/src/Controller/Component/CRUDComponent.php
@@ -1365,7 +1365,7 @@ class CRUDComponent extends Component
                 if (!empty($additionalData['redirect'])) { // If a redirection occurs, we need to make sure the flash message gets displayed
                     $this->Controller->Flash->error($message);
                 }
-                $this->Controller->ajaxResponsePayload = $this->RestResponse->ajaxFailResponse($this->ObjectAlias, $action, $data, $message, !is_null($errors) ? $errors : $data->getErrors());
+                $this->Controller->ajaxResponsePayload = $this->RestResponse->ajaxFailResponse($this->ObjectAlias, $action, $data, $message, $errors);
             } else {
                 $this->Controller->Flash->error($message);
                 $this->Controller->redirect($this->Controller->referer());
diff --git a/src/Controller/Component/RestResponseComponent.php b/src/Controller/Component/RestResponseComponent.php
index 899968c..ecaabba 100644
--- a/src/Controller/Component/RestResponseComponent.php
+++ b/src/Controller/Component/RestResponseComponent.php
@@ -440,10 +440,14 @@ class RestResponseComponent extends Component
         return $this->viewData($response);
     }
 
-    public function ajaxFailResponse($ObjectAlias, $action, $entity, $message, $errors = [], $description = '')
+    public function ajaxFailResponse($ObjectAlias, $action, $entity = null, $message, $errors = [], $description = '')
     {
         $action = $this->__dissectAdminRouting($action);
-        $entity = is_array($entity) ? $entity : $entity->toArray();
+        if (empty($entity)) {
+            $entity = [];
+        } else {
+            $entity = is_array($entity) ? $entity : $entity->toArray();
+        }
         $response = [
             'success' => false,
             'message' => $message,
diff --git a/src/Controller/UserSettingsController.php b/src/Controller/UserSettingsController.php
index d97dbe7..54e7e62 100644
--- a/src/Controller/UserSettingsController.php
+++ b/src/Controller/UserSettingsController.php
@@ -243,9 +243,10 @@ class UserSettingsController extends AppController
     public function saveMyBookmark()
     {
         if (!$this->request->is('get')) {
-            $result = $this->UserSettings->saveBookmark($this->ACL->getUser(), $this->request->getData());
+            $errors = null;
+            $result = $this->UserSettings->saveBookmark($this->ACL->getUser(), $this->request->getData(), $errors);
             $success = !empty($result);
-            $message = $success ? __('Bookmark saved') : __('Could not save bookmark');
+            $message = $success ? __('Bookmark saved') : ($errors ?? __('Could not save bookmark'));
             $this->CRUD->setResponseForController('saveBookmark', $success, $message, $result);
             $responsePayload = $this->CRUD->getResponsePayload();
             if (!empty($responsePayload)) {
diff --git a/src/Model/Table/UserSettingsTable.php b/src/Model/Table/UserSettingsTable.php
index 0fc1dcc..21c5a84 100644
--- a/src/Model/Table/UserSettingsTable.php
+++ b/src/Model/Table/UserSettingsTable.php
@@ -95,9 +95,24 @@ class UserSettingsTable extends AppTable
         return $savedData;
     }
 
-    public function saveBookmark($user, $data)
+    public function saveBookmark($user, $data, &$message = null)
     {
         $setting = $this->getSettingByName($user, $this->BOOKMARK_SETTING_NAME);
+        $fieldsToCheck = [
+            'bookmark_label' => __('Label'),
+            'bookmark_name' => __('Name'),
+            'bookmark_url' => __('URL')
+        ];
+        foreach ($fieldsToCheck as $field => $fieldName) {
+            if (empty($data[$field])) {
+                $message = __('Please fill in all fields, {0} missing.', $fieldName);
+                return null;
+            }
+        }
+        if (empty($data['bookmark_label']) || empty($data['bookmark_name']) || empty($data['bookmark_url'])) {
+
+            return null;
+        }
         $bookmarkData = [
             'label' => $data['bookmark_label'],
             'name' => $data['bookmark_name'],
@@ -107,7 +122,7 @@ class UserSettingsTable extends AppTable
         if (!empty($restricted_domains)) {
             $restricted_domains = explode(',', $restricted_domains);
             $parsed = parse_url($bookmarkData['url']);
-            if (!in_array($parsed['host'], $restricted_domains)) {
+            if (!empty($parsed['host']) && !in_array($parsed['host'], $restricted_domains)) {
                 return null;
             }
         }