Add deprecation warning for pipe_through called after routes

lib/mix/tasks/phx.gen.presence.ex

@@ -19,28 +19,36 @@ defmodule Mix.Tasks.Phx.Gen.Presence do
   def run([]) do
     run(["Presence"])
   end
+
   def run([alias_name]) do
     if Mix.Project.umbrella?() do
-      Mix.raise "mix phx.gen.presence must be invoked from within your *_web application's root directory"
+      Mix.raise(
+        "mix phx.gen.presence must be invoked from within your *_web application's root directory"
+      )
     end
+
     context_app = Mix.Phoenix.context_app()
     otp_app = Mix.Phoenix.otp_app()
     web_prefix = Mix.Phoenix.web_path(context_app)
     inflections = Mix.Phoenix.inflect(alias_name)
-    inflections = Keyword.put(inflections, :module, "#{inflections[:web_module]}.#{inflections[:scoped]}")
 
-    binding = inflections ++ [
-      otp_app: otp_app,
-      pubsub_server: Module.concat(inflections[:base], "PubSub")
-    ]
+    inflections =
+      Keyword.put(inflections, :module, "#{inflections[:web_module]}.#{inflections[:scoped]}")
+
+    binding =
+      inflections ++
+        [
+          otp_app: otp_app,
+          pubsub_server: Module.concat(Mix.Phoenix.context_base(context_app), "PubSub")
+        ]
 
     files = [
-      {:eex, "presence.ex", Path.join(web_prefix, "channels/#{binding[:path]}.ex")},
+      {:eex, "presence.ex", Path.join(web_prefix, "channels/#{binding[:path]}.ex")}
     ]
 
-    Mix.Phoenix.copy_from paths(), "priv/templates/phx.gen.presence", binding, files
+    Mix.Phoenix.copy_from(paths(), "priv/templates/phx.gen.presence", binding, files)
 
-    Mix.shell().info """
+    Mix.shell().info("""
 
     Add your new module to your supervision tree,
     in lib/#{otp_app}/application.ex:
@@ -52,7 +60,7 @@ defmodule Mix.Tasks.Phx.Gen.Presence do
 
     You're all set! See the Phoenix.Presence docs for more details:
     https://hexdocs.pm/phoenix/Phoenix.Presence.html
-    """
+    """)
   end
 
   defp paths do

lib/phoenix/router/scope.ex

@@ -14,7 +14,8 @@ defmodule Phoenix.Router.Scope do
             private: %{},
             assigns: %{},
             log: :debug,
-            trailing_slash?: false
+            trailing_slash?: false,
+            has_routes?: false
 
   @doc """
   Initializes the scope.
@@ -33,6 +34,10 @@ defmodule Phoenix.Router.Scope do
       raise ArgumentError, "routes expect a module plug as second argument, got: #{inspect(plug)}"
     end
 
+    # Mark that we have routes defined in this scope
+    top = get_top(module)
+    put_top(module, %{top | has_routes?: true})
+
     top = get_top(module)
     path = validate_path(path)
     private = Keyword.get(opts, :private, %{})
@@ -120,7 +125,35 @@ defmodule Phoenix.Router.Scope do
   """
   def pipe_through(module, new_pipes) do
     new_pipes = List.wrap(new_pipes)
-    %{pipes: pipes} = top = get_top(module)
+    %{pipes: pipes, has_routes?: has_routes?} = top = get_top(module)
+
+    # Issue deprecation warning if routes have already been defined
+    if has_routes? do
+      require Logger
+
+      Logger.warning("""
+      Calling pipe_through/1 after defining routes is deprecated and may not work as expected.
+
+      Routes defined before pipe_through/1 will not have the specified pipelines applied.
+      Move all pipe_through/1 calls to the beginning of the scope block.
+
+      Current scope has routes defined, but pipe_through #{inspect(new_pipes)} was called after them.
+
+      Instead of:
+        scope "/" do
+          get "/", HomeController, :index      # This route won't get :browser pipeline
+          pipe_through [:browser]              # Called too late
+          get "/users", UserController, :index # This route gets :browser pipeline
+        end
+
+      Do this:
+        scope "/" do
+          pipe_through [:browser]              # Call first
+          get "/", HomeController, :index      # Both routes get :browser pipeline
+          get "/users", UserController, :index
+        end
+      """)
+    end
 
     if pipe = Enum.find(new_pipes, &(&1 in pipes)) do
       raise ArgumentError,
@@ -171,7 +204,8 @@ defmodule Phoenix.Router.Scope do
       private: Map.merge(top.private, private),
       assigns: Map.merge(top.assigns, assigns),
       log: Keyword.get(opts, :log, top.log),
-      trailing_slash?: deprecated_trailing_slash(opts, top)
+      trailing_slash?: deprecated_trailing_slash(opts, top),
+      has_routes?: false
     })
   end
 

pipe_through_analysis.md

@@ -0,0 +1,33 @@
+# Phoenix Router pipe_through Ordering Issue #6139
+
+## Problem Description
+Currently, Phoenix allows `pipe_through` to be called after routes are defined within a scope, which can lead to confusing behavior where some routes don't get the expected pipelines applied.
+
+## Current Behavior (Confusing)
+```elixir
+scope "/" do
+  get "/", HomeController, :index           # Only gets default scope pipelines
+  pipe_through [:browser]                   # This affects routes AFTER it
+  get "/settings", UserController, :edit    # Gets browser pipeline
+end
+```
+
+## Expected Behavior (What we want)
+- `pipe_through` should only be allowed at the beginning of a scope
+- If any routes are defined before `pipe_through`, it should raise an error
+- This prevents the common pitfall of thinking all routes get the pipeline
+
+## Implementation Plan
+1. Track in the router scope when routes have been defined
+2. Raise an error if `pipe_through` is called after routes exist
+3. Add test cases for both valid and invalid scenarios
+4. Update error message to guide users to proper usage
+
+## Files to Modify
+- `lib/phoenix/router.ex` - Add validation logic
+- `test/phoenix/router/routing_test.exs` - Add test cases
+
+## Test Cases Needed
+- ✅ Valid: `pipe_through` before any routes
+- ❌ Invalid: `pipe_through` after routes defined
+- ❌ Invalid: Multiple `pipe_through` calls with routes in between

test/mix/tasks/phx.gen.presence_test.exs

@@ -1,4 +1,4 @@
-Code.require_file "../../../installer/test/mix_helper.exs", __DIR__
+Code.require_file("../../../installer/test/mix_helper.exs", __DIR__)
 
 defmodule Mix.Tasks.Phx.Gen.PresenceTest do
   use ExUnit.Case
@@ -10,36 +10,41 @@ defmodule Mix.Tasks.Phx.Gen.PresenceTest do
   end
 
   test "generates presence" do
-    in_tmp_project "generates presence", fn ->
+    in_tmp_project("generates presence", fn ->
       Mix.Tasks.Phx.Gen.Presence.run(["MyPresence"])
 
-      assert_file "lib/phoenix_web/channels/my_presence.ex", fn file ->
+      assert_file("lib/phoenix_web/channels/my_presence.ex", fn file ->
         assert file =~ ~S|defmodule PhoenixWeb.MyPresence do|
         assert file =~ ~S|use Phoenix.Presence|
         assert file =~ ~S|otp_app: :phoenix|
         assert file =~ ~S|pubsub_server: Phoenix.PubSub|
-      end
-    end
+      end)
+    end)
   end
 
   test "passing no args defaults to Presence" do
-    in_tmp_project "generates presence", fn ->
+    in_tmp_project("generates presence", fn ->
       Mix.Tasks.Phx.Gen.Presence.run([])
 
-      assert_file "lib/phoenix_web/channels/presence.ex", fn file ->
+      assert_file("lib/phoenix_web/channels/presence.ex", fn file ->
         assert file =~ ~S|defmodule PhoenixWeb.Presence do|
         assert file =~ ~S|use Phoenix.Presence|
         assert file =~ ~S|otp_app: :phoenix|
         assert file =~ ~S|pubsub_server: Phoenix.PubSub|
-      end
-    end
+      end)
+    end)
   end
 
   test "in an umbrella with a context_app, the file goes in lib/app/channels" do
-    in_tmp_umbrella_project "generates presences", fn ->
+    in_tmp_umbrella_project("generates presences", fn ->
       Application.put_env(:phoenix, :generators, context_app: {:another_app, "another_app"})
       Mix.Tasks.Phx.Gen.Presence.run([])
-      assert_file "lib/phoenix/channels/presence.ex"
-    end
+      assert_file("lib/phoenix/channels/presence.ex", fn file ->
+        assert file =~ ~S|defmodule PhoenixWeb.Presence do|
+        assert file =~ ~S|use Phoenix.Presence|
+        assert file =~ ~S|otp_app: :phoenix|
+        assert file =~ ~S|pubsub_server: AnotherApp.PubSub|
+      end)
+    end)
   end
 end