Skip to content

Add Phoenix.HTML.Safe to Duration #463

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

Merged
merged 3 commits into from
Apr 30, 2025
Merged

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