Nested errors not handled for map's FormData implementation

[LostKobrakai]

With the new FormData implementation for plain maps I tried to set errors on nested inputs, but it looks like errors are not handled for nested inputs.

[josevalim]

Any ideas on how it should be handled? Thoughts and PRs are welcome!

[LostKobrakai]

Not concretely. I was looking for (more) documentation when I discovered it's not handled yet. The only thing I found was that it's supposed to be a Keyword list, but I guess one_to_many also needs some kind of indexing / matching length lists.

[LostKobrakai]

So currently errors seem to be a expected to be a list of [{field :: String.t | atom, error :: term}].

Nesting could be added to that in different ways:

• Expecting a (single) list item per parent within that list:
  {parent_field, [{inner_field :: String.t | atom, error :: term}]}
  There's the complexity of how to deal with multiple parent keys in the errors list as well as differenciating cases, where we set an error on the parent, vs it holding errors of nested fields.
• Changing the field key of the tuple to allow nested fields:
  [{field :: String.t | atom | [String.t | atom], error :: term}]

# First option
errors = [
  {"title", "is invalid"},
  {"title", "must be larger than 5"},
  {
    "parent",
    [
      {"inner", "is invalid"},
      {"inner", "must be larger than 5"},
    ]
  }
]

# Second option
errors = [
  {"title", "is invalid"},
  {"title", "must be larger than 5"},
  {["parent", "inner"], "is invalid"},
  {["parent", "inner"], "must be larger than 5"}
]

I feel like the second option would be the cleaner one, but open to discussion or input on this one.

[josevalim]

Thanks. The issue with this approach is that we enlarge the contract for everyone. The root of the problem is that a map (or an atom or a conn) does not provide enough information for nested error handling, so I am more inclined to say you will need a more complete data-structure for this, such as changesets (or anything similar to them).

[LostKobrakai]

Then I'm wondering how useful it is to support nesting for those structures in the first place. Not supporting error handling severly limits the usefulness of a form integration and to me this is all the reason why I never even bothered using the atom/conn implementation in the past.

With the recent updates and seemingly more push to make using maps to power forms usable I had hoped these drawbacks would've been addressed – especially as I'm interested in forms, where I don't need to use atoms as keys, which rules out using changesets (without some external mapping).

Is there any appetite to have a more feature complete implementation in phoenix_html, or would it be better for me to bite the bullet and go with a custom implementation for my usecase?

[josevalim]

The goal of adding maps was to provide a less confusing API than for={:foo} as={:this_is_ambiguous} params={map}, in favor of for={params} as={:foo} which mirrors changesets too. Plus getting rid of the Plug dependency.

It doesn't mean we can't improve it but I hope this provides some background (which means you should probably bite the bullet, sorry!).

[LostKobrakai]

Ok, I guess I'll one follow up question then. Can you expand a bit on where / which contract you see "enlarged for everyone" trying to support nested errors for maps? If I can avoid touching those parts then maybe there's a chance to port back the things I do into phoenix_html.

[josevalim]

As you said, errors is defined as [{field :: String.t | atom, error :: term}] and if we want to support nesting through the errors themselves, we would have to enlarge it. This is not an issue with Ecto because the nested errors is kept in a separate field.

[LostKobrakai]

This was already extended recently to support string field names over just atom field names, so I'm wondering how much this is a concern. We for sure don't want to jump the gun on what format would be appropriate for nested errors, but that doesn't look like changes couldn't be made.

[guidomb]

@josevalim I found myself in a similar situation not understanding why errors where not displayed when using nested fields, inputs_for for a form created using to_form passing a map as its first argument. Maybe I'm oversimplifying the problem but couldn't this issue be solved by changing how the child form is constructed here from

is_map(default) ->
        [
          %Phoenix.HTML.Form{
            source: conn_or_atom_or_map,
            impl: __MODULE__,
            id: id,
            name: name,
            data: default,
            action: action,
            params: params || %{},
            hidden: hidden,
            options: opts
          }
        ]

to

is_map(default) ->
        [
          %Phoenix.HTML.Form{
            source: conn_or_atom_or_map,
            impl: __MODULE__,
            id: id,
            name: name,
            data: default,
            action: action,
            params: params || %{},
            hidden: hidden,
            options: opts,
            errors: Keyword.get(form.errors, field, [])
          }
        ]

by adding this line errors: Keyword.get(form.errors, field, [])? (not sure how the is_list case should be handled yet)

[guidomb]

guidomb

I just tested this on our fork in our application and it worked https://github.com/cedalio/phoenix_html/commit/02ac3af77ab7c35a504191b68eb26381377d8f30. Should I send a PR?

[honungsburk]

That errors do not work for nested fields is super unintuitive. I'm a new phoenix user and I just assumed that it would work and wasted many hours before finding this issue.