Repository: phoenixframework/phoenix (elixir)

Issue:
Raise when Plug.Router.forward/2 API is used instead of Phoenix.Router.
Currently, Plug.Router has a [forward/2](https://hexdocs.pm/plug/Plug.Router.html#forward/2) macro that looks like:

    forward "/foo", to: SomePlug, init_opts: [foo: :bar]

Phoenix also has a `forward` macro, with the following API:

    forward "/foo", SomePlug, foo: :bar

A user could easily be confused (for example. when reading documentation for a library based on Plug) and try to use the Plug arguments in the Phoenix Router.

This currently fails with:
```

== Compilation error in file web/router.ex ==
** (ArgumentError) cannot convert the given list to a string.

To be converted to a string, a list must contain only:

  * strings
  * integers representing Unicode codepoints
  * or a list containing one of these three elements

Please check the given list or call inspect/1 to get the list representation, got:

[to: SomePlug, init_opts: [foo: :bar]]

    (elixir) lib/list.ex:821: List.to_string/1
    (phoenix) lib/phoenix/naming.ex:21: Phoenix.Naming.resource_name/2
    (phoenix) lib/phoenix/router/scope.ex:25: Phoenix.Router.Scope.route/8
    web/router.ex:56: (module)
    (stdlib) erl_eval.erl:677: :erl_eval.do_apply/6
    (elixir) lib/kernel/parallel_compiler.ex:198: anonymous fn/4 in Kernel.ParallelCompiler.spawn_workers/6
```

Instead, we should detect if the forward macro that a user is attempting to use the Plug style arguments and raise with the changes required.

Pull request:
Show a better error when trying use plug forward in router
Previously, passing a non-atom argument to the forward macro:

    forward "/admin", to: AdminController

Would result in the following error message;

    == Compilation error in file web/router.ex ==
    ** (ArgumentError) cannot convert the given list to a string.

    To be converted to a string, a list must contain only:

      * strings
      * integers representing Unicode codepoints
      * or a list containing one of these three elements

    Please check the given list or call inspect/1 to get the list
    representation, got:

    [to: SomePlug, init_opts: [foo: :bar]]

Now the following error message is displayed:

    forward expects a module as the second argument, [to: AdminController]
    given.

This closes #2995

Patch:
diff --git a/assets/js/phoenix.js b/assets/js/phoenix.js
index 4145e8dea..109a8fd0c 100644
--- a/assets/js/phoenix.js
+++ b/assets/js/phoenix.js
@@ -185,7 +185,7 @@
  * @module phoenix
  */
 
-const global = typeof(self) !== "undefined" ? self : window
+const global = typeof self !== "undefined" ? self : window
 const VSN = "2.0.0"
 const SOCKET_STATES = {connecting: 0, open: 1, closing: 2, closed: 3}
 const DEFAULT_TIMEOUT = 10000
@@ -218,7 +218,7 @@ const TRANSPORTS = {
 
 // wraps value in closure or returns closure
 let closure = (value) => {
-  if(typeof(value) === "function"){
+  if(typeof value === "function"){
     return value
   } else {
     let closure = function(){ return value }
diff --git a/assets/package.json b/assets/package.json
index a40f57790..8a310f8e3 100644
--- a/assets/package.json
+++ b/assets/package.json
@@ -23,6 +23,7 @@
     "webpack": "4.0.0",
     "webpack-cli": "^2.0.10"
   },
+  "sideEffects": false,
   "scripts": {
     "test": "mocha ./test/**/*.js --compilers js:babel-register -r jsdom-global/register",
     "docs": "documentation build js/phoenix.js -f html -o ../doc/js",
diff --git a/lib/phoenix/controller.ex b/lib/phoenix/controller.ex
index beba8dce3..db3cb9296 100644
--- a/lib/phoenix/controller.ex
+++ b/lib/phoenix/controller.ex
@@ -559,7 +559,7 @@ defmodule Phoenix.Controller do
   @doc """
   Retrieves the current layout.
   """
-  @spec layout(Plug.Conn.t) :: {atom, String.t} | false
+  @spec layout(Plug.Conn.t) :: {atom, String.t | atom} | false
   def layout(conn), do: conn.private |> Map.get(:phoenix_layout, false)
 
   @doc """
diff --git a/lib/phoenix/endpoint.ex b/lib/phoenix/endpoint.ex
index dc052dcab..9868c3493 100644
--- a/lib/phoenix/endpoint.ex
+++ b/lib/phoenix/endpoint.ex
@@ -871,7 +871,10 @@ defmodule Phoenix.Endpoint do
 
       For example:
 
-        connect_info: [:peer_data, :x_headers, :uri]
+          socket "/socket", AppWeb.UserSocket,
+            websocket: [
+              connect_info: [:peer_data, :x_headers, :uri]
+            ]
 
   ## Websocket configuration
 
diff --git a/lib/phoenix/router.ex b/lib/phoenix/router.ex
index 8df6867a4..af260cd73 100644
--- a/lib/phoenix/router.ex
+++ b/lib/phoenix/router.ex
@@ -775,9 +775,7 @@ defmodule Phoenix.Router do
     router_opts = Keyword.put(router_opts, :as, nil)
 
     quote unquote: true, bind_quoted: [path: path, plug: plug] do
-      plug = Scope.expand_alias(__MODULE__, plug)
-      path_segments = Route.forward_path_segments(path, plug, @phoenix_forwards)
-      @phoenix_forwards Map.put(@phoenix_forwards, plug, path_segments)
+      plug = Scope.register_forwards(__MODULE__, path, plug)
       unquote(add_route(:forward, :*, path, plug, plug_opts, router_opts))
     end
   end
diff --git a/lib/phoenix/router/scope.ex b/lib/phoenix/router/scope.ex
index 47e4fef38..32ca4000f 100644
--- a/lib/phoenix/router/scope.ex
+++ b/lib/phoenix/router/scope.ex
@@ -1,5 +1,5 @@
 defmodule Phoenix.Router.Scope do
-  alias Phoenix.Router.Scope
+  alias Phoenix.Router.{Scope, Route}
   @moduledoc false
 
   @stack :phoenix_router_scopes
@@ -117,9 +117,22 @@ defmodule Phoenix.Router.Scope do
   def inside_scope?(module), do: length(get_stack(module)) > 1
 
   @doc """
-  Expands alias within scoped block.
+  Add a forward to the router.
   """
-  def expand_alias(module, alias) do
+  def register_forwards(module, path, plug) when is_atom(plug) do
+    plug = expand_alias(module, plug)
+    phoenix_forwards = Module.get_attribute(module, :phoenix_forwards)
+    path_segments = Route.forward_path_segments(path, plug, phoenix_forwards)
+    phoenix_forwards = Map.put(phoenix_forwards, plug, path_segments)
+    Module.put_attribute(module, :phoenix_forwards, phoenix_forwards)
+    plug
+  end
+
+  def register_forwards(_, _, plug) do
+    raise ArgumentError, "forward expects a module as the second argument, #{inspect plug} given"
+  end
+
+  defp expand_alias(module, alias) do
     if inside_scope?(module) do
       module
       |> get_stack()
diff --git a/mix.exs b/mix.exs
index ab890b391..e46037ab6 100644
--- a/mix.exs
+++ b/mix.exs
@@ -144,6 +144,7 @@ defmodule Phoenix.MixProject do
     # Phoenix
     # Phoenix.Channel
     # Phoenix.Controller
+    # Phoenix.Endpoint
     # Phoenix.Naming
     # Phoenix.Param
     # Phoenix.Presence
@@ -152,20 +153,19 @@ defmodule Phoenix.MixProject do
     # Phoenix.View
 
     [
-      "Endpoint And Plugs": [
+      "Testing": [
+        Phoenix.ChannelTest,
+        Phoenix.ConnTest,
+      ],
+
+      "Adapters and Plugs": [
         Phoenix.CodeReloader,
-        Phoenix.Endpoint,
         Phoenix.Endpoint.CowboyAdapter,
         Phoenix.Endpoint.Cowboy2Adapter,
         Phoenix.Logger,
       ],
 
-      "Testing": [
-        Phoenix.ChannelTest,
-        Phoenix.ConnTest,
-      ],
-
-      "Socket And Transport": [
+      "Socket and Transport": [
         Phoenix.Socket,
         Phoenix.Socket.Broadcast,
         Phoenix.Socket.Message,


Test patch:
diff --git a/test/phoenix/router/forward_test.exs b/test/phoenix/router/forward_test.exs
index 84869b3c5..58c44de8e 100644
--- a/test/phoenix/router/forward_test.exs
+++ b/test/phoenix/router/forward_test.exs
@@ -142,4 +142,16 @@ defmodule Phoenix.Router.ForwardTest do
     assert conn.resp_body == "health"
     assert conn.private[ApiRouter] == {[], %{Phoenix.Test.HealthController => []}}
   end
+
+  test "forwards raises if using the plug to arguments" do
+    error_message = ~r/expects a module/
+    assert_raise(ArgumentError, error_message, fn ->
+      defmodule BrokenRouter do
+        use Phoenix.Router
+        scope "/" do
+          forward "/health", to: HealthController
+        end
+      end
+    end)
+  end
 end