Repository: phoenixframework/phoenix
Pull request #4709 (2022-03-02 07:23:00)

Deprecate {:system, env_var} tuples for configuring endpoint urls
I was reading through the endpoint supervisor code and saw the following TODO so I thought I'd try tackling it.
```
# TODO: Deprecate {:system, env_var} once we require Elixir v1.9+
```
This adds the following log message when an endpoint's `:url` or `:static_url` function has a deprecated `{:system, env_var}` tuple.

```
00:14:41.051 [warn] :url configuration containing values of {:system, env_var} tuples for Phoenix.Endpoint.EndpointTest.SystemTupleEndpoint is deprecated.

Configuration:

    config :phoenix, Phoenix.Endpoint.EndpointTest.SystemTupleEndpoint,
      [host: {:system, "ENDPOINT_TEST_HOST"}, port: {:system, "ENDPOINT_TEST_PORT"}]

Either move these config values into config/runtime.exs:

    config :phoenix, Phoenix.Endpoint.EndpointTest.SystemTupleEndpoint,
      host: System.get_env("ENDPOINT_TEST_HOST"),
      port: System.get_env("ENDPOINT_TEST_PORT")

Or read them during initialization inside of the Phoenix.Endpoint.EndpointTest.SystemTupleEndpoint.init/2 callback.

    def init(:supervisor, config) do
      config
      |> put_in([:url, :host], System.get_env("ENDPOINT_TEST_HOST"))
      |> put_in([:url, :port], System.get_env("ENDPOINT_TEST_PORT"))

      {:ok, config}
    end
```

Please let me know what you think!

aaronrenner: I just reworked the log message because I realized some of the code samples were wrong. Here's the updated log message:

```
08:16:54.563 [warn] :url configuration containing {:system, env_var} tuples for Phoenix.Endpoint.EndpointTest.SystemTupleEndpoint is deprecated.

Configuration with deprecated values:

    config :phoenix, Phoenix.Endpoint.EndpointTest.SystemTupleEndpoint,
      url: [
        host: {:system, "ENDPOINT_TEST_HOST"},
        port: {:system, "ENDPOINT_TEST_PORT"}
      ]

Move this configuration into config/runtime.exs and replace the {:system, env_var} tuples
with System.get_env/1 function calls:

    config :phoenix, Phoenix.Endpoint.EndpointTest.SystemTupleEndpoint,
      url: [
        host: System.get_env("ENDPOINT_TEST_HOST"),
        port: System.get_env("ENDPOINT_TEST_PORT")
      ]
```
josevalim: No need to backport imo.

Patch:
diff --git a/lib/phoenix/endpoint/supervisor.ex b/lib/phoenix/endpoint/supervisor.ex
index 800fc7983..4b016c4de 100644
--- a/lib/phoenix/endpoint/supervisor.ex
+++ b/lib/phoenix/endpoint/supervisor.ex
@@ -97,6 +97,10 @@ defmodule Phoenix.Endpoint.Supervisor do
       Phoenix.CodeReloader.Server.check_symlinks()
     end
 
+    # TODO: Remove this once {:system, env_var} tuples are removed
+    warn_on_deprecated_system_env_tuples(otp_app, mod, conf, :url)
+    warn_on_deprecated_system_env_tuples(otp_app, mod, conf, :static_url)
+
     children =
       config_children(mod, secret_conf, default_conf) ++
       pubsub_children(mod, conf) ++
@@ -350,15 +354,43 @@ defmodule Phoenix.Endpoint.Supervisor do
     raise ArgumentError, "expected a path starting with a single / but got #{inspect path}"
   end
 
-  # TODO: Deprecate {:system, env_var} once we require Elixir v1.9+
+  # TODO: Remove the first function clause once {:system, env_var} tuples are removed
   defp host_to_binary({:system, env_var}), do: host_to_binary(System.get_env(env_var))
   defp host_to_binary(host), do: host
 
-  # TODO: Deprecate {:system, env_var} once we require Elixir v1.9+
+  # TODO: Remove the first function clause once {:system, env_var} tuples are removed
   defp port_to_integer({:system, env_var}), do: port_to_integer(System.get_env(env_var))
   defp port_to_integer(port) when is_binary(port), do: String.to_integer(port)
   defp port_to_integer(port) when is_integer(port), do: port
 
+  defp warn_on_deprecated_system_env_tuples(otp_app, mod, conf, key) do
+    deprecated_configs = Enum.filter(conf[key] || [], &match?({_, {:system, _}}, &1))
+
+    if Enum.any?(deprecated_configs) do
+      deprecated_config_lines = for {k, v} <- deprecated_configs, do: "#{k}: #{inspect(v)}"
+      runtime_exs_config_lines = for {key, {:system, env_var}} <- deprecated_configs, do: ~s|#{key}: System.get_env("#{env_var}")|
+
+      Logger.warn """
+      #{inspect(key)} configuration containing {:system, env_var} tuples for #{inspect(mod)} is deprecated.
+
+      Configuration with deprecated values:
+
+          config #{inspect(otp_app)}, #{inspect(mod)},
+            #{key}: [
+              #{deprecated_config_lines |> Enum.join(",\r\n        ")}
+            ]
+
+      Move this configuration into config/runtime.exs and replace the {:system, env_var} tuples
+      with System.get_env/1 function calls:
+
+          config #{inspect(otp_app)}, #{inspect(mod)},
+            #{key}: [
+              #{runtime_exs_config_lines |> Enum.join(",\r\n        ")}
+            ]
+      """
+    end
+  end
+
   @doc """
   Invoked to warm up caches on start and config change.
   """


Test patch:
diff --git a/test/phoenix/endpoint/endpoint_test.exs b/test/phoenix/endpoint/endpoint_test.exs
index 2df5e6a08..bb1e8c756 100644
--- a/test/phoenix/endpoint/endpoint_test.exs
+++ b/test/phoenix/endpoint/endpoint_test.exs
@@ -1,4 +1,7 @@
 System.put_env("ENDPOINT_TEST_HOST", "example.com")
+System.put_env("ENDPOINT_TEST_PORT", "80")
+System.put_env("ENDPOINT_TEST_ASSET_HOST", "assets.example.com")
+System.put_env("ENDPOINT_TEST_ASSET_PORT", "443")
 
 defmodule Phoenix.Endpoint.EndpointTest do
   use ExUnit.Case, async: true
@@ -28,6 +31,10 @@ defmodule Phoenix.Endpoint.EndpointTest do
     use Phoenix.Endpoint, otp_app: :phoenix
   end
 
+  defmodule SystemTupleEndpoint do
+    use Phoenix.Endpoint, otp_app: :phoenix
+  end
+
   setup_all do
     ExUnit.CaptureLog.capture_log(fn -> start_supervised! Endpoint end)
     start_supervised! {Phoenix.PubSub, name: :endpoint_pub}
@@ -75,6 +82,29 @@ defmodule Phoenix.Endpoint.EndpointTest do
     assert Endpoint.struct_url() == %URI{scheme: "https", host: "example.com", port: 1234}
   end
 
+  test "{:system, env_var} tuples are deprecated" do
+    Application.put_env(:phoenix, __MODULE__.SystemTupleEndpoint,
+      url: [
+        host: {:system, "ENDPOINT_TEST_HOST"},
+        port: {:system, "ENDPOINT_TEST_PORT"}
+      ],
+      static_url: [
+        host: {:system, "ENDPOINT_TEST_ASSET_HOST"},
+        port: {:system, "ENDPOINT_TEST_ASSET_PORT"}
+      ]
+    )
+
+    log =
+      ExUnit.CaptureLog.capture_log(fn ->
+        start_supervised!(SystemTupleEndpoint)
+      end)
+
+    assert log =~ ~S|host: System.get_env("ENDPOINT_TEST_HOST")|
+    assert log =~ ~S|port: System.get_env("ENDPOINT_TEST_PORT")|
+    assert log =~ ~S|host: System.get_env("ENDPOINT_TEST_ASSET_HOST")|
+    assert log =~ ~S|port: System.get_env("ENDPOINT_TEST_ASSET_PORT")|
+  end
+
   test "sets script name when using path" do
     conn = conn(:get, "https://example.com/")
     assert Endpoint.call(conn, []).script_name == ~w"api"
diff --git a/test/phoenix/socket/transport_test.exs b/test/phoenix/socket/transport_test.exs
index 4f1d794fa..74f642dac 100644
--- a/test/phoenix/socket/transport_test.exs
+++ b/test/phoenix/socket/transport_test.exs
@@ -1,5 +1,3 @@
-System.put_env("TRANSPORT_TEST_HOST", "host.com")
-
 defmodule Phoenix.Socket.TransportTest do
   use ExUnit.Case, async: true
   use RouterHelper
@@ -12,7 +10,7 @@ defmodule Phoenix.Socket.TransportTest do
 
   Application.put_env :phoenix, __MODULE__.Endpoint,
     force_ssl: [],
-    url: [host: {:system, "TRANSPORT_TEST_HOST"}],
+    url: [host: "host.com"],
     check_origin: ["//endpoint.com"],
     secret_key_base: @secret_key_base