Support error messages for custom node validation rules

[MoritzLost]

What are you trying to do?

I'm adding some validation logic to prevent saving nodes in some scenarios. In particular, I want to only allow nested children under passive nodes, not active nodes. This can be done using hooks:

public function init()
{
    Event::on(
        Node::class,
        Node::EVENT_BEFORE_SAVE,
        $this->preventNestedMenuLinksOnSave(...),
    );

    Event::on(
        Structures::class,
        Structures::EVENT_BEFORE_MOVE_ELEMENT,
        $this->preventNestedMenuLinksOnMove(...),
    );

    parent::init();
}

public function preventNestedMenuLinksOnSave(ModelEvent $e)
{
    /** @var Node $node */
    $node = $e->sender;

    /** @var null|Node $parent */
    $parent = $node->parent;

    // Prevent adding children to non-passive nodes
    if ($parent && !$parent->isPassive()) {
        $e->isValid = false;
    }

    // Prevent changing the node type from passive if the node has children
    if ($node->hasDescendants && !$node->isPassive()) {
        $e->isValid = false;
    }
}

public function preventNestedMenuLinksOnMove(MoveElementEvent $e)
{
    $element = $e->element;

    if (!$element instanceof Node) {
        return;
    }

    /** @var Node $target */
    $target = $e->getTargetElement();

    // Prevent moving nodes below non-passive nodes
    if ($target && !$target->isPassive()) {
        $e->isValid = false;
    }
}

However, the Author Experience is not great. When the user tries to add a node to an active parent, the error popup simply says Couldn't save node. When they try to move it below a new parent that isn't passive, it's even worse, you only get A server error occured.

What's your proposed solution?

Custom validation logic sort of requires the ability to add error messages. I know that's not fully under the plugin's control (especially in the second case, I remember this was an issue for #359). But maybe there's a way to still show meaningful error messages? Maybe extend the ModelEvent and add a property for a custom error message there? Or make it possible to add an error using $node->addError() and display that in the frontend?

As another solution to this particular use-case, maybe a setting could be added for navigations to not allow nested active links? We found that having nested active links presents accessibility and UX issues in many common scenarios, so an option like this might be useful.

Additional context

No response

[engram-design]

The tricky part here is that the move event is controlled by Craft's structures/move-element controller which doesn't give us much opportunity to provide a better error message. We would need to spin up our own controller action just to have a nicer error message. It's doable for sure, just not my first preference.

As an aside, you can also use the NodeType::EVENT_DEFINE_RULES event to register custom Yii/Craft validation rules to solve this, as opposed to the before/after events. But they'll both produce the same error text. This is actually how the plugin does validation.

[MoritzLost]

@engram-design Thanks for the tip! I've switched to custom validation rules, this improves the AX when adding & editing nodes considerably, since it enables custom error messages:

I'll leave the code below in case anyone wants to do something similar.

The tricky part here is that the move event is controlled by Craft's structures/move-element controller which doesn't give us much opportunity to provide a better error message. We would need to spin up our own controller action just to have a nicer error message. It's doable for sure, just not my first preference.

Yeah, that's still not ideal. Looks like model validation isn't triggered at all when elements are moved in a structure? So even with the additional validation rules, I still need the hook on Structures::EVENT_BEFORE_MOVE_ELEMENT … pretty annoying. And while the nice error messages show up when adding/editing elements using the custom validation rules, using drag-and-drop still results in A server error occured.

Maybe this could be addressed in Craft itself?

---

Here's the code for my custom validator:

// init() method in a custom module
Event::on(
    Node::class,
    Node::EVENT_DEFINE_RULES,
    function (DefineRulesEvent $e) {
        $e->rules[] = [['type', 'parentId'], NavigationNodeValidator::class];
    }
);

// NavigationNodeValidator.php
<?php

namespace modules\ControlPanelCustomization\validators;

use Craft;
use verbb\navigation\elements\Node;
use yii\validators\Validator;

/**
 * Custom validation rules for navigation nodes.
 */
class NavigationNodeValidator extends Validator
{
    public function validateAttribute($model, $attribute)
    {
        if (!$model instanceof Node) {
            return;
        }

        switch ($attribute) {
            case 'type':
                $this->validateType($model);
                break;
            case 'parentId':
                $this->validateParentId($model);
                break;
        }
    }

    private function validateType(Node $model): void
    {
        if (!$model->hasDescendants || $model->isPassive()) {
            return;
        }

        $model->addError('type', Craft::t('navigation', 'Only passive nodes are allowed to have children.'));
    }

    private function validateParentId(Node $model): void
    {
        /** @var null|Node $parent */
        $parent = $model->parent;

        if (!$parent || $parent->isPassive()) {
            return;
        }

        $model->addError('parentId', Craft::t('navigation', 'Only passive nodes are allowed as parents.'));
    }
}

[engram-design]

Good call on needing structures::EVENT_BEFORE_MOVE_ELEMENT that’s true.

I’ll see what I can do to improve this. Although they are two different things - one is a save event, the other is a move event, but it’s arguable that both should validate the model regardless. Even if moving an element doesn’t save the element directly. I don’t think Craft would be keen on this with other elements so it might be up to us to deal with. I’ll chat with the Craft crew.

[MoritzLost]

@engram-design Thanks! I agree, moving elements should always trigger a full validation. Otherwise, any validation rules related to parent / children can't really be used effectively. The ideal solution would be for the built-in structures controller to trigger a full validation on every move, and for the structure editor to have a way to display validation errors. Though that's probably a breaking change.

[MoritzLost]

@engram-design Looks like Craft 5.3 will support better error messages when custom validation prevents moving elements in a structure: https://github.com/craftcms/cms/issues/15292

Can this be used to improve the UX when adding custom validation rules for node placement?

[engram-design]

That's great news to hear! I've just added the UserException on my end when moving nodes, and you could do the same in your custom validators, swapping out $e->isValid = false;. This will have no effect on installs before Craft 4.11 and 5.3, but it also be harmless in that it won't cause an issue if included.

[MoritzLost]

@engram-design Great, thanks! I'll give that a try.