Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve messages in an array format #1498

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Improve messages in an array format #1498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 21 additions & 21 deletions library/Message/StandardFormatter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,42 +106,42 @@ public function array(Result $result, array $templates, Translator $translator):
{
$selectedTemplates = $this->selectTemplates($result, $templates);
$deduplicatedChildren = $this->extractDeduplicatedChildren($result);
if (count($deduplicatedChildren) === 0 || $this->isFinalTemplate($result, $selectedTemplates)) {
return [
$result->getDeepestPath() ?? $result->id => $this->renderer->render(
$this->getTemplated($result->withDeepestPath(), $selectedTemplates),
$messages = [
'messages' => [
$result->id => $this->renderer->render(
$this->getTemplated($result, $selectedTemplates)->withWithoutPath(),
$translator
),
];
}

$messages = [];
],
'details' => [],
'children' => [],
];
foreach ($deduplicatedChildren as $child) {
if ($child->path === null) {
$messages['details'][$child->id] = $this->renderer->render(
$this->getTemplated($child, $selectedTemplates)->withWithoutPath(),
$translator
);
continue;
}
$key = $child->getDeepestPath() ?? $child->id;
$messages[$key] = $this->array(
$messages['children'][$key] = $this->array(
$this->resultWithPath($result, $child),
$this->selectTemplates($child, $selectedTemplates),
$translator
);
if (count($messages[$key]) !== 1) {
if (count($messages['children'][$key]) !== 1) {
continue;
}

$messages[$key] = current($messages[$key]);
$messages['children'][$key] = current($messages['children'][$key]);
}

if (count($messages) > 1) {
$self = [
'__root__' => $this->renderer->render(
$this->getTemplated($result->withDeepestPath(), $selectedTemplates),
$translator
),
];

return $self + $messages;
if ($result->path !== null) {
return [$result->getDeepestPath() => array_filter($messages)];
}

return $messages;
return array_filter($messages);
}

public function resultWithPath(Result $parent, Result $child): Result
Expand Down
17 changes: 17 additions & 0 deletions library/Result.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,23 @@ public function withDeepestPath(): self
);
}

public function withWithoutPath(): self
{
return new self(
$this->isValid,
$this->input,
$this->rule,
$this->parameters,
$this->template,
$this->mode,
$this->name,
$this->id,
$this->adjacent?->withWithoutPath(),
null,
...$this->children
);
}

public function getDeepestPath(): ?string
{
if ($this->path === null) {
Expand Down
93 changes: 93 additions & 0 deletions tests/feature/Issues/Issue1427Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?php

/*
* Copyright (c) Alexandre Gomes Gaigalas <[email protected]>
* SPDX-License-Identifier: MIT
*/

declare(strict_types=1);

test('https://github.com/Respect/Validation/discussions/1427', expectAll(
fn () => v::each(
v::arrayVal()
->key('groups', v::each(v::intVal()))
->key('permissions', v::each(v::boolVal()))
)
->assert([
16 => [
'groups' => [1, 'A', 3, 4, 5],
'permissions' => [
'perm1' => true,
'perm2' => false,
'perm3' => 'boom!',
],
],
18 => false,
24 => ['permissions' => false],
]),
'`.16.groups.1` must be an integer value',
<<<'FULL_MESSAGE'
- Each item in `[16: ["groups": [1, "A", 3, 4, 5], "permissions": ["perm1": true, "perm2": false, "perm3": "boom!"]], 18: false, ... ]` must be valid
- `.16` must pass the rules
- Each item in `.groups` must be valid
- `.1` must be an integer value
- Each item in `.permissions` must be valid
- `.perm3` must be a boolean value
- `.18` must pass all the rules
- `.18` must be an array value
- `.groups` must be present
- `.permissions` must be present
- `.24` must pass the rules
- `.groups` must be present
- `.permissions` must be iterable
FULL_MESSAGE,
[
'messages' => ['each' => 'Each item in `[16: ["groups": [1, "A", 3, 4, 5], "permissions": ["perm1": true, "perm2": false, "perm3": "boom!"]], 18: false, ... ]` must be valid'],
'children' => [
16 => [
'messages' => ['allOf' => '`["groups": [1, "A", 3, 4, 5], "permissions": ["perm1": true, "perm2": false, "perm3": "boom!"]]` must pass the rules'],
'children' => [
'groups' => [
'messages' => ['each' => 'Each item in `[1, "A", 3, 4, 5]` must be valid'],
'children' => [
1 => [
'messages' => ['intVal' => '"A" must be an integer value'],
],
],
],
'permissions' => [
'messages' => ['each' => 'Each item in `["perm1": true, "perm2": false, "perm3": "boom!"]` must be valid'],
'children' => [
'perm3' => [
'messages' => ['boolVal' => '"boom!" must be a boolean value'],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not replacing the input of each rule with the key, because I think that, since the structure is already in the array, it's best to keep the original message instead, so you can actually see the value that failed.

],
],
],
],
],
18 => [
'messages' => ['allOf' => '`false` must pass all the rules'],
'details' => ['arrayVal' => '`false` must be an array value'],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this key because some rules have children but they're respective to the parent validation, not a child. I thought I would add an extra key there to identify those. I think that one would always use details instead of messages when it's available because messages would usually be only headers of nesting validations.

'children' => [
'groups' => [
'messages' => ['keyExists' => '`false` must be present'],
],
'permissions' => [
'messages' => ['keyExists' => '`false` must be present'],
],
],
],
24 => [
'messages' => ['allOf' => '`["permissions": false]` must pass the rules'],
'children' => [
'groups' => [
'messages' => ['keyExists' => '`["permissions": false]` must be present'],
],
'permissions' => [
'messages' => ['each' => '`false` must be iterable'],
],
],
],
],
]
henriquemoody marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member Author

@henriquemoody henriquemoody Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gotta say, we can added a whole bunch of other things here, I'm just not sure how they can be useful. Each element could contain the keys:

  • messages: all the messages that fails
  • details: more details about the messages that failed
  • children: all the children (which in turn could have all the keys I'm mentioning here
  • input: the input that was proceeded by the rule
  • path: the path of the rule that failed

But when I start to really think about this, I think this is not even an array in an exception anymore, it could be an object that contains all this information and handles all of them properly.

Maybe I'm just thinking of providing this array to the exception because of how the library worked before, but I could just drop the whole array thing, and return a proper object in Validator::validate() that would contain all the failed messages and the extra information that is needed -- maybe I'm just conditioned to how the library worked in the past.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can offer some perspective on what I was thinking when I made the original library.

The array thing and a lot of things (like setting properties in runtime objects) were implementation details, mostly private stuff, that could be done in whatever way fits best (I decided to go for the solution that had lass code back then).

To me, the reportError (or was it reportException?) method was part of the validatable interface that could be extended and the thing I wanted to look nice. Together with the base lib, it offered two standard ways of creating exceptions:

  • ValidationException, which is a simple single message object.
  • AbstractNestedException, which is a more elaborate object, mostly used by aggregate validators, but actually an object of its own.

AbstractNestedException had things like findMessages with paths back then, although the workings were slightly different. Since each validator had to had a dedicated exception, I went for an abstract instead of a concrete named object like ValidationResults.

The decision to use exceptions also played a role in the design of Validatable::check, which I think doesn't exist anymore. It was designed to catch exceptions by type early instead of running the whole thing (the decision whether to fail fast or not). This complicated the internal workings of the project, and it was a compromise I'm not sure we still need to make. It seems our users don't care about fail fast that much.

So, you're still dealign with a lot of interlocked design decisions I made back then :D. You don't need to follow any of them if they don't make sense anymore. I would just go for whatever fullfills your vision best. That was what I did: I just moved things around until it was cohese.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for giving me some perspective, @alganet, also it's always nice to hear from you! That does give me more peace with changing things a bit more, although at this point, only the API of the previous Validator remains, and not all of it. At times, I even wonder how you perceive it, since you created the library.

I think that failing fast is still important in some cases. We'll have Circuit in version 3.0 to fulfil that purpose, but users will need to be intentional about it.

One thing that I liked a lot about check() was the ability to pick the top message, instead of the whole thing, and this will be similar in version 3.0.

Since you're here, I'd love to get your perspective on something else. When you created the library were the validation messages meant for engineers or end-users? That's something that I'm often conflicted about.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@henriquemoody

I thought of use cases for both engineers and end-users.

The findMessages and getMessages APIs were designed to work with HTML forms. It was designed so the engineer can pick, for each field, what he wants ("oh, for this I want detailed messages like the domain is wrong, the tld is wrong.... and for this one I just want a simple message").

Some of the features were designed to later work with APIs as well. That's why findMessages had selectors. You could kind of use the same validator, but change the exception catching part to deliver different reports to different kinds of users. There was also some feng shui to adhere informally (duck typing) to the Respect\Rest interface (you could use a validator directly as a when() routine, which was also part of the design we don't exactly need anymore).

The getFullMessage API that returned an ASCII tree was designed for CLI applications, print debugging, stuff like that. It gave a full usable text block as a message. I was expecting to see that in logs, tests, stdout from services.

So, in a sense, I did for both, but I always had some specific use cases in mind.

The way I perceive what you're changing is to move to a canonical, fully serializable "report". Mine was different, in my idea you could "query" this non-linear exception tree for a specific serialization. There are compromises in both, and I don't prefer any over the other, both are cool.

One thing that bothered me in my original design was that the validation tree and the result tree were sometimes not the same. I discovered too late that these trees were not equal. I never truly figured this part out, and the API grew from the assumption that these two trees (the composite validator and the result exception) were "equivalent". However, several objects "knew inside" that there were violations to that equivalency. I sometimes see you wrestling with similar problems, but I honestly don't have much to add :) I think it's an unsolvable problem, and you'll do similar in spirit to what I did (just trim the rough edges as best as possible), but with a more robust implementation design.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you mentioned about the trees is so true... It got a bit better with the Result since we don't loose some information that we would have lost if we were only dealing with exceptions (since we would only have exceptions when the validation failed, not if it succeeded), but still, there's quite a bit of overhead, Result objects that serve no purpose but to hold the validation of its children. I'm reducing the overhead with the Reducer rule, but still.

As for this specific change connected to your last comment, just as you, I also thought of getMessages() as a useful and simple to handle HTML forms (or API requests). That's why I'm a bit reluctant to create this super structure here, it seems like when people get to the point of processing this structure, an array wouldn't be as useful anyways.

Good to hear your thoughts. It was also good to be reminded of the findMessages() method. I've never used it myself, but I see how useful that can be, and I would like to keep it.

I'm thinking of repurposing the validate() method, to return an object with failures. It could be something like this.

$failures = v::key('foo', v::intVal()->positive()->greaterThan())->validate($input);
if ($failures === null) {
    echo 'No failures' . PHP_EOL;
    return;
}

// Getting all messages
echo $failures->getMessage() . PHP_EOL;
echo $failures->getFullMessage() . PHP_EOL;
echo print_r($failures->getMessages(), true) . PHP_EOL;

// Finding specific messages
echo $failures->findMessage('foo') . PHP_EOL;
echo print_r($failures->findMessages(['foo' => 'Something went wrong']), true) . PHP_EOL;

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks cool!

I would use $result instead of $failures, and maybe do a $result->hasPassed() method instead of the null check. I have grown wary of optional nulls in interfaces recently :)

Copy link
Member Author

@henriquemoody henriquemoody Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that too, and I suggested returning the failures because I think it's no use for the user to deal with anything but the failures, and consequently no reason for this object to contain passing results. Apart from that, the Rule::evaluate() method already returns a Result.

I do get what you're saying about interfaces that return nullable objects. Maybe something in-between could work:

$failures = v::key('foo', v::intVal()->positive()->greaterThan())->validate($input);
if (count($failures) === 0) {
    echo 'No failures' . PHP_EOL;
    return;
}

It looks very similar to Symfony Validator 🙃 But alternatively, it could be something like this:

$failures = v::key('foo', v::intVal()->positive()->greaterThan())->validate($input);
if ($failures->isEmpty()) {
    echo 'No failures' . PHP_EOL;
    return;
}

What do you think?

Copy link
Member

@alganet alganet Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it (the latest snippet)! 🐼

));
Loading