Add deprecation warning for pipe_through called after routes #6297

chrismccord · 2025-06-12T01:22:56Z

Summary

Addresses issue #6139 by adding a deprecation warning when pipe_through is called after routes are defined in a router scope.

Problem

Previously, developers could call pipe_through after defining routes, which led to confusing behavior where only routes defined after the pipe_through call would have the pipelines applied. This was a common source of confusion.

Solution

Backwards Compatible: Added deprecation warning instead of breaking change
Clear Guidance: Comprehensive warning message with before/after examples
Robust Implementation: Tracks route definitions per scope and warns appropriately

Changes

Core Implementation

lib/phoenix/router/scope.ex: Added has_routes? field to track when routes are defined and issue warnings in pipe_through/2

Test Coverage

test/phoenix/router/pipe_through_deprecation_test.exs: 7 comprehensive test cases covering all warning scenarios

Example Warning Output

[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 [:browser] 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

Testing

✅ All new tests pass (7/7)
✅ All existing router tests pass
✅ No breaking changes - existing code continues to work
✅ Warning is appropriately triggered in existing test suite

This provides a smooth migration path for users while preventing the common pitfall that confused developers.

When generating a presence tracker in an umbrella app, the generator was incorrectly using the web module name (e.g., MyAppWeb) instead of the context app module name (e.g., MyApp) for the PubSub server. This resulted in incorrect PubSub server references like: pubsub_server: MyAppWeb.PubSub Instead of the correct: pubsub_server: MyApp.PubSub Fixed by using Mix.Phoenix.context_base/1 instead of inflections[:base] to correctly identify the context app module name in umbrella apps. Fixes #6287

- Add has_routes? field to Phoenix.Router.Scope to track when routes are defined - Issue Logger.warning when pipe_through is called after routes in a scope - Add comprehensive test coverage for deprecation warning behavior - Maintains backwards compatibility while guiding users to best practices - Addresses issue #6139

SteffenDE

Note that @josevalim added a route in tests that now has that warning as well, that's why the current tests are flaky.

SteffenDE · 2025-06-13T12:16:27Z

lib/phoenix/router/scope.ex

+    # Mark that we have routes defined in this scope
+    top = get_top(module)
+    put_top(module, %{top | has_routes?: true})
+


I assume we can skip the second get_top call in line 41?

Suggested change

# Mark that we have routes defined in this scope

top = get_top(module)

put_top(module, %{top | has_routes?: true})

# Get the top scope and mark that we have routes defined

top = %{get_top(module) | has_routes?: true}

put_top(module, top)

SteffenDE · 2025-06-13T12:17:12Z

pipe_through_analysis.md

+# 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


Let's remove that file?

SteffenDE · 2025-06-13T12:18:06Z

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__)


Those changes are unrelated, can we undo them? Same with phx.gen.presence itself.

We should mix format the whole repository at some point.

SteffenDE · 2025-06-13T12:19:59Z

lib/phoenix/router/scope.ex

+      require Logger
+
+      Logger.warning("""


Aren't deprecation warnings usually done with IO.warn?

SteffenDE · 2025-06-13T12:25:51Z

test/phoenix/router/pipe_through_deprecation_test.exs

+
+      conn = call(Phoenix.Router.PipeThroughDeprecationTest.TestRouter7, :get, "/second")
+      assert conn.status == 200
+      assert conn.resp_body == "users show"


I assume it should actually check that it is called?

Suggested change

assert conn.resp_body == "users show"

assert conn.resp_body == "users show"

assert conn.assigns.browser_called

josevalim · 2025-06-13T13:42:05Z

test/phoenix/router/pipe_through_deprecation_test.exs

@@ -0,0 +1,191 @@
+defmodule Phoenix.Router.PipeThroughDeprecationTest do


We should not have a new test file only for this and the tests can definitely be shorter. We only need to check for the warning, no need to for the other success cases.

Fly Dev added 10 commits June 12, 2025 00:15

document phoenix presence generator issue analysis

490eb87

uncomment and fix umbrella test for PubSub server name

7c8792b

fix expected file path for umbrella test

dde338d

fix umbrella test file path expectation

5b5848d

fix all test file paths to match expected generator behavior

dc48afd

fix umbrella test otp_app expectation

0313434

Combine tests

acad965

analyze pipe_through ordering issue

efc5889

SteffenDE reviewed Jun 13, 2025

View reviewed changes

josevalim reviewed Jun 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add deprecation warning for pipe_through called after routes #6297

Add deprecation warning for pipe_through called after routes #6297

Uh oh!

chrismccord commented Jun 12, 2025

Uh oh!

SteffenDE left a comment

Uh oh!

SteffenDE Jun 13, 2025

Uh oh!

SteffenDE Jun 13, 2025

Uh oh!

SteffenDE Jun 13, 2025

Uh oh!

SteffenDE Jun 13, 2025

Uh oh!

SteffenDE Jun 13, 2025

Uh oh!

josevalim Jun 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -1,4 +1,4 @@
		Code.require_file "../../../installer/test/mix_helper.exs", __DIR__
		Code.require_file("../../../installer/test/mix_helper.exs", __DIR__)

	assert conn.resp_body == "users show"
	assert conn.resp_body == "users show"
	assert conn.assigns.browser_called

		@@ -0,0 +1,191 @@
		defmodule Phoenix.Router.PipeThroughDeprecationTest do

Uh oh!

Add deprecation warning for pipe_through called after routes #6297

Are you sure you want to change the base?

Add deprecation warning for pipe_through called after routes #6297

Uh oh!

Conversation

chrismccord commented Jun 12, 2025

Summary

Problem

Solution

Changes

Core Implementation

Test Coverage

Example Warning Output

Testing

Uh oh!

SteffenDE left a comment

Choose a reason for hiding this comment

Uh oh!

SteffenDE Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

SteffenDE Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

SteffenDE Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

SteffenDE Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

SteffenDE Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

josevalim Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants