Skip to content

Commit

Permalink
Deprecate Phoenix.Endpoint.init/2
Browse files Browse the repository at this point in the history
  • Loading branch information
@josevalim
josevalim committed Dec 24, 2023
1 parent 8416fb6 commit 5539590
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 87 deletions.
15 changes: 0 additions & 15 deletions lib/phoenix/endpoint.ex
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,14 +290,6 @@ defmodule Phoenix.Endpoint do
"""
@callback config_change(changed :: term, removed :: term) :: term

@doc """
Initialize the endpoint configuration.
Invoked when the endpoint supervisor starts, allows dynamically
configuring the endpoint from system environment or other runtime sources.
"""
@callback init(:supervisor, config :: Keyword.t()) :: {:ok, Keyword.t()}

# Paths and URLs

@doc """
Expand Down Expand Up @@ -422,13 +414,6 @@ defmodule Phoenix.Endpoint do

# Avoid unused variable warnings
_ = var!(code_reloading?)

@doc false
def init(_key, config) do
{:ok, config}
end

defoverridable init: 2
end
end

Expand Down
24 changes: 13 additions & 11 deletions lib/phoenix/endpoint/supervisor.ex
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,21 @@ defmodule Phoenix.Endpoint.Supervisor do
env_conf = config(otp_app, mod, default_conf)

secret_conf =
case mod.init(:supervisor, env_conf) do
{:ok, init_conf} ->
if is_nil(Application.get_env(otp_app, mod)) and init_conf == env_conf do
Logger.warning(
"no configuration found for otp_app #{inspect(otp_app)} and module #{inspect(mod)}"
)
end

cond do
Code.ensure_loaded?(mod) and function_exported?(mod, :init, 2) ->
IO.warn("#{inspect(mod)}.init/2 is deprecated, use config/runtime.exs instead")
{:ok, init_conf} = mod.init(:supervisor, env_conf)
init_conf

other ->
raise ArgumentError,
"expected init/2 callback to return {:ok, config}, got: #{inspect(other)}"
is_nil(Application.get_env(otp_app, mod)) ->
Logger.warning(
"no configuration found for otp_app #{inspect(otp_app)} and module #{inspect(mod)}"
)

env_conf

true ->
env_conf
end

extra_conf = [
Expand Down
118 changes: 74 additions & 44 deletions test/phoenix/endpoint/endpoint_test.exs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@ defmodule Phoenix.Endpoint.EndpointTest do
use ExUnit.Case, async: true
use RouterHelper

@config [url: [host: {:system, "ENDPOINT_TEST_HOST"}, path: "/api"],
static_url: [host: "static.example.com"],
server: false, http: [port: 80], https: [port: 443],
force_ssl: [subdomains: true],
cache_manifest_skip_vsn: false,
cache_static_manifest: "../../../../test/fixtures/digest/compile/cache_manifest.json",
pubsub_server: :endpoint_pub]
@config [
url: [host: {:system, "ENDPOINT_TEST_HOST"}, path: "/api"],
static_url: [host: "static.example.com"],
server: false,
http: [port: 80],
https: [port: 443],
force_ssl: [subdomains: true],
cache_manifest_skip_vsn: false,
cache_static_manifest: "../../../../test/fixtures/digest/compile/cache_manifest.json",
pubsub_server: :endpoint_pub
]

Application.put_env(:phoenix, __MODULE__.Endpoint, @config)

Expand All @@ -39,24 +43,24 @@ defmodule Phoenix.Endpoint.EndpointTest do
end

setup_all do
ExUnit.CaptureLog.capture_log(fn -> start_supervised! Endpoint end)
start_supervised! {Phoenix.PubSub, name: :endpoint_pub}
on_exit fn -> Application.delete_env(:phoenix, :serve_endpoints) end
ExUnit.CaptureLog.capture_log(fn -> start_supervised!(Endpoint) end)
start_supervised!({Phoenix.PubSub, name: :endpoint_pub})
on_exit(fn -> Application.delete_env(:phoenix, :serve_endpoints) end)
:ok
end

test "defines child_spec/1" do
assert Endpoint.child_spec([]) == %{
id: Endpoint,
start: {Endpoint, :start_link, [[]]},
type: :supervisor
}
id: Endpoint,
start: {Endpoint, :start_link, [[]]},
type: :supervisor
}
end

test "warns if there is no configuration for an endpoint" do
assert ExUnit.CaptureLog.capture_log(fn ->
NoConfigEndpoint.start_link()
end) =~ "no configuration"
NoConfigEndpoint.start_link()
end) =~ "no configuration"
end

test "has reloadable configuration" do
Expand All @@ -75,10 +79,13 @@ defmodule Phoenix.Endpoint.EndpointTest do

assert Endpoint.config_change([{Endpoint, config}], []) == :ok
assert Endpoint.config(:endpoint_id) == endpoint_id

assert Enum.sort(Endpoint.config(:url)) ==
[host: {:system, "ENDPOINT_TEST_HOST"}, path: "/api", port: 1234]
[host: {:system, "ENDPOINT_TEST_HOST"}, path: "/api", port: 1234]

assert Enum.sort(Endpoint.config(:static_url)) ==
[host: "static.example.com", port: 456]
[host: "static.example.com", port: 456]

assert Endpoint.url() == "https://example.com:1234"
assert Endpoint.path("/") == "/api/"
assert Endpoint.static_url() == "https://static.example.com:456"
Expand Down Expand Up @@ -136,7 +143,7 @@ defmodule Phoenix.Endpoint.EndpointTest do
conn = conn(:get, "https://example.com/")
assert Endpoint.call(conn, []).script_name == ~w"api"

conn = put_in conn.script_name, ~w(foo)
conn = put_in(conn.script_name, ~w(foo))
assert Endpoint.call(conn, []).script_name == ~w"api"
end

Expand All @@ -149,19 +156,25 @@ defmodule Phoenix.Endpoint.EndpointTest do

test "sends hsts on https requests on force_ssl" do
conn = Endpoint.call(conn(:get, "https://example.com/"), [])

assert get_resp_header(conn, "strict-transport-security") ==
["max-age=31536000; includeSubDomains"]
["max-age=31536000; includeSubDomains"]
end

test "warms up caches on load and config change" do
assert Endpoint.config_change([{Endpoint, @config}], []) == :ok

assert Endpoint.config(:cache_static_manifest_latest) ==
%{"foo.css" => "foo-d978852bea6530fcd197b5445ed008fd.css"}

assert Endpoint.static_path("/foo.css") == "/foo-d978852bea6530fcd197b5445ed008fd.css?vsn=d"

# Trigger a config change and the cache should be warmed up again
config = put_in(@config[:cache_static_manifest], "../../../../test/fixtures/digest/compile/cache_manifest_upgrade.json")
config =
put_in(
@config[:cache_static_manifest],
"../../../../test/fixtures/digest/compile/cache_manifest_upgrade.json"
)

assert Endpoint.config_change([{Endpoint, config}], []) == :ok
assert Endpoint.config(:cache_static_manifest_latest) == %{"foo.css" => "foo-ghijkl.css"}
Expand Down Expand Up @@ -190,27 +203,14 @@ defmodule Phoenix.Endpoint.EndpointTest do
"/foo-d978852bea6530fcd197b5445ed008fd.css?vsn=d#info#me"
end

@tag :capture_log
test "invokes init/2 callback" do
defmodule InitEndpoint do
use Phoenix.Endpoint, otp_app: :phoenix

def init(:supervisor, opts) do
send opts[:parent], {self(), :sample}
{:ok, opts}
end
end

{:ok, pid} = InitEndpoint.start_link(parent: self())
assert_receive {^pid, :sample}
end

@tag :capture_log
test "uses url configuration for static path" do
Application.put_env(:phoenix, __MODULE__.UrlEndpoint, url: [path: "/api"])

defmodule UrlEndpoint do
use Phoenix.Endpoint, otp_app: :phoenix
end

UrlEndpoint.start_link()
assert UrlEndpoint.path("/phoenix.png") =~ "/api/phoenix.png"
assert UrlEndpoint.static_path("/phoenix.png") =~ "/api/phoenix.png"
Expand All @@ -219,9 +219,11 @@ defmodule Phoenix.Endpoint.EndpointTest do
@tag :capture_log
test "uses static_url configuration for static path" do
Application.put_env(:phoenix, __MODULE__.StaticEndpoint, static_url: [path: "/static"])

defmodule StaticEndpoint do
use Phoenix.Endpoint, otp_app: :phoenix
end

StaticEndpoint.start_link()
assert StaticEndpoint.path("/phoenix.png") =~ "/phoenix.png"
assert StaticEndpoint.static_path("/phoenix.png") =~ "/static/phoenix.png"
Expand All @@ -245,35 +247,63 @@ defmodule Phoenix.Endpoint.EndpointTest do

test "injects pubsub broadcast with configured server" do
Endpoint.subscribe("sometopic")
some = spawn fn -> :ok end
some = spawn(fn -> :ok end)

Endpoint.broadcast_from(some, "sometopic", "event1", %{key: :val})

assert_receive %Phoenix.Socket.Broadcast{
event: "event1", payload: %{key: :val}, topic: "sometopic"}
event: "event1",
payload: %{key: :val},
topic: "sometopic"
}

Endpoint.broadcast_from!(some, "sometopic", "event2", %{key: :val})

assert_receive %Phoenix.Socket.Broadcast{
event: "event2", payload: %{key: :val}, topic: "sometopic"}
event: "event2",
payload: %{key: :val},
topic: "sometopic"
}

Endpoint.broadcast("sometopic", "event3", %{key: :val})

assert_receive %Phoenix.Socket.Broadcast{
event: "event3", payload: %{key: :val}, topic: "sometopic"}
event: "event3",
payload: %{key: :val},
topic: "sometopic"
}

Endpoint.broadcast!("sometopic", "event4", %{key: :val})

assert_receive %Phoenix.Socket.Broadcast{
event: "event4", payload: %{key: :val}, topic: "sometopic"}
event: "event4",
payload: %{key: :val},
topic: "sometopic"
}

Endpoint.local_broadcast_from(some, "sometopic", "event1", %{key: :val})

assert_receive %Phoenix.Socket.Broadcast{
event: "event1", payload: %{key: :val}, topic: "sometopic"}
event: "event1",
payload: %{key: :val},
topic: "sometopic"
}

Endpoint.local_broadcast("sometopic", "event3", %{key: :val})

assert_receive %Phoenix.Socket.Broadcast{
event: "event3", payload: %{key: :val}, topic: "sometopic"}
event: "event3",
payload: %{key: :val},
topic: "sometopic"
}
end

test "loads cache manifest from specified application" do
config = put_in(@config[:cache_static_manifest], {:phoenix, "../../../../test/fixtures/digest/compile/cache_manifest.json"})
config =
put_in(
@config[:cache_static_manifest],
{:phoenix, "../../../../test/fixtures/digest/compile/cache_manifest.json"}
)

assert Endpoint.config_change([{Endpoint, config}], []) == :ok
assert Endpoint.static_path("/foo.css") == "/foo-d978852bea6530fcd197b5445ed008fd.css?vsn=d"
Expand Down
38 changes: 21 additions & 17 deletions test/phoenix/endpoint/supervisor_test.exs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ defmodule Phoenix.Endpoint.SupervisorTest do
end

defmodule ServerEndpoint do
def init(:supervisor, config), do: {:ok, config}
def __sockets__(), do: []
end

Expand Down Expand Up @@ -92,42 +91,47 @@ defmodule Phoenix.Endpoint.SupervisorTest do
end

import ExUnit.CaptureLog

test "logs info if :http or :https configuration is set but not :server when running in release" do
Logger.configure(level: :info)
# simulate running inside release
System.put_env("RELEASE_NAME", "phoenix-test")
Application.put_env(:phoenix, ServerEndpoint, [server: false, http: [], https: []])
Application.put_env(:phoenix, ServerEndpoint, server: false, http: [], https: [])

assert capture_log(fn ->
{:ok, {_, _children}} = Supervisor.init({:phoenix, ServerEndpoint, []})
end) =~ "Configuration :server"
{:ok, {_, _children}} = Supervisor.init({:phoenix, ServerEndpoint, []})
end) =~ "Configuration :server"

Application.put_env(:phoenix, ServerEndpoint, server: false, http: [])

Application.put_env(:phoenix, ServerEndpoint, [server: false, http: []])
assert capture_log(fn ->
{:ok, {_, _children}} = Supervisor.init({:phoenix, ServerEndpoint, []})
end) =~ "Configuration :server"
{:ok, {_, _children}} = Supervisor.init({:phoenix, ServerEndpoint, []})
end) =~ "Configuration :server"

Application.put_env(:phoenix, ServerEndpoint, server: false, https: [])

Application.put_env(:phoenix, ServerEndpoint, [server: false, https: []])
assert capture_log(fn ->
{:ok, {_, _children}} = Supervisor.init({:phoenix, ServerEndpoint, []})
end) =~ "Configuration :server"
{:ok, {_, _children}} = Supervisor.init({:phoenix, ServerEndpoint, []})
end) =~ "Configuration :server"

Application.put_env(:phoenix, ServerEndpoint, server: false)

Application.put_env(:phoenix, ServerEndpoint, [server: false])
refute capture_log(fn ->
{:ok, {_, _children}} = Supervisor.init({:phoenix, ServerEndpoint, []})
end) =~ "Configuration :server"
{:ok, {_, _children}} = Supervisor.init({:phoenix, ServerEndpoint, []})
end) =~ "Configuration :server"

Application.put_env(:phoenix, ServerEndpoint, server: true)

Application.put_env(:phoenix, ServerEndpoint, [server: true])
refute capture_log(fn ->
{:ok, {_, _children}} = Supervisor.init({:phoenix, ServerEndpoint, []})
end) =~ "Configuration :server"
{:ok, {_, _children}} = Supervisor.init({:phoenix, ServerEndpoint, []})
end) =~ "Configuration :server"

Application.delete_env(:phoenix, ServerEndpoint)
Logger.configure(level: :warning)
end

describe "watchers" do
defmodule WatchersEndpoint do
def init(:supervisor, config), do: {:ok, config}
def __sockets__(), do: []
end

Expand Down

6 comments on commit 5539590

@dvic
Copy link
Contributor

@dvic dvic commented on 5539590 Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josevalim What is the reason for this change? For regular apps I guess this isn't needed, but we have a reusable Phoenix Endpoint, in which case we don't have any global config (we want the users just to be able to mount the endpoint). Our CI broke on this commit and it was just a matter of removing @impl true, but now I have an ugly warning message each time I start ;)

@josevalim
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can either use config/runtime.exs or pass the options to the endpoint directly on start, such as {MyApp.Endpoint, override: 123}. As far as I can see, there isn't a benefit to this approach.

@dvic
Copy link
Contributor

@dvic dvic commented on 5539590 Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our case, the latter is way more convenient because we have tests with various different configurations and also scripts that start our system in different configurations. Using config/runtime.exs for all these usecases would get convoluted.

@josevalim
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said above, you don't need to use config/runtime.exs. You can pass the additional options as arguments in the supervision tree/start_link.

@dvic
Copy link
Contributor

@dvic dvic commented on 5539590 Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, I misunderstood your point. Arguments giving to the supervision tree works, but I lost a place to apply defaults then right? For example, we had something like this currently:

  def init(:supervisor, config) do
    mode_config =
      case Keyword.fetch!(config, :mode) do
        :dev ->
          port = Keyword.get(config, :port, 4000)

          [asset_base_url: Keyword.get(config, :asset_base_url, "http://assets.localhost:4002"), ...]

        ...
      end 
    ...
  end

So the recommended approach then here would be to use a wrapper childspec that applies the defaults?

@josevalim
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can apply defaults by passing them to start_link and you can also read them by using Application.get_env.

Please sign in to comment.