Repository: phoenixframework/phoenix
Pull request #3371 (2019-04-13 20:53:50)

Refactor flash methods from Phoenix.Controller into Phoenix.Controller.Flash
Based on a conversation here:

https://groups.google.com/forum/#!searchin/phoenix-core/liveview%7Csort:date/phoenix-core/61kVmWkF6QI/70BFY0zFCAAJ

The proposed step forward for getting `--live` (and eventually `--no-live`) integrated into phoenix directly.

```
1. phx.new --live flag (it will be opt-in on v1.5 while we figure things out but probably enabled by default on v1.6)
2. phx.new.live generators
3. Bring the functionality in LiveView.Flash to Phoenix so we can get rid of one plug
```

It was suggested to start with #3

Once released, then I can help remove that code from phoenix_live_view and update the documentation to remove the need for the second flash plug.

@josevalim if I have mis-understood, let me know.



aforward: In the examples,

I kept the code as-is and observed the flash working as expected

<img width="817" alt="Screen Shot 2019-04-13 at 5 47 29 PM" src="https://user-images.githubusercontent.com/48086/56085612-39960280-5e14-11e9-89db-12cbf6fcc897.png">


Then I tried the fetch_flash as-is

```
    plug :fetch_flash
    # plug Phoenix.LiveView.Flash
```

The flash did not work (as expected)

<img width="342" alt="Screen Shot 2019-04-13 at 5 46 36 PM" src="https://user-images.githubusercontent.com/48086/56085602-18cdad00-5e14-11e9-9f60-0099b8151b47.png">

I am now testing this branch to get it working "as-is", which it isn't yet.  I will be updating the PR as I troubleshoot the behaviour for the merged code.


josevalim: Hi @aforward!

What do you think if we break this PR in two? Can you please first send a PR that moves all of the Flash functions to a Flash module?

Then another PR can add the new feature, this would make the code easier to review. :) Thank you!
aforward: There we go... `__phoenix_flash___` != `__phoenix_flash__` 
aforward: Yes @josevalim, will do
aforward: Tested locally against the phoenix live view project

```
$ git diff lib/ mix.exs 
diff --git a/lib/demo_web/router.ex b/lib/demo_web/router.ex
index db8f7db..14e91e3 100644
--- a/lib/demo_web/router.ex
+++ b/lib/demo_web/router.ex
@@ -5,7 +5,8 @@ defmodule DemoWeb.Router do
   pipeline :browser do
     plug :accepts, ["html"]
     plug :fetch_session
-    plug Phoenix.LiveView.Flash
+    plug :fetch_flash
+    plug Phoenix.Controller.Flash
     plug :protect_from_forgery
     plug :put_secure_browser_headers
     plug :put_layout, {DemoWeb.LayoutView, :app}
diff --git a/mix.exs b/mix.exs
index c4d4fe8..8ee6873 100644
--- a/mix.exs
+++ b/mix.exs
@@ -33,7 +33,8 @@ defmodule Demo.Mixfile do
   # Type `mix help deps` for examples and options.
   defp deps do
     [
-      {:phoenix, github: "phoenixframework/phoenix", branch: "v1.4", override: true},
+      # {:phoenix, github: "aforward-oss/phoenix", branch: "f/liveview_flash_plug", override: true},
+      {:phoenix, path: "../../oss/phoenix", override: true},
       {:phoenix_live_view, github: "phoenixframework/phoenix_live_view"},
       {:plug_cowboy, "~> 2.0"},
       {:plug, "~> 1.7"},
```

And the flash messages are persisted as expected

<img width="810" alt="Screen Shot 2019-04-14 at 8 49 42 AM" src="https://user-images.githubusercontent.com/48086/56093083-448f7800-5e92-11e9-8c91-a2801deede8f.png">

If turned off `# plug Phoenix.Controller.Flash` then the flash no longer works (as expected).

@josevalim let me know if this aligns with your thoughts on a first step.
aforward: I have done making changes based on all comments above.  Let me know if there is more work to be done / comments to be made.
josevalim: So I have an idea on how to make the signing stuff work! We would make it work like CSRF tokens.

We will add three new functions:

  * `load_state` - loads the flash state
  * `dump_state` - dumps the flash state
  * `sign_flash` - signs the given flash

All of those work on a signing salt. The signing salt will be automatically generated whenever `dump_state` or `sign_flash` are  called and stored in the process dictionary. Then in `register_before_send` we will store the  value in pdict in the session. If there is already something in the session and it is exactly the same as pdict, then we don't need to do anything.

On every request, we will see if there is said signing salt in the session and, if so, we put it into pdict. If there is a signing salt, we will also check if there is a cookie. If there is, we go ahead, decode it, merge into the flash, and erase the cookie.

But let's implement all of this as a separate issue.

Then we will have to work on the LiveView integration, which can discuss once this done. :)


Patch:
diff --git a/lib/phoenix/controller.ex b/lib/phoenix/controller.ex
index bde5b6c58..7629df639 100644
--- a/lib/phoenix/controller.ex
+++ b/lib/phoenix/controller.ex
@@ -1273,24 +1273,7 @@ defmodule Phoenix.Controller do
   @doc """
   Fetches the flash storage.
   """
-  def fetch_flash(conn, _opts \\ []) do
-    session_flash = get_session(conn, "phoenix_flash")
-    conn = persist_flash(conn, session_flash || %{})
-
-    register_before_send conn, fn conn ->
-      flash = conn.private.phoenix_flash
-      flash_size = map_size(flash)
-
-      cond do
-        is_nil(session_flash) and flash_size == 0 ->
-          conn
-        flash_size > 0 and conn.status in 300..308 ->
-          put_session(conn, "phoenix_flash", flash)
-        true ->
-          delete_session(conn, "phoenix_flash")
-      end
-    end
-  end
+  defdelegate fetch_flash(conn, opts \\ []), to: Phoenix.Controller.Flash
 
   @doc """
   Persists a value in flash.
@@ -1304,9 +1287,7 @@ defmodule Phoenix.Controller do
       "Welcome Back!"
 
   """
-  def put_flash(conn, key, message) do
-    persist_flash(conn, Map.put(get_flash(conn), flash_key(key), message))
-  end
+  defdelegate put_flash(conn, key, message), to: Phoenix.Controller.Flash
 
   @doc """
   Returns a map of previously set flash messages or an empty map.
@@ -1321,10 +1302,7 @@ defmodule Phoenix.Controller do
       %{"info" => "Welcome Back!"}
 
   """
-  def get_flash(conn) do
-    Map.get(conn.private, :phoenix_flash) ||
-      raise ArgumentError, message: "flash not fetched, call fetch_flash/2"
-  end
+  defdelegate get_flash(conn), to: Phoenix.Controller.Flash
 
   @doc """
   Returns a message from flash by `key`.
@@ -1336,9 +1314,7 @@ defmodule Phoenix.Controller do
       "Welcome Back!"
 
   """
-  def get_flash(conn, key) do
-    get_flash(conn)[flash_key(key)]
-  end
+  defdelegate get_flash(conn, key), to: Phoenix.Controller.Flash
 
   @doc """
   Generates a status message from the template name.
@@ -1364,16 +1340,7 @@ defmodule Phoenix.Controller do
   @doc """
   Clears all flash messages.
   """
-  def clear_flash(conn) do
-    persist_flash(conn, %{})
-  end
-
-  defp flash_key(binary) when is_binary(binary), do: binary
-  defp flash_key(atom) when is_atom(atom), do: Atom.to_string(atom)
-
-  defp persist_flash(conn, value) do
-    put_private(conn, :phoenix_flash, value)
-  end
+  defdelegate clear_flash(conn), to: Phoenix.Controller.Flash
 
   @doc """
   Returns the current request path with its default query parameters:
diff --git a/lib/phoenix/controller/flash.ex b/lib/phoenix/controller/flash.ex
new file mode 100644
index 000000000..d7035cf93
--- /dev/null
+++ b/lib/phoenix/controller/flash.ex
@@ -0,0 +1,56 @@
+defmodule Phoenix.Controller.Flash do
+  import Plug.Conn
+
+  @moduledoc false
+
+  @session_key "phoenix_flash"
+  @session_atom :phoenix_flash
+
+  @doc false
+  def fetch_flash(conn, _opts \\ []) do
+    found_flash = get_session(conn, @session_key)
+    conn = persist_flash(conn, found_flash || %{})
+
+    register_before_send conn, fn conn ->
+      flash = Map.fetch!(conn.private, @session_atom)
+      flash_size = map_size(flash)
+
+      cond do
+        is_nil(found_flash) and flash_size == 0 ->
+          conn
+        flash_size > 0 and conn.status in 300..308 ->
+          put_session(conn, @session_key, flash)
+        true ->
+          delete_session(conn, @session_key)
+      end
+    end
+  end
+
+  @doc false
+  def clear_flash(conn) do
+    persist_flash(conn, %{})
+  end
+
+  @doc false
+  def put_flash(conn, key, message) do
+    persist_flash(conn, Map.put(get_flash(conn), flash_key(key), message))
+  end
+
+  @doc false
+  def get_flash(conn) do
+    Map.get(conn.private, @session_atom) ||
+      raise ArgumentError, "flash not fetched, call fetch_flash/2"
+  end
+
+  def get_flash(conn, key) do
+    get_flash(conn)[flash_key(key)]
+  end
+
+  defp persist_flash(conn, value) do
+    put_private(conn, @session_atom, value)
+  end
+
+  defp flash_key(binary) when is_binary(binary), do: binary
+  defp flash_key(atom) when is_atom(atom), do: Atom.to_string(atom)
+
+end


Test patch:
diff --git a/test/phoenix/controller/flash_test.exs b/test/phoenix/controller/flash_test.exs
index 104e306fb..c0a1761ca 100644
--- a/test/phoenix/controller/flash_test.exs
+++ b/test/phoenix/controller/flash_test.exs
@@ -11,30 +11,29 @@ defmodule Phoenix.Controller.FlashTest do
 
   test "flash is persisted when status is a redirect" do
     for status <- 300..308 do
-      conn = conn(:get, "/") |> with_session |> fetch_flash()
+      conn = conn() |> fetch_flash()
                              |> put_flash(:notice, "elixir") |> send_resp(status, "ok")
       assert get_flash(conn, :notice) == "elixir"
       assert get_resp_header(conn, "set-cookie") != []
-      conn = conn(:get, "/") |> recycle_cookies(conn) |> with_session |> fetch_flash()
+      conn = conn(recycle_cookies: conn) |> fetch_flash()
       assert get_flash(conn, :notice) == "elixir"
     end
   end
 
   test "flash is not persisted when status is not redirect" do
     for status <- [299, 309, 200, 404] do
-      conn = conn(:get, "/") |> with_session |> fetch_flash()
+      conn = conn() |> fetch_flash()
                              |> put_flash(:notice, "elixir") |> send_resp(status, "ok")
       assert get_flash(conn, :notice) == "elixir"
       assert get_resp_header(conn, "set-cookie") != []
-      conn = conn(:get, "/") |> recycle_cookies(conn) |> with_session |> fetch_flash()
+      conn = conn(recycle_cookies: conn) |> fetch_flash()
       assert get_flash(conn, :notice) == nil
     end
   end
 
   test "flash does not write to session when it is empty and no session exists" do
     conn =
-      conn(:get, "/")
-      |> with_session()
+      conn()
       |> fetch_flash()
       |> clear_flash()
       |> send_resp(302, "ok")
@@ -44,16 +43,13 @@ defmodule Phoenix.Controller.FlashTest do
 
   test "flash writes to session when it is empty and a previous session exists" do
     persisted_flash_conn =
-      conn(:get, "/")
-      |> with_session()
+      conn()
       |> fetch_flash()
       |> put_flash(:info, "existing")
       |> send_resp(302, "ok")
 
     conn =
-      conn(:get, "/")
-      |> Plug.Test.recycle_cookies(persisted_flash_conn)
-      |> with_session()
+      conn(recycle_cookies: persisted_flash_conn)
       |> fetch_flash()
       |> clear_flash()
       |> send_resp(200, "ok")
@@ -63,37 +59,36 @@ defmodule Phoenix.Controller.FlashTest do
 
   test "get_flash/1 raises ArgumentError when flash not previously fetched" do
     assert_raise ArgumentError, fn ->
-      conn(:get, "/") |> with_session |> get_flash()
+      conn() |> get_flash()
     end
   end
 
   test "get_flash/1 returns the map of messages" do
-    conn = conn(:get, "/") |> with_session |> fetch_flash([]) |> put_flash(:notice, "hi")
+    conn = conn() |> fetch_flash([]) |> put_flash(:notice, "hi")
     assert get_flash(conn) == %{"notice" => "hi"}
   end
 
   test "get_flash/2 returns the message by key" do
-    conn = conn(:get, "/") |> with_session |> fetch_flash([]) |> put_flash(:notice, "hi")
+    conn = conn() |> fetch_flash([]) |> put_flash(:notice, "hi")
     assert get_flash(conn, :notice) == "hi"
     assert get_flash(conn, "notice") == "hi"
   end
 
   test "get_flash/2 returns nil for missing key" do
-    conn = conn(:get, "/") |> with_session |> fetch_flash([])
+    conn = conn() |> fetch_flash([])
     assert get_flash(conn, :notice) == nil
     assert get_flash(conn, "notice") == nil
   end
 
   test "put_flash/3 raises ArgumentError when flash not previously fetched" do
     assert_raise ArgumentError, fn ->
-      conn(:get, "/") |> with_session |> put_flash(:error, "boom!")
+      conn() |> put_flash(:error, "boom!")
     end
   end
 
   test "put_flash/3 adds the key/message pair to the flash" do
     conn =
-      conn(:get, "/")
-      |> with_session
+      conn()
       |> fetch_flash([])
       |> put_flash(:error, "oh noes!")
       |> put_flash(:notice, "false alarm!")
@@ -106,8 +101,7 @@ defmodule Phoenix.Controller.FlashTest do
 
   test "clear_flash/1 clears the flash messages" do
     conn =
-      conn(:get, "/")
-      |> with_session
+      conn()
       |> fetch_flash([])
       |> put_flash(:error, "oh noes!")
       |> put_flash(:notice, "false alarm!")
@@ -122,4 +116,17 @@ defmodule Phoenix.Controller.FlashTest do
       conn(:get, "/") |> fetch_flash([])
     end
   end
+
+  defp conn(opts \\ []) do
+    conn = conn(:get, "/")
+
+    conn = case opts[:recycle_cookies] do
+      nil -> conn
+      old_conn -> conn |> recycle_cookies(old_conn)
+    end
+
+    conn
+    |> with_session()
+  end
+
 end