Merge pull request #12125 from craftcms/feature/dev-212-errors-summary

Feature/dev 212 errors summary
craftcms · Aug 9, 2023 · 3a86b7e · 3a86b7e
2 parents 6c9f737 + 1c2368b
commit 3a86b7e
Show file tree

Hide file tree

Showing 22 changed files with 387 additions and 23 deletions.
diff --git a/CHANGELOG-WIP.md b/CHANGELOG-WIP.md
@@ -3,6 +3,7 @@
 ### Content Management
 - Entry and category edit pages now show other authors who are currently editing the same element. ([#13420](https://github.com/craftcms/cms/pull/13420))
 - Entry and category edit pages now display a notification when the element has been saved by another author. ([#13420](https://github.com/craftcms/cms/pull/13420))
+- Entry and category edit pages now display a validation error summary at the top of the page, including a mention of errors from other sites. ([#11569](https://github.com/craftcms/cms/issues/11569), [#12125](https://github.com/craftcms/cms/pull/12125))
 - Table fields can now have a “Row heading” column. ([#13231](https://github.com/craftcms/cms/pull/13231))
 - Table fields now have a “Static Rows” setting. ([#13231](https://github.com/craftcms/cms/pull/13231))
 - Table fields no longer show a heading row, if all heading values are blank. ([#13231](https://github.com/craftcms/cms/pull/13231))
@@ -94,6 +95,7 @@
 - Added `craft\services\Structures::EVENT_BEFORE_INSERT_ELEMENT`. ([#13429](https://github.com/craftcms/cms/pull/13429))
 - Added `craft\web\Controller::EVENT_DEFINE_BEHAVIORS`. ([#13477](https://github.com/craftcms/cms/pull/13477))
 - Added `craft\web\Controller::defineBehaviors()`. ([#13477](https://github.com/craftcms/cms/pull/13477))
+- Added `craft\web\CpScreenResponseBehavior::$errorSummary`, `errorSummary()`, and `errorSummaryTemplate()`. ([#12125](https://github.com/craftcms/cms/pull/12125))
 - Added `craft\web\CpScreenResponseBehavior::$pageSidebar`, `pageSidebar()`, and `pageSidebarTemplate()`. ([#13019](https://github.com/craftcms/cms/pull/13019), [#12795](https://github.com/craftcms/cms/issues/12795))
 - Added `craft\web\CpScreenResponseBehavior::$slideoutBodyClass`.
 - `craft\helpers\Cp::selectizeFieldHtml()`, `selectizeHtml()`, and `_includes/forms/selectize.twig` now support a `multi` param. ([#13176](https://github.com/craftcms/cms/pull/13176))

diff --git a/src/controllers/ElementsController.php b/src/controllers/ElementsController.php
@@ -32,6 +32,7 @@
 use craft\web\CpScreenResponseBehavior;
 use craft\web\View;
 use Throwable;
+use yii\helpers\Markdown;
 use yii\web\BadRequestHttpException;
 use yii\web\ForbiddenHttpException;
 use yii\web\Response;
@@ -395,6 +396,7 @@ public function actionEdit(?ElementInterface $element, ?int $elementId = null):
                 $isDraft
             ))
             ->notice($element->isProvisionalDraft ? fn() => $this->_draftNotice() : null)
+            ->errorSummary(fn() => $this->_errorSummary($element))
             ->prepareScreen(
                 fn(Response $response, string $containerId) => $this->_prepareEditor(
                     $element,
@@ -869,6 +871,86 @@ private function _editorContent(
         return trim($html);
     }
 
+    /**
+     * Return html for errors summary box
+     *
+     * @param ElementInterface $element
+     * @return string
+     */
+    private function _errorSummary(ElementInterface $element): string
+    {
+        $html = '';
+
+        if ($element->hasErrors()) {
+            $allErrors = $element->getErrors();
+            $allKeys = array_keys($allErrors);
+
+            // only show "top-level" errors
+            // if you e.g. have an assets field which is set to validate related assets,
+            // you should only see the top-level "Fix validation errors on the related asset" error
+            // and not the details of what's wrong with the selected asset;
+            foreach ($allKeys as $key) {
+                $lastNestedKey = substr_replace($key, '', strrpos($key, '.'));
+                $lastNestedKey = substr_replace($lastNestedKey, '', strrpos($lastNestedKey, '['));
+                if (!empty($lastNestedKey)) {
+                    if (in_array($lastNestedKey, $allKeys)) {
+                        unset($allErrors[$key]);
+                    }
+                }
+            }
+            $errorsList = [];
+            foreach ($allErrors as $key => $errors) {
+                foreach ($errors as $error) {
+                    $errorItem = Html::beginTag('li');
+
+                    // this is true in case of e.g. cross site validation error
+                    if (preg_match('/^\s?\<a /', $error)) {
+                        $errorItem .= $error;
+                    } else {
+                        $error = Markdown::processParagraph(htmlspecialchars($error));
+                        $errorItem .= Html::a(Craft::t('app', $error), '#', [
+                            'data' => [
+                                'field-error-key' => $key,
+                            ],
+                        ]);
+                    }
+
+                    $errorItem .= Html::endTag('li');
+
+                    $errorsList[] = $errorItem;
+                }
+            }
+
+            if (!empty($errorsList)) {
+                $heading = Craft::t('app', 'Found {num, number} {num, plural, =1{error} other{errors}}', [
+                    'num' => count($errorsList),
+                ]);
+
+                $html = Html::beginTag('div', [
+                        'class' => ['error-summary'],
+                        'tabindex' => '-1',
+                    ]) .
+                    Html::beginTag('div') .
+                    Html::tag('span', '', [
+                        'class' => 'notification-icon',
+                        'data-icon' => 'alert',
+                        'aria-label' => 'error',
+                        'role' => 'img',
+                    ]) .
+                    Html::tag('h2', $heading) .
+                    Html::endTag('div') .
+                    Html::beginTag('ul', [
+                        'class' => ['errors'],
+                    ]) .
+                    implode('', $errorsList) .
+                    Html::endTag('ul') .
+                    Html::endTag('div');
+            }
+        }
+
+        return $html;
+    }
+
     private function _editorSidebar(
         ElementInterface $element,
         bool $mergedCanonicalChanges,
@@ -957,7 +1039,7 @@ public function actionSave(): ?Response
         }
 
         try {
-            $success = $elementsService->saveElement($element);
+            $success = $elementsService->saveElement($element, crossSiteValidate: Craft::$app->getIsMultiSite());
         } catch (UnsupportedSiteException $e) {
             $element->addError('siteId', $e->getMessage());
             $success = false;
@@ -1345,7 +1427,7 @@ public function actionApplyDraft(): ?Response
             $element->setScenario(Element::SCENARIO_LIVE);
         }
 
-        if (!$elementsService->saveElement($element)) {
+        if (!$elementsService->saveElement($element, crossSiteValidate: Craft::$app->getIsMultiSite())) {
             return $this->_asAppyDraftFailure($element);
         }
 
@@ -1897,6 +1979,7 @@ private function _asFailure(ElementInterface $element, string $message): ?Respon
             'modelName' => 'element',
             'element' => $element->toArray($element->attributes()),
             'errors' => $element->getErrors(),
+            'errorSummary' => $this->_errorSummary($element),
         ];
 
         return $this->asFailure($message, $data, ['element' => $element]);

diff --git a/src/fields/BaseRelationField.php b/src/fields/BaseRelationField.php
@@ -495,8 +495,9 @@ public function validateRelatedElements(ElementInterface $element): void
         if ($errorCount) {
             /** @var ElementInterface|string $elementType */
             $elementType = static::elementType();
-            $element->addError($this->handle, Craft::t('app', 'Fix validation errors on the related {type}.', [
+            $element->addError($this->handle, Craft::t('app', 'Validation errors in {attribute} {type} - please fix them.', [
                 'type' => $errorCount === 1 ? $elementType::lowerDisplayName() : $elementType::pluralLowerDisplayName(),
+                'attribute' => $this->getAttributeLabel($this->handle),
             ]));
         }
     }

diff --git a/src/helpers/Cp.php b/src/helpers/Cp.php
@@ -776,6 +776,7 @@ public static function fieldHtml(string $input, array $config = []): string
                     'data' => [
                         'attribute' => $attribute,
                     ],
+                    'tabindex' => -1,
                 ],
                 $config['fieldAttributes'] ?? []
             )) .

diff --git a/src/i18n/I18N.php b/src/i18n/I18N.php
@@ -320,6 +320,17 @@ public function translate($category, $message, $params, $language): ?string
         return $translation;
     }
 
+    /**
+     * @inheritdoc
+     */
+    public function format($message, $params, $language)
+    {
+        // wrap attribute value in an <em> tag
+        array_walk($params, fn(&$val, $key) => ($key == 'attribute') ? $val = "*$val*" : $val);
+
+        return parent::format($message, $params, $language);
+    }
+
     /**
      * Returns whether [[translate()]] should wrap translations with `@` characters,
      * per the `translationDebugOutput` config setting.

diff --git a/src/services/Elements.php b/src/services/Elements.php
@@ -48,8 +48,10 @@
 use craft\helpers\DateTimeHelper;
 use craft\helpers\Db;
 use craft\helpers\ElementHelper;
+use craft\helpers\Html;
 use craft\helpers\Queue;
 use craft\helpers\StringHelper;
+use craft\helpers\UrlHelper;
 use craft\i18n\Translation;
 use craft\models\ElementActivity;
 use craft\queue\jobs\FindAndReplace;
@@ -1053,6 +1055,7 @@ public function saveElement(
         bool $propagate = true,
         ?bool $updateSearchIndex = null,
         bool $forceTouch = false,
+        ?bool $crossSiteValidate = false,
     ): bool {
         // Force propagation for new elements
         $propagate = !$element->id || $propagate;
@@ -1067,6 +1070,7 @@ public function saveElement(
             $propagate,
             $updateSearchIndex,
             forceTouch: $forceTouch,
+            crossSiteValidate: $crossSiteValidate,
         );
         $element->duplicateOf = $duplicateOf;
         return $success;
@@ -3077,6 +3081,7 @@ private function _saveElementInternal(
         ?bool $updateSearchIndex = null,
         ?array $supportedSites = null,
         bool $forceTouch = false,
+        bool $crossSiteValidate = false,
     ): bool {
         /** @var ElementInterface|DraftBehavior|RevisionBehavior $element */
         $isNewElement = !$element->id;
@@ -3333,7 +3338,9 @@ private function _saveElementInternal(
                         // Skip the initial site
                         if ($siteId != $element->siteId) {
                             $siteElement = $siteElements[$siteId] ?? false;
-                            $this->_propagateElement($element, $supportedSites, $siteId, $siteElement);
+                            if (!$this->_propagateElement($element, $supportedSites, $siteId, $siteElement, crossSiteValidate: $crossSiteValidate)) {
+                                return false;
+                            }
                         }
                     }
                 }
@@ -3466,14 +3473,17 @@ private function _saveElementInternal(
      * @param array $supportedSites The element’s supported site info, indexed by site ID
      * @param int $siteId The site ID being propagated to
      * @param ElementInterface|false|null $siteElement The element loaded for the propagated site
+     * @param bool $crossSiteValidate Whether we should "live" validate the element across all sites
+     * @retrun bool
      * @throws Exception if the element couldn't be propagated
      */
     private function _propagateElement(
         ElementInterface $element,
         array $supportedSites,
         int $siteId,
         ElementInterface|false|null &$siteElement = null,
-    ) {
+        bool $crossSiteValidate = false,
+    ): bool {
         // Make sure the element actually supports the site it's being saved in
         if (!isset($supportedSites[$siteId])) {
             throw new UnsupportedSiteException($element, $siteId, 'Attempting to propagate an element to an unsupported site.');
@@ -3562,17 +3572,74 @@ private function _propagateElement(
 
         // Save it
         $siteElement->setScenario(Element::SCENARIO_ESSENTIALS);
+
+        // validate element against "live" scenario across all sites, if element is enabled for the site
+        if ($crossSiteValidate && $siteElement->enabled && $siteElement->getEnabledForSite()) {
+            $siteElement->setScenario(Element::SCENARIO_LIVE);
+        }
+
         $siteElement->propagating = true;
 
-        if ($this->_saveElementInternal($siteElement, true, false, null, $supportedSites) === false) {
-            // Log the errors
-            $error = 'Couldn’t propagate element to other site due to validation errors:';
-            foreach ($siteElement->getFirstErrors() as $attributeError) {
-                $error .= "\n- " . $attributeError;
+        if ($this->_saveElementInternal($siteElement, true, false, null, $supportedSites, crossSiteValidate: $crossSiteValidate) === false) {
+            // if the element we're trying to save has validation errors, notify original element about them
+            if ($siteElement->hasErrors()) {
+                return $this->_crossSiteValidationErrors($siteElement, $element);
+            } else {
+                // Log the errors
+                $error = 'Couldn’t propagate element to other site due to validation errors:';
+                foreach ($siteElement->getFirstErrors() as $attributeError) {
+                    $error .= "\n- " . $attributeError;
+                }
+                Craft::error($error);
+                throw new Exception('Couldn’t propagate element to other site.');
             }
-            Craft::error($error);
-            throw new Exception('Couldn’t propagate element to other site.');
         }
+
+        return true;
+    }
+
+    /**
+     * @param ElementInterface $siteElement
+     * @param ElementInterface $element
+     * @return bool
+     * @throws Throwable
+     */
+    private function _crossSiteValidationErrors(
+        ElementInterface $siteElement,
+        ElementInterface $element,
+    ): bool {
+        // get site we're propagating to
+        $propagateToSite = Craft::$app->getSites()->getSiteById($siteElement->siteId);
+        $user = Craft::$app->getUser()->getIdentity();
+        $message = Craft::t('app', 'Validation errors for site: “{siteName}“', [
+            'siteName' => $propagateToSite?->name,
+        ]);
+
+        // check user can edit this element for the site that throws validation error on propagation
+        if ($user &&
+            Craft::$app->getIsMultiSite() &&
+            $user->can("editSite:{$propagateToSite?->uid}") &&
+            $siteElement->canSave($user)
+        ) {
+            $queryParams = ArrayHelper::without(Craft::$app->getRequest()->getQueryParams(), 'site');
+            $url = UrlHelper::url($siteElement->getCpEditUrl(), $queryParams + ['prevalidate' => 1]);
+            $message = Html::beginTag('a', [
+                'href' => $url,
+                'class' => 'cross-site-validate',
+                'target' => '_blank',
+            ]) .
+                $message .
+                Html::tag('span', '', [
+                    'data-icon' => 'external',
+                    'aria-label' => Craft::t('app', 'Open the full edit page in a new tab'),
+                    'role' => 'img',
+                ]) .
+                Html::endTag('a');
+        }
+
+        $element->addError('global', $message);
+
+        return false;
     }
 
     /**

diff --git a/src/templates/_layouts/cp.twig b/src/templates/_layouts/cp.twig
@@ -75,6 +75,7 @@
 {% set footer = (footer ?? block('footer') ?? '')|trim %}
 {% set crumbs = crumbs ?? null %}
 {% set tabs = (tabs ?? [])|length > 1 ? tabs : null %}
+{% set errorSummary = errorSummary ?? null %}
 
 {% set mainContentClasses = [
     sidebar ? 'has-sidebar',
@@ -243,6 +244,9 @@
                                 <h2 id="content-heading"></h2>
                             {% endif %}
                             {% block main %}
+                                {% if errorSummary is not empty %}
+                                    {{ errorSummary is defined ? errorSummary|raw }}
+                                {% endif %}
                                 <div id="content" class="content-pane">
                                     {% if contentNotice or tabs %}
                                         <header id="content-header" class="pane-header">

diff --git a/src/translations/en/app.php b/src/translations/en/app.php
@@ -668,6 +668,7 @@
     'Forgot your password?' => 'Forgot your password?',
     'Format' => 'Format',
     'Formatting Locale' => 'Formatting Locale',
+    'Found {num, number} {num, plural, =1{error} other{errors}}' => 'Found {num, number} {num, plural, =1{error} other{errors}}',
     'Free' => 'Free',
     'From {date}' => 'From {date}',
     'From' => 'From',
@@ -1709,6 +1710,8 @@
     'Utilities' => 'Utilities',
     'Validate custom fields on public registration' => 'Validate custom fields on public registration',
     'Validate related {type}' => 'Validate related {type}',
+    'Validation errors in {attribute} {type} - please fix them.' => 'Validation errors in {attribute} {type} - please fix them.',
+    'Validation errors for site: “{siteName}“' => 'Validation errors for site: “{siteName}“',
     'Value prefixed by “{prefix}”.' => 'Value prefixed by “{prefix}”.',
     'Value suffixed by “{suffix}”.' => 'Value suffixed by “{suffix}”.',
     'Value' => 'Value',

diff --git a/src/translations/pl/app.php b/src/translations/pl/app.php