Trace paths instead of IDs

[henriquemoody]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 80.30303% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.08%. Comparing base (2b36476) to head (abe204a).

Files with missing lines	Patch %	Lines
library/FailedResultIterator.php	72.91%	13 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1532      +/-   ##
============================================
- Coverage     96.65%   96.08%   -0.57%     
- Complexity     1022     1043      +21     
============================================
  Files           206      209       +3     
  Lines          2569     2608      +39     
============================================
+ Hits           2483     2506      +23     
- Misses           86      102      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[alganet]

Is the intent here to pick just one of the submessages, or are we working to flatten the message tree?

Flattening would leave something like author.intType, author.lengthBetween and so on (recursively, so multiple.nested.validators.all.get.their.path), all in the root array.

This flattening behavior is similar to what findMessages did in the past, although findMessages was using a trick (the user provided the path, we just walked it).

I also had trouble with making unambiguous paths (without relying on the lib user to provide search paths for the errors he/she is interested in).

It is an interesting problem though.

[alganet]

Okay, so I see that this is just temporary dev stuff and the intention is not to flatten it out (at least not in the StandardFormatter).

@henriquemoody we should have a discussion about design here.

I'm worried about each:

        0 => '`.my_int` must be present',
        1 => '`.my_int` must be an odd number',
        2 => [
            '__root__' => '`.2` must pass all the rules',
            'arrayType' => '`.2` must be an array',
            'my_int' => '`.my_int` must be present',
        ],

It would be ideal if all the items for Each had the same topology. Some items report a single string (0 and 1) and others report an array (2). I'm assuming you also struggled with choices regarding being concise or easy to navigate the structure (not requiring users to do is_array and stuff).

From what I understood now, you don't want to flatten the entire message results, but you want that uniform topology. Maybe a single string for each item (thus limiting a little bit the detail it can deliver).

I think that if we don't collapse (0 and 1 would still be arrays with some verbosity), we can avoid having to do full path tracing. The trick would be to do it just for Each (the design choice is: first level of Each is always verbose, to simplify consuming it). In simple terms, each item in Each should have a root.

I could be missing something regarding templates and stuff. As I said, I need some time to get really close to 3.0 architecture. I'm learning it. If you have any insights you think could help me, please share.

[henriquemoody]

It's gonna be difficult to talk about this until I am back from vacation on the 15th of December.

All I can tell you from the top or my head is that after years working with findMessages() and getMessages(), and getting multiple bug reports expecting different structures, I came to realise that the best is to keep the structure as similar as the input -- and even that is kinda impossible. The idea of getMessages() is to return a structure that people can use to, let's say, attach to a <form>, but doing that only through arrays is impossible. The findMessages provided a much more elegant solution for that case, but it wouldn't play so well making a distinction between a single failed validation or a composite.

I can also say that Each is the biggest challenge with any version of Validation, IMO. So, for 3.0 my goal was to just get getMessages to return a useful structure, and figure out in future versions a mechanism for people to iterate over the failed results.

This PR is not so much about the getMessages(), though, but about getMessage() returning a path that is recognisable, right now if a nested key fails, we only see the key that fails without the context of its parents and this is what I am trying to solve here.

[henriquemoody]

Also, I think I might have several changes in my local branch...

[alganet]

I think we are aligned in what we want for Each: A predictable response structure. I agree that is one of the hardest validators to deal with (and we dropped key validation! This monster used to validate array keys as well, not only values).

Would you be OK with having some if ($result->rule instanceof Each in the StandardFormatter? Maybe an interface instead of the specific rule just to be cleaner.

If we can do that, we can flatten the Each result. Example with the problematic skipped test:

        '__root__' => 'Each item in `[["not_int": "wrong"], ["my_int": 2], "not an array"]` must be valid',
        '0.my_int' => '`.my_int` must be present',
        '1.my_int' => '`.my_int` must be an odd number',
        '2' => '`.2` must pass all the rules',
        '2.arrayType' => '`.2` must be an array',
        '2.my_int' => '`.my_int` must be present',

You can see that I don't have a clear "path" (mixes keys with validator names in some results). There's no one-size-fits all solution here. Some "forms" (or API error decorators) will want some things, others will want other things.

If we can find the expected array we want, we find the solution.

I'll keep thinking about it, and stop botching your vacation 🐼 We can discuss it better after you come back!