Repository: phoenixframework/phoenix
Pull request #4288 (2021-04-12 11:28:12)

Change vsn query in assets url to be configurable
When generating `static_path` or `static_url` as well as when running `mix phx.digest`it would be nice to have the option of the generated urls to have the `vsn=d`query or not.
This PR adds the possibility and maintains the current behavior as the default, being opt-in.
The `vsn` option as added to the runtime configuration `:url` of `Phoenix.Endpoint` as it looked like an expected place for it.
This PR is also related to #1784

josevalim: Thanks for the PR @0urobor0s but I want to point out that vsn=d is a requirement if you are using Plug.Static, as it feels Plug.Static to set long expiry dates. You can only skip it if you are using something else to server assets. So given the potential footgun, I would prefer to not merge this unless there is a strong use case for this feature. Thank you!


Patch:
diff --git a/lib/mix/tasks/phx.digest.ex b/lib/mix/tasks/phx.digest.ex
index 970d2b9d8..e17271268 100644
--- a/lib/mix/tasks/phx.digest.ex
+++ b/lib/mix/tasks/phx.digest.ex
@@ -33,19 +33,26 @@ defmodule Mix.Tasks.Phx.Digest do
     * app-eb0a5b9302e8d32828d8a73f137cc8f0.js.gz
     * cache_manifest.json
 
+  ## vsn
+
+  It is possible to digest the stylesheet asset references without the query
+  string "?vsn=d" with the option `--no-vsn`.
   """
 
+  @default_opts [vsn: true]
+
   @doc false
   def run(all_args) do
     Mix.Task.run "compile", all_args
-    {opts, args, _} = OptionParser.parse(all_args, switches: [output: :string], aliases: [o: :output])
+    {opts, args, _} = OptionParser.parse(all_args, switches: [output: :string, vsn: :boolean], aliases: [o: :output])
     input_path = List.first(args) || @default_input_path
     output_path = opts[:output] || input_path
+    with_vsn? = Keyword.merge(@default_opts, opts)[:vsn]
 
     Mix.Task.run "deps.loadpaths", all_args
     {:ok, _} = Application.ensure_all_started(:phoenix)
 
-    case Phoenix.Digester.compile(input_path, output_path) do
+    case Phoenix.Digester.compile(input_path, output_path, with_vsn?) do
       :ok ->
         # We need to call build structure so everything we have
         # generated into priv is copied to _build in case we have
diff --git a/lib/phoenix/digester.ex b/lib/phoenix/digester.ex
index 559bcb73b..f20fe7dde 100644
--- a/lib/phoenix/digester.ex
+++ b/lib/phoenix/digester.ex
@@ -17,15 +17,15 @@ defmodule Phoenix.Digester do
   Digests and compresses the static files in the given `input_path`
   and saves them in the given `output_path`.
   """
-  @spec compile(String.t(), String.t()) :: :ok | {:error, :invalid_path}
-  def compile(input_path, output_path) do
+  @spec compile(String.t(), String.t(), boolean()) :: :ok | {:error, :invalid_path}
+  def compile(input_path, output_path, with_vsn?) do
     if File.exists?(input_path) do
       File.mkdir_p!(output_path)
 
       files = filter_files(input_path)
       latest = generate_latest(files)
       digests = load_compile_digests(output_path)
-      digested_files = Enum.map(files, &digested_contents(&1, latest))
+      digested_files = Enum.map(files, &digested_contents(&1, latest, with_vsn?))
 
       save_manifest(digested_files, latest, digests, output_path)
       Enum.each(digested_files, &write_to_disk(&1, output_path))
@@ -178,12 +178,12 @@ defmodule Phoenix.Digester do
     file
   end
 
-  defp digested_contents(file, latest) do
+  defp digested_contents(file, latest, with_vsn?) do
     ext = Path.extname(file.filename)
 
     digested_content =
       case ext do
-        ".css" -> digest_stylesheet_asset_references(file, latest)
+        ".css" -> digest_stylesheet_asset_references(file, latest, with_vsn?)
         ".js" -> digest_javascript_asset_references(file, latest)
         ".map" -> digest_javascript_map_asset_references(file, latest)
         _ -> file.content
@@ -195,14 +195,14 @@ defmodule Phoenix.Digester do
   @stylesheet_url_regex ~r{(url\(\s*)(\S+?)(\s*\))}
   @quoted_text_regex ~r{\A(['"])(.+)\1\z}
 
-  defp digest_stylesheet_asset_references(file, latest) do
+  defp digest_stylesheet_asset_references(file, latest, with_vsn?) do
     Regex.replace(@stylesheet_url_regex, file.content, fn _, open, url, close ->
       case Regex.run(@quoted_text_regex, url) do
         [_, quote_symbol, url] ->
-          open <> quote_symbol <> digested_url(url, file, latest, true) <> quote_symbol <> close
+          open <> quote_symbol <> digested_url(url, file, latest, with_vsn?) <> quote_symbol <> close
 
         nil ->
-          open <> digested_url(url, file, latest, true) <> close
+          open <> digested_url(url, file, latest, with_vsn?) <> close
       end
     end)
   end
diff --git a/lib/phoenix/endpoint.ex b/lib/phoenix/endpoint.ex
index 21cf6ca34..600ac464d 100644
--- a/lib/phoenix/endpoint.ex
+++ b/lib/phoenix/endpoint.ex
@@ -112,7 +112,15 @@ defmodule Phoenix.Endpoint do
       digest version. This is automatically loaded from `cache_static_manifest` on
       boot. However, if you have your own static handling mechanism, you may want to
       set this value explicitly. This is used by projects such as `LiveView` to
-      detect if the client is running on the latest version of all assets
+      detect if the client is running on the latest version of all assets.
+
+    * `:cache_manifest_skip_vsn` - when true, skips the appended query string 
+      "?vsn=d" when generatic paths to static assets. This query string is used
+      by `Plug.Static` to set long expiry dates, therefore, you should set this
+      option to true only if you are not using `Plug.Static` to serve assets,
+      for example, if you are using a CDN. If you are setting this option, you
+      should also consider passing `--no-vsn` to `mix phx.digest`. Defaults to
+      `false`.
 
     * `:check_origin` - configure the default `:check_origin` setting for
       transports. See `socket/3` for options. Defaults to `true`.
diff --git a/lib/phoenix/endpoint/supervisor.ex b/lib/phoenix/endpoint/supervisor.ex
index 49e4077bb..03a486c91 100644
--- a/lib/phoenix/endpoint/supervisor.ex
+++ b/lib/phoenix/endpoint/supervisor.ex
@@ -210,6 +210,7 @@ defmodule Phoenix.Endpoint.Supervisor do
      secret_key_base: nil,
      static_url: nil,
      url: [host: "localhost", path: "/"],
+     cache_manifest_skip_vsn: false,
 
      # Supervisor config
      watchers: []]
@@ -389,10 +390,11 @@ defmodule Phoenix.Endpoint.Supervisor do
 
   defp warmup_static(endpoint, %{"latest" => latest, "digests" => digests}) do
     Phoenix.Config.put_new(endpoint, :cache_static_manifest_latest, latest)
+    with_vsn? = !endpoint.config(:cache_manifest_skip_vsn)
 
     Enum.each(latest, fn {key, _} ->
       Phoenix.Config.cache(endpoint, {:__phoenix_static__, "/" <> key}, fn _ ->
-        {:cache, static_cache(digests, Map.get(latest, key))}
+        {:cache, static_cache(digests, Map.get(latest, key), with_vsn?)}
       end)
     end)
   end
@@ -401,10 +403,14 @@ defmodule Phoenix.Endpoint.Supervisor do
     raise ArgumentError, "expected warmup_static/2 to include 'latest' and 'digests' keys in manifest"
   end
 
-  defp static_cache(digests, value) do
+  defp static_cache(digests, value, true) do
     {"/#{value}?vsn=d", static_integrity(digests[value]["sha512"])}
   end
 
+  defp static_cache(digests, value, false) do
+    {"/#{value}", static_integrity(digests[value]["sha512"])}
+  end
+
   defp static_integrity(nil), do: nil
   defp static_integrity(sha), do: "sha512-#{sha}"
 


Test patch:
diff --git a/test/phoenix/digester_test.exs b/test/phoenix/digester_test.exs
index 78594f2e6..b52971502 100644
--- a/test/phoenix/digester_test.exs
+++ b/test/phoenix/digester_test.exs
@@ -12,12 +12,12 @@ defmodule Phoenix.DigesterTest do
 
   describe "compile" do
     test "fails when the given paths are invalid" do
-      assert {:error, :invalid_path} = Phoenix.Digester.compile("nonexistent path", "/ ?? /path")
+      assert {:error, :invalid_path} = Phoenix.Digester.compile("nonexistent path", "/ ?? /path", true)
     end
 
     test "digests and compress files" do
       input_path = "test/fixtures/digest/priv/static/"
-      assert :ok = Phoenix.Digester.compile(input_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, true)
       output_files = assets_files(@output_path)
 
       assert "phoenix.png" in output_files
@@ -61,7 +61,7 @@ defmodule Phoenix.DigesterTest do
           Path.join(@output_path, "cache_manifest.json")
         )
 
-      assert :ok = Phoenix.Digester.compile(input_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, true)
 
       json = Path.join(@output_path, "cache_manifest.json") |> json_read!()
 
@@ -88,13 +88,13 @@ defmodule Phoenix.DigesterTest do
 
     test "excludes compiled files" do
       input_path = "test/fixtures/digest/priv/static/"
-      assert :ok = Phoenix.Digester.compile(input_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, true)
       output_files = assets_files(@output_path)
 
       json = Path.join(@output_path, "cache_manifest.json") |> json_read!()
       refute json["latest"]["precompressed.js.gz"]
 
-      assert :ok = Phoenix.Digester.compile(@output_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(@output_path, @output_path, true)
       assert output_files == assets_files(@output_path)
     end
 
@@ -119,7 +119,7 @@ defmodule Phoenix.DigesterTest do
 
       File.write!(Path.join(input_path, "foo.css"), ".foo { background-color: blue }")
 
-      assert :ok = Phoenix.Digester.compile(input_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, true)
       json = Path.join(@output_path, "cache_manifest.json") |> json_read!()
 
       assert json["digests"]["foo-d978852bea6530fcd197b5445ed008fd.css"]["mtime"] == 32_132_171
@@ -140,7 +140,7 @@ defmodule Phoenix.DigesterTest do
           Path.join(input_path, "cache_manifest.json")
         )
 
-      assert :ok = Phoenix.Digester.compile(input_path, input_path)
+      assert :ok = Phoenix.Digester.compile(input_path, input_path, true)
 
       json = Path.join(input_path, "cache_manifest.json") |> json_read!()
       assert json["digests"] == %{}
@@ -148,7 +148,7 @@ defmodule Phoenix.DigesterTest do
 
     test "digests and compress nested files" do
       input_path = "test/fixtures/digest/priv/"
-      assert :ok = Phoenix.Digester.compile(input_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, true)
 
       output_files = assets_files(@output_path)
 
@@ -171,13 +171,13 @@ defmodule Phoenix.DigesterTest do
       File.mkdir_p!(@output_path)
 
       File.write!(input_file, "console.log('test');")
-      assert :ok = Phoenix.Digester.compile(input_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, true)
 
       json1 = Path.join(@output_path, "cache_manifest.json") |> json_read!()
       assert Enum.count(json1["digests"]) == 1
 
       File.write!(input_file, "console.log('test2');")
-      assert :ok = Phoenix.Digester.compile(input_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, true)
 
       json2 = Path.join(@output_path, "cache_manifest.json") |> json_read!()
       assert Enum.count(json2["digests"]) == 2
@@ -191,8 +191,8 @@ defmodule Phoenix.DigesterTest do
       File.mkdir_p!(input_path)
       File.write!(input_file, "console.log('test');")
 
-      assert :ok = Phoenix.Digester.compile(input_path, input_path)
-      assert :ok = Phoenix.Digester.compile(input_path, input_path)
+      assert :ok = Phoenix.Digester.compile(input_path, input_path, true)
+      assert :ok = Phoenix.Digester.compile(input_path, input_path, true)
 
       output_files = assets_files(input_path)
       refute "file.js.gz.gz" in output_files
@@ -200,9 +200,9 @@ defmodule Phoenix.DigesterTest do
       refute Enum.any?(output_files, &(&1 =~ ~r/file-#{@hash_regex}.[\w|\d]*.[-#{@hash_regex}/))
     end
 
-    test "digests only absolute and relative asset paths found within stylesheets" do
+    test "digests only absolute and relative asset paths found within stylesheets with vsn" do
       input_path = "test/fixtures/digest/priv/static/"
-      assert :ok = Phoenix.Digester.compile(input_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, true)
 
       digested_css_filename =
         assets_files(@output_path)
@@ -221,9 +221,30 @@ defmodule Phoenix.DigesterTest do
       assert digested_css =~ ~r"http://www.phoenixframework.org/absolute.png"
     end
 
+    test "digests only absolute and relative asset paths found within stylesheets without vsn" do
+      input_path = "test/fixtures/digest/priv/static/"
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, false)
+
+      digested_css_filename =
+        assets_files(@output_path)
+        |> Enum.find(&(&1 =~ ~r"app-#{@hash_regex}.css"))
+
+      digested_css =
+        Path.join(@output_path, digested_css_filename)
+        |> File.read!()
+
+      refute digested_css =~ ~r"/phoenix\.png"
+      refute digested_css =~ ~r"\.\./images/relative\.png"
+      assert digested_css =~ ~r"/phoenix-#{@hash_regex}\.png"
+      assert digested_css =~ ~r"\.\./images/relative-#{@hash_regex}\.png"
+
+      refute digested_css =~ ~r"http://www.phoenixframework.org/absolute-#{@hash_regex}.png"
+      assert digested_css =~ ~r"http://www.phoenixframework.org/absolute.png"
+    end
+
     test "sha512 matches content of digested file" do
       input_path = "test/fixtures/digest/priv/static/"
-      assert :ok = Phoenix.Digester.compile(input_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, true)
 
       digested_css_filename =
         assets_files(@output_path)
@@ -245,7 +266,7 @@ defmodule Phoenix.DigesterTest do
 
     test "digests sourceMappingURL asset paths found within javascript source files" do
       input_path = "test/fixtures/digest/priv/static/"
-      assert :ok = Phoenix.Digester.compile(input_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, true)
 
       digested_js_map_filename =
         assets_files(@output_path)
@@ -266,7 +287,7 @@ defmodule Phoenix.DigesterTest do
 
     test "digests file url paths found within javascript mapping files" do
       input_path = "test/fixtures/digest/priv/static/"
-      assert :ok = Phoenix.Digester.compile(input_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, true)
 
       digested_js_map_filename =
         assets_files(@output_path)
@@ -286,7 +307,7 @@ defmodule Phoenix.DigesterTest do
 
     test "does not digest assets within undigested files" do
       input_path = "test/fixtures/digest/priv/static/"
-      assert :ok = Phoenix.Digester.compile(input_path, @output_path)
+      assert :ok = Phoenix.Digester.compile(input_path, @output_path, true)
 
       undigested_css =
         Path.join(@output_path, "css/app.css")
diff --git a/test/phoenix/endpoint/endpoint_test.exs b/test/phoenix/endpoint/endpoint_test.exs
index b84477dd8..2df5e6a08 100644
--- a/test/phoenix/endpoint/endpoint_test.exs
+++ b/test/phoenix/endpoint/endpoint_test.exs
@@ -8,6 +8,7 @@ defmodule Phoenix.Endpoint.EndpointTest do
            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]
 
@@ -96,6 +97,7 @@ defmodule Phoenix.Endpoint.EndpointTest do
   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"}
 
@@ -109,6 +111,16 @@ defmodule Phoenix.Endpoint.EndpointTest do
     assert Endpoint.static_path("/foo.css") == "/foo-ghijkl.css?vsn=d"
   end
 
+  test "uses correct path accordingly to vsn setting" do
+    config = put_in(@config[:cache_manifest_skip_vsn], false)
+    assert Endpoint.config_change([{Endpoint, config}], []) == :ok
+    assert Endpoint.static_path("/foo.css") == "/foo-d978852bea6530fcd197b5445ed008fd.css?vsn=d"
+
+    config = put_in(@config[:cache_manifest_skip_vsn], true)
+    assert Endpoint.config_change([{Endpoint, config}], []) == :ok
+    assert Endpoint.static_path("/foo.css") == "/foo-d978852bea6530fcd197b5445ed008fd.css"
+  end
+
   @tag :capture_log
   test "invokes init/2 callback" do
     defmodule InitEndpoint do