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.

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

Phoenix.Component.inputs_for/1 Ignores id and index from Phoenix.HTML.Form struct #3673

Open
woylie opened this issue Feb 12, 2025 · 4 comments · May be fixed by #3677
Open

Phoenix.Component.inputs_for/1 Ignores id and index from Phoenix.HTML.Form struct #3673

woylie opened this issue Feb 12, 2025 · 4 comments · May be fixed by #3677

Comments

@woylie
Copy link
Contributor

woylie commented Feb 12, 2025

I posted this before on Elixir Forum before, but didn't get any response. I'm not sure whether this is a bug or whether there's a reason behind the current implementation, so please excuse me for posting the same question here.


Flop.Phoenix implements the Phoenix.HTML.FormData protocol for its meta struct: https://github.com/woylie/flop_phoenix/blob/main/lib/flop_phoenix/form_data.ex.

At some point, an offset option was added to allow users to build more complex form layouts. By setting the option, the :index, :id, and :name fields on the Phoenix.HTML.Form struct are set in to_form/4:

offset_string = Integer.to_string(offset)

%Phoenix.HTML.Form{
  index: offset,
  id: id <> "_" <> offset_string,
  name: name <> "[" <> offset_string <> "]",
  # ...
}

This worked fine with the old inputs_for function that was moved to PhoenixHTMLHelpers. However, this stopped working with Phoenix.Component.inputs_for/1. I can still pass the offset option (or any other option) to make it available in the form data implementation:

~H"""
<.inputs_for
  :let={ff}
  field={@form[:filters]}
  options={[fields: @fields, offset: 5]}
>
  <%!-- ... %>
</.inputs_for>
"""

And, given the offset 5, the protocol implementation produces the correct values in the Form struct:

%Phoenix.HTML.Form{
  id: "flop_filters_5",
  name: "filters[5]",
  index: 5,
  # ...
}

However, Phoenix.Component.inputs_for/1 appears to ignore both the index and the id that the protocol implementation produces. The form structs passed to the inner block always start with index 0 and an according ID (although the name is derived correctly from the form struct returned by the protocol implementation):

%Phoenix.HTML.Form{
  id: "flop_filters_0",
  name: "filters[5]",
  hidden: [{"_persistent_id", "0"}, {:field, :email}],
  params: %{"_persistent_id" => "0"},
  index: 0
}

I noticed that I can initialize the params on the form struct produced by my protocol implementation with a _persistent_id to achieve the previous behavior:

%Phoenix.HTML.Form{
  index: index,
  id: id <> "_" <> index_string,
  name: name <> "[" <> index_string <> "]",
  params: %{"_persistent_id" => index}
  # ...
}

This leads to the correct index being used by inputs_for, but this relies on an internal implementation detail and probably isn’t meant to be used this way.

I don’t know why it was implemented this way, and whether this was an oversight, or what the purpose of the _persistent_id is, or whether I’m doing something wrong. Shouldn't inputs_for retrieve everything it can from the Form struct?

@SteffenDE
Copy link
Collaborator

@woylie the persistent_id thing was added by Chris to correctly support reordering (#2570). The way I understand it is that it ensures that even when forms are re-ordered, the fields retain their original ID and therefore morphdom does not recreate them. I think we can change it to start at the given index without problems, but I don't think we can use the ID from the form, as that would introduce changing IDs again. When using inputs_for outside of LiveView, it wouldn't be a problem, so maybe there should be a way to opt out of _persistent_id?

cc @chrismccord

@woylie
Copy link
Contributor Author

woylie commented Feb 12, 2025

Starting at the given index would solve my use case.

When using inputs_for outside of LiveView, it wouldn't be a problem, so maybe there should be a way to opt out of _persistent_id?

Even within LiveView, reordering inputs isn't relevant for most filter forms. Neither is the tracking of unused fields (this was commented on here). Usually, we prefer to keep the filter parameters as query parameters in the URL. The addition of _persistent_id and _unused parameters leads to unnecessarily long URLs without any benefit. We can remove those parameters in handle_event, but it would be nice to be able to opt out of both indeed.

SteffenDE added a commit that referenced this issue Feb 13, 2025
@SteffenDE SteffenDE linked a pull request Feb 13, 2025 that will close this issue
SteffenDE added a commit that referenced this issue Feb 13, 2025
@SteffenDE
Copy link
Collaborator

@woylie I opened #3677 to opt out of the persistent_id generation. I did not change it to use the forms' indexes, because I'm not sure about the consequences. Let me know if that works for you :)

@woylie
Copy link
Contributor Author

woylie commented Feb 18, 2025

@SteffenDE My test unskipped in https://github.com/woylie/flop_phoenix/pull/430/files passes again with this change, so this appears to resolve the issue. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants