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

Bump phoenix_html to 4.1.1 #379

Merged
merged 5 commits into from
Jun 21, 2024
Merged

Bump phoenix_html to 4.1.1 #379

merged 5 commits into from
Jun 21, 2024

Conversation

guess
Copy link
Contributor

@guess guess commented Jun 20, 2024

The inputs_for and hidden_inputs_for functions have been moved to phoenix_html_helpers

https://hexdocs.pm/phoenix_html/changelog.html#v4-0-0-2023-12-19

@Flo0807
Copy link
Collaborator

Flo0807 commented Jun 20, 2024

The app doesn't start:

app-1 | Because "your app" depends on "phoenix_html empty" which doesn't match any versions, version solving failed.
app-1 | ** (Mix) Hex dependency resolution failed
app-1 exited with code 1

This is because we also have phoenix_html as a dependency in our demo project, not only in Backpex itself (see demo/mix.exs).

I deleted the dependency from the demo project. This will start the app, but raise an error if I try to access http://localhost:4000/admin/users/new.

app-1 | ** (Protocol.UndefinedError) protocol Enumerable not implemented for {:safe, []} of type Tuple. This protocol is implemented for the following type(s): CircularBuffer, DBConnection.PrepareStream, DBConnection.Stream, Date.Range, Ecto.Adapters.SQL.Stream, File.Stream, Floki.HTMLTree, Function, GenEvent.Stream, HashDict, HashSet, IO.Stream, Jason.OrderedObject, List, Map, MapSet, Phoenix.LiveView.LiveStream, Postgrex.Stream, Range, Stream
app-1 | (elixir 1.15.7) lib/enum.ex:1: Enumerable.impl_for!/1
app-1 | (elixir 1.15.7) lib/enum.ex:166: Enumerable.reduce/3
app-1 | (elixir 1.15.7) lib/enum.ex:4387: Enum.filter/2
app-1 | (backpex 0.3.2) lib/backpex/fields/has_many_through.ex:180: Backpex.Fields.HasManyThrough.render_form/1

The reason might be that we do not use the inputs_for function in Backpex.Fields.HasManyThrough#L507 as intended. The HasManyThrough field is still in beta state and there needs to be some refactoring done, before we can safely update to phoenix_html v4, I think.

@guess
Copy link
Contributor Author

guess commented Jun 20, 2024

Ah sorry I didn't see the demo project. I can try to take a look at the previous state and what happens on the update to see if there's any obvious fixes :)

@Flo0807
Copy link
Collaborator

Flo0807 commented Jun 20, 2024

Ah sorry I didn't see the demo project. I can try to take a look at the previous state and what happens on the update to see if there's any obvious fixes :)

That's awesome, thanks! 🙏

@pehbehbeh
Copy link
Member

I have come a bit further. We need to change this line:

-    inputs_for(form, association.pivot.field)
+    form.impl.to_form(form.source, form, association.pivot.field, [])

Because before we were using this variant of inputs_for/3 of which is now deprecated. But the change above replicates the old behaviour.

The field is still broken though...

@guess
Copy link
Contributor Author

guess commented Jun 20, 2024

The addresses field doesn't seem to work on the latest develop either – selecting any values has no effect.
You can also see that happening here actually: https://backpex.live/admin/users/new

@Flo0807
Copy link
Collaborator

Flo0807 commented Jun 21, 2024

The addresses field doesn't seem to work on the latest develop either – selecting any values has no effect. You can also see that happening here actually: https://backpex.live/admin/users/new

Thanks for your report. I provided a fix for this in #387. The change will be on the website after the next release.

@pehbehbeh
Copy link
Member

The addresses field doesn't seem to work on the latest develop either

Yep, thats what I meant with:

The field is still broken though...

Thanks @Flo0807 for the fix! :-)

Then all we need here is the update in the demo and the line change I suggested.

@guess Are you going to finish this or should I take over?

@Flo0807 Flo0807 added the dependencies Pull requests that update a dependency file label Jun 21, 2024
@guess
Copy link
Contributor Author

guess commented Jun 21, 2024

Yay! Pushed and tested that it works in the demo project 🎉 @Flo0807 @pehbehbeh

@Flo0807 Flo0807 merged commit 27e2f30 into naymspace:develop Jun 21, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants