Skip to content

Commit

Permalink
Raise in LookupContext#formats= on invalid format
Browse files Browse the repository at this point in the history 
This is a developer quality of life improvement, to ensure that unknown
formats aren't assigned (which it would previously accept, but wouldn't
work 100% correctly due to caching).
  • Loading branch information
@jhawthorn
jhawthorn committed Mar 18, 2019
1 parent ffc842a commit 0eb9e1e
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 0 deletions.
5 changes: 5 additions & 0 deletions actionview/lib/action_view/lookup_context.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,11 @@ def formats=(values)
values.concat(default_formats) if values.delete "*/*"
values.uniq!

invalid_types = (values - Template::Types.symbols)
unless invalid_types.empty?

This comment has been minimized.

Sign in to view

Copy link
@Edouard-chin

Edouard-chin Mar 27, 2019

Member

Very much of an edgecase but this now breaks in case you have an action that does this

def my_action
  respond_to do |format|
    format.all { ... }
  end
end

The negotiated format returned will be :all which isn't part of the Template::Types.symbols.

Not sure if we want to fix since there is no point to defined a single all, but it surprised me that this suddenly broke

This comment has been minimized.

Sign in to view

Copy link
@kaspth

kaspth Mar 27, 2019 •
edited

Contributor

I'm guessing it's the same for format.any(:html, :js) { … }.

This comment has been minimized.

Sign in to view

Copy link
@rafaelfranca

rafaelfranca Mar 27, 2019

Member

:all and "*/*" should be the same. We should fix this issue.

This comment has been minimized.

Sign in to view

Copy link
@jhawthorn

jhawthorn Mar 27, 2019

Author Member

@Edouard-chin I can't reproduce this. Rails has tests for format.any (which all is aliased to), and they are all passing.

I wrote another test to use templates and it also seemed to work fine. rails:93dbbe3...jhawthorn:8e6dd4a

If this is an issue could you provide steps to reproduce? 🙇

This comment has been minimized.

Sign in to view

Copy link
@Edouard-chin

Edouard-chin Mar 27, 2019 •
edited

Member

Oops sorry, suyre

diff --git a/actionpack/test/controller/mime/respond_to_test.rb b/actionpack/test/controller/mime/respond_to_test.rb
index a190a7cb3a..05363607dd 100644
--- a/actionpack/test/controller/mime/respond_to_test.rb
+++ b/actionpack/test/controller/mime/respond_to_test.rb
@@ -579,7 +579,9 @@ def test_browser_check_with_any_any
   end

   def test_handle_any_with_template
-    get :handle_any_with_template, format: :js
+    @request.accept = '*/*'
+
+    get :handle_any_with_template
     assert_equal "Hello world!", @response.body
   end

(Based on your commit above)

AFAICT this is the only possibility where the mime mime negotiation will return :all

rails/actionpack/lib/action_dispatch/http/mime_negotiation.rb

Line 153 in 93dbbe3

order.include?(Mime::ALL) ? format : nil

This comment has been minimized.

Sign in to view

Copy link
@jhawthorn

jhawthorn Mar 28, 2019 •
edited

Author Member

🙇 Thank you. This should be fixed by #35775

I'm actually quite happy this changed caught this issue, previously we would have been looking for with .all as the extension, ex. welcome.all.erb, and not actually all extensions!
raise ArgumentError, "Invalid formats: #{invalid_types.map(&:inspect).join(", ")}"
end

if values == [:js]
values << :html
@html_fallback_for_js = true
Expand Down
8 changes: 8 additions & 0 deletions actionview/test/template/lookup_context_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ def teardown
assert_equal [:js, :html], @lookup_context.formats
end

test "raises on invalid format assignment" do
ex = assert_raises ArgumentError do
@lookup_context.formats = [:html, :javascript]
end

assert_equal "Invalid formats: :javascript", ex.message
end

test "provides getters and setters for locale" do
@lookup_context.locale = :pt
assert_equal :pt, @lookup_context.locale
Expand Down

0 comments on commit 0eb9e1e

Please sign in to comment.