surface-ui / surface

A server-side rendering component library for Phoenix
https://surface-ui.org
MIT License
2.04k stars 152 forks source link

Fix hidden inputs #716

Closed marcandre closed 9 months ago

marcandre commented 10 months ago

This PR kills two birds with one stone. 1) Avoid using deprecated hidden_inputs_for 2) Avoid crash when base form is a changeset

On that second point, this is a regression caused by 82d7c6e . I didn't dig into it, but somehow the hidden did not use to have {"_persistent_id", "0"} before it. Now that it does (correctly, right?) it crashes:

  2) test based on a changeset (Surface.Components.Form.HiddenInputsTest)
     test/surface/components/form/hidden_inputs_test.exs:53
     ** (ArgumentError) expected field to be an atom, got: "_persistent_id"
     code: <HiddenInputs for={f} />
     stacktrace:
       (phoenix_ecto 4.4.0) lib/phoenix_ecto/html.ex:128: Phoenix.HTML.FormData.Ecto.Changeset.input_value/4
       (phoenix_html 3.3.1) lib/phoenix_html/form.ex:941: Phoenix.HTML.Form.generic_input/4
       (phoenix_html 3.3.1) lib/phoenix_html/form.ex:785: Phoenix.HTML.Form.hidden_inputs_for/3
       (elixir 1.15.7) lib/enum.ex:4317: Enum.flat_map_list/2
       (surface 0.11.1) lib/surface/components/form/hidden_inputs.ex:20: anonymous fn/2 in Surface.Components.Form.HiddenInputs."render (overridable 1)"/1
       /Users/mal/surface/test/surface/components/form/hidden_inputs_test.exs:61: (test)

HiddenInputs simply calls hidden_inputs_for, and I find it odd it actually crashes, and I'm not sure if it's a bug or a "feature". Indeed, that loops of the hidden and calls hidden_input with value: "0". But the implementation of hidden_input calls Keyword.put_new(:value, input_value(...)) and not Keyword.put_new_lazy so Phoenix.HTML.input_value is always called, even if there's a :value given. That function officially accepts fields that are strings, but Ecto.Changeset's implementation for Phoenix.HTML.FormData somehow only accepts atoms.

In any case, hidden_inputs_for is being removed so implementation for HiddenInputs needs to change.

noozo commented 9 months ago

any plans to merge this?

noozo commented 9 months ago

@msaraiva?

msaraiva commented 9 months ago

@marcandre thank you so much for the PR. 💚

Sorry for the late reply.