Make to_form/2 more strict about the maps it accepts?

[eriknaslund]

Environment

• Elixir version (elixir -v): Elixir 1.16.0 (compiled with Erlang/OTP 26)
• Phoenix version (mix deps): 1.7.10
• Phoenix LiveView version (mix deps): 0.20.3
• Operating system: Manjaro
• Browsers you attempted to reproduce this bug on (the more the merrier): Opera
• Does the problem persist after removing "assets/node_modules" and trying again? Yes/no: Yes

Actual behavior

Calling to_form/2 on a map with atom keys seems to work, but it doesn't really. Here's a minimal snippet to demonstrate:

<.form :let={f} for={to_form(%{"string_key" => "value", atom_key: "value"})}>
    String key: <.form_input type="text" field={f["string_key"]} />
    Atom key: <.form_input type="text" field={f[:atom_key]} />
</.form>

The value set for the string key is displayed correctly, but the value for the atom key isn't.

I'm aware that this "bug" has already been raised as https://github.com/phoenixframework/phoenix_live_view/issues/2469. However I think we can do better, and have something that is even more helpful for the developer :)

Expected behavior

If you inspect the Form and the FormField structs you'll see the following:

FORM: %Phoenix.HTML.Form{
  source: %{:atom_key => "value", "string_key" => "value"},
  impl: Phoenix.HTML.FormData.Map,
  id: nil,
  name: nil,
  data: %{},
  hidden: [],
  params: %{:atom_key => "value", "string_key" => "value"},
  errors: [],
  options: [],
  index: nil
}
STRING KEY FIELD: %Phoenix.HTML.FormField{
  id: "string_key",
  name: "string_key",
  errors: [],
  field: "string_key",
  form: %Phoenix.HTML.Form{
    source: %{:atom_key => "value", "string_key" => "value"},
    impl: Phoenix.HTML.FormData.Map,
    id: nil,
    name: nil,
    data: %{},
    hidden: [],
    params: %{:atom_key => "value", "string_key" => "value"},
    errors: [],
    options: [],
    index: nil
  },
  value: "value"
}
ATOM KEY FIELD: %Phoenix.HTML.FormField{
  id: "atom_key",
  name: "atom_key",
  errors: [],
  field: :atom_key,
  form: %Phoenix.HTML.Form{
    source: %{:atom_key => "value", "string_key" => "value"},
    impl: Phoenix.HTML.FormData.Map,
    id: nil,
    name: nil,
    data: %{},
    hidden: [],
    params: %{:atom_key => "value", "string_key" => "value"},
    errors: [],
    options: [],
    index: nil
  },
  value: nil
}

The only (unexpected?) difference between the string and atom key is that value is set to nil for the atom key...and therefore isn't displayed when rendering.

I know the docs for to_form/2 state that (_When you pass a map to toform/1, it assumes said map contains the form parameters, which are expected to have string keys.). However I think it'd be more user friendly to hard-fail if the developer tries to pass a map with atom keys.

If you pass to_form/2 a map with atom keys it returns something that almost works. These pseudo-failures are often hard to debug, because it looks like you're doing things right at first glance. Imo it'd be better to be strict about the inputs here.

What do you think?

[SteffenDE]

to_form with a map uses https://github.com/phoenixframework/phoenix_html/blob/2b222dbf104b3ac952053de441b61f56f89f4bf8/lib/phoenix_html/form_data.ex#L80 under the hood, so probably the place to add the check would be in phoenix_html. I'm not sure if this is intentional or not, but @josevalim probably knows.

[josevalim]

I think a PR that does :maps.next(:maps.iterator(map)) and check if the string is an atom and then warns would be welcome!

[SteffenDE]

Well, I should have looked a couple lines below.. https://github.com/phoenixframework/phoenix_html/commit/2d2e2e9fcab76c376b07a9bbb7b2659bee591b46

We already have the warning 😃

[SteffenDE]

@eriknaslund which version of phoenix_html are you using? I think the existing warning should be just fine, wdyt?

[josevalim]

Should we hard fail then? Since we have been emitting the warning for a while?

[SteffenDE]

Should we hard fail then? Since we have been emitting the warning for a while?

I'm not sure, I think a warning is probably good enough. It would be a breaking change to raise.

There is a bit of luck involved with the check currently, right? We only warn if the first key we get is an atom, but by chance the first one could be a string as well and only a later one an atom.

Is this something the type system could warn about in the future?

[josevalim]

There is a bit of luck involved with the check currently, right? We only warn if the first key we get is an atom, but by chance the first one could be a string as well and only a later one an atom.

For small maps, atoms always come before strings, so it should be fine unless you have 32+ keys (or 16+?)

Is this something the type system could warn about in the future?

YES!

[SteffenDE]

Alright then. I‘d say the existing warning is good enough for now :)

[eriknaslund]

@SteffenDE I have to hang my head in shame and admit I was sloppy enough to simply miss the warning. It was indeed there (I was on phoenix_html 4.1.1).

I develop a bit of "warning-ignoritis" from time to time, seeing things like dependencies using Logger.warn emitting warnings. This somehow tricks my brain that warnings are just "things that are kinda bad, but it still works" (like C compile warnings). That's completely on me, and I'll take Elixir's warnings more seriously from now on - sorry Elixir! :)

I completely get the rationale about avoiding breaking changes. Closing the issue seems sensible to me, since there is a warning in place.

@josevalim Now I'm looking forward to the type system even more :).