Skip to content

Conversation

@joshua-bouv
Copy link
Contributor

I was creating a form which had a set of options, the values being durations

<.input
  label="Duration"
  field={@form[:duration]}
  type="select"
  options={[
    {"Daily", Duration.new!(day: 1)},
    {"Weekly", Duration.new!(week: 1)},
    {"Monthly", Duration.new!(month: 1)},
    {"Yearly", Duration.new!(year: 1)}
  ]}
/>

Duration has to_iso8601 and from_iso8601 functions so I assumed this would work like Date and Time does (auto converts to ISO), but Phoenix.HTML.Safe hadn't been implemented for Duration causing this error: protocol Phoenix.HTML.Safe not implemented for type Duration (a struct).

This PR adds the Phoenix.HTML.Safe implentation. Their might of been a reason why this wasn't added originally though, but im not too sure why that could be?

@SteffenDE
Copy link
Contributor

The minimum supported Elixir version is 1.7 at the moment

elixir: "~> 1.7",

which does not know anything about Duration, so we might need to conditionally define this? @josevalim

@joshua-bouv
Copy link
Contributor Author

The minimum supported Elixir version is 1.7 at the moment

elixir: "~> 1.7",

which does not know anything about Duration, so we might need to conditionally define this? @josevalim

Good spot!

If the decision is to addif Version.match?(System.version(), ">= 1.17.0") do around the function (and test) I have the commit on stand-by 👍

@josevalim josevalim closed this Apr 30, 2025
@josevalim josevalim reopened this Apr 30, 2025
@josevalim
Copy link
Member

I am almost sure that previous Elixir versions did not complain if you implemented a protocol for a module/struct that was not available. But this has changed on v1.18 or 1.19 because of the type system. Let's see what CI says.

@josevalim
Copy link
Member

@joshua-bouv yeah, you will need to wrap both the definition and the tests in if Code.ensure_loaded?(Duration) do blocks. :)

@joshua-bouv
Copy link
Contributor Author

Done 👍

@josevalim josevalim merged commit 49bb6e5 into phoenixframework:main Apr 30, 2025
2 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

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 this pull request may close these issues.

3 participants