Fix param handling for nested embeds

[nduitz]

This fixes two things:

a) It picks up the correct action (not from the changeset but from the parent form)
b) It fixes handling parameters for cardinality :many

If you remove the first commit you will see the test is failing and it will produce the incorrect form.
Instead of generating two forms per polymorphic type it will generate only one form for the device type

[rmoorman]

This PR seems to fix some of the immediate form validation problems I was having (even though the issue you were mentioning this PR in (#113) is about polymorphic_embeds_many, and I was having trouble with polymorphic_embeds_one).

There seems to be one failing test though

  1) test form with improved param handling for different param types (PolymorphicEmbedTest)
     test/polymorphic_embed_test.exs:3234
     Assertion with == failed
     code:  assert f.params == %{"ref" => "789"}
     left:  %{"0" => %{"ref" => "789"}, "1" => %{"address" => "012 Oak Ave"}}
     right: %{"ref" => "789"}

I am not sure why this is happening though, as right now I didn't dive deep enough into phoenix form handling yet. Maybe it is happening because recent changes in phoenix html/form handling?

Furthermore, you mentioned in #113 that there were issues with the sort params as well, specifically with the approach taken in this PR.

Did you find some time to have another look at this yet @nduitz ?

[nduitz]

Hey there,
we are currently running this branch in our project which works well so far: https://github.com/nduitz/polymorphic_embed/commits/patch-2

One thing I noticed yesterday which caused me a lot of headaches was that polymorphic_embed_inputs_for does not render hidden id inputs even though we do not specify skip_hidden: true.
Will look into that once we finished implementing all forms and continue to refine them.

[nduitz]

Hey there, we are currently running this branch in our project which works well so far: https://github.com/nduitz/polymorphic_embed/commits/patch-2

One thing I noticed yesterday which caused me a lot of headaches was that polymorphic_embed_inputs_for does not render hidden id inputs even though we do not specify skip_hidden: true. Will look into that once we finished implementing all forms and continue to refine them.

Never mind my comment about polymorphic_embed_inputs_for not rendering a hidden id field. That is the same for the inputs_for implementation of phoenix_live_view. Somehow I assumed inputs_for does this.

[coladarci]

I would love for this to be merged as we just ran into this today and are jumping through hoops to get around it.

[mathieuprog]

@coladarci @nduitz it is currently in Draft status and it has conflicts. Can anyone resolve and test this?

[coladarci]

My fix was off latest release but it appears master is pretty far ahead? I'll investigate how to apply my fix to latest master and open a new PR as a backup.. thanks for the reply! I love this library and find myself going to it in project after project!

[nduitz]

I am actually running another commit in prod right now that fixed all of my issues. Good thing is, I implemented tests for all problems I encountered. I will have a look at it next week, checkout the latest main, apply the tests and see if I can reproduce the issues and fix them again.

[nduitz]

Nevermind, could do it now.

2fcfcbe and 0bd61b5 apply the fixes of the original PR
Additionally I added:
a34caba and ee6d450which show and tries to fix that params are not carried on when using polymorphic_embed_inputs_for
Changes simply get lost in certain cases and are replaced with empty structs.

But I can't remember in which cases. I could investigate more and create a minimal reproduction repo if you want. But that would take some time :)

[coladarci]

Thanks @nduitz !! Yeah I had issues with deeply embedded changesets being nuked.

[nduitz]

With #115 now being merged, this only two commits could be dropped from this. I didn't realize I had another PR open actually :D It's been a while. Sorry for that

[mathieuprog]

I asked ChatGPT 5.4 xthink for a review and it says:

Child error visibility no longer follows the parent form action. In helpers.ex:116 the %Ecto.Changeset{} branch skips apply_action(parent_action), even though the helper’s own contract says a parent with no action should suppress child errors at helpers.ex:144. I reproduced this on the PR head with a parent form whose action was nil: the nested child changeset still had :insert, and the rendered child form exposed validation errors. The changed expectation at polymorphic_embed_test.exs:2803 now locks that regression in.

wdyt?

[mathieuprog]

Added the failing test

[nduitz]

Thanks :) I will have a look at the implementation

[nduitz]

Hey, sorry this took me a while.

I actually made the wrong assumption in the test, which led to me introducing another bug :D
What my code should fix, is that data is lost and replaced by a changeset.
But imho polymorphic embeds should mirror the behaviour of classic embeds as closely as possible.

I changed the intial assumption (fix up commit) and added an fix for the bug I introduced :)

[mathieuprog]

Added failing tests. ChatGPT said:

---

1. The new %Ecto.Changeset{} branch only calls apply_action/2, and that helper is a no-op when the parent action is non-nil. Repro: build an invalid child email changeset with action: nil, put it into the parent, and set the parent action to :insert; to_form/4 returns form.action == :insert but form.errors == [] while form.source.errors still contains the address validation error. Regular inputs_for promotes the child action in the same setup, so this still does not fully honor the 'take the action from the parent form' contract.

2. embeds_many params are still not row-local for map-backed params" body="This still wraps the raw %{"0" => ..., "1" => ...} params map as a single list element, so valid sorted embeds_many rows end up with params like %{"0" => ..., "1" => ...} for the first child and %{} for the later ones instead of one params map per row. I reproduced that with a sorted contexts payload on the PR head. The visible input values may still fall back to data, but PolymorphicEmbed.HTML.Component reads f.params["_persistent_id"] to preserve stable row ids, so valid rows will keep losing their per-row param metadata."

---

In other words:

1. Parent action is still only half-fixed in helpers.ex (line 116). If the nested value is already an %Ecto.Changeset{} and its own action is nil, a parent form with action: :insert still suppresses the child errors. I reproduced form.action == :insert with form.errors == [] and form.source.errors != []. Ordinary inputs_for does not behave that way.

2. :many param handling is still wrong in helpers.ex (line 56). For string-keyed map params plus sorting, the helper does not give each child form its own params slice. Regular inputs_for does. That matters because component.ex (line 77) relies on per-row params["_persistent_id"] to keep stable IDs.

---

In other words:

Finding 1 is not a regression, but it is still an incomplete fix. On the base commit, the same repro already suppresses child errors for a prebuilt invalid child changeset under a parent with action: :insert. In /private/tmp/polph-132-base, to_form/4 produced form_action: :insert and form_errors: [] for that setup. The PR changes the internal failure mode in helpers.ex (line 116): before, the helper effectively erased the child errors from f.source; now it preserves them on f.source.errors but still fails to promote the child action, so f.errors stays empty. So the bug is real, but it is not newly introduced by this PR. It is better described as “the new %Ecto.Changeset{} path still does not complete the parent-action fix.”

Finding 2 is also not a regression. The bad :many params behavior already exists on the base commit, and the relevant extraction line is unchanged: head helpers (line 56) matches base helpers (line 56). Running the same sorted string-keyed params repro on the base commit produced the same result as head: sorted row structs were correct, but row params came back as [%{"0" => ..., "1" => ...}, %{}] instead of one params map per row. So this is a pre-existing bug that the PR still does not fix.

[mathieuprog]

sorry to throw that at you like that btw lol