At present, if you skip a callback that hasn't been defined,
activesupport callbacks silently does nothing. However, it's easy to
mistype the name of a callback and mistakenly think that it's being
skipped, when it is not.
This problem even exists in the current test suite.
CallbacksTest::SkipCallbacksTest#test_skip_person attempts to skip
callbacks that were never set up.
This PR changes `skip_callback` to raise an `ArgumentError` if the
specified callback cannot be found.
As part of #19029, in future `skip_before_action`, `skip_after_action` and
`skip_around_action` will raise an ArgumentError if the specified
callback does not exist. `skip_action_callback` calls all three of these
methods and will almost certainly result in an ArgumentError. If anyone
wants to remove all three callbacks then they can still call the three
individual methods. Therefore let's deprecate `skip_action_callback` now
and remove it when #19029 is merged.
The new test/docs further explain the conflicts that can happen when
mixing `:if`/`:unless` options with `:only`/`:except` options in
`skip_before_action`.
The gist is that "positive" filters always have priority over negative
ones.
The previous commit already showed that `:only` has priority over `:if`.
This commit shows that `:if` has priority over `:except`.
For instance, the following snippets are equivalent:
```ruby
skip_before_action :some_callback, if: -> { condition }, except: action
```
```ruby
skip_before_action :some_callback, if: -> { condition }
```
Test case for using skip_before_filter with the options :only and :if
both present. In this case, the :if option will be ignored and :only
will be executed.
Closes#14549 (the commit was cherry-picked from there).
Introduce explicit way of halting callback chains by throwing :abort. Deprecate current implicit behavior of halting callback chains by returning `false` in apps ported to Rails 5.0. Completely remove that behavior in brand new Rails 5.0 apps.
Conflicts:
railties/CHANGELOG.md
This commit changes arguments and default value of CallbackChain's :terminator
option.
After this commit, Chains of callbacks defined **without** an explicit
`:terminator` option will be halted as soon as a `before_` callback throws
`:abort`.
Chains of callbacks defined **with** a `:terminator` option will maintain their
existing behavior of halting as soon as a `before_` callback matches the
terminator's expectation. For instance, ActiveModel's callbacks will still
halt the chain when a `before_` callback returns `false`.
Email does not support relative links since there is no implicit host. Therefore all links inside of emails must be fully qualified URLs. All path helpers are now deprecated. When removed, the error will give early indication to developers to use `*_url` methods instead.
Currently if a developer uses a `*_path` helper, their tests and `mail_view` will not catch the mistake. The only way to see the error is by sending emails in production. Preventing sending out emails with non-working path's is the desired end goal of this PR.
Currently path helpers are mixed-in to controllers (the ActionMailer::Base acts as a controller). All `*_url` and `*_path` helpers are made available through the same module. This PR separates this behavior into two modules so we can extend the `*_path` methods to add a Deprecation to them. Once deprecated we can use this same area to raise a NoMethodError and add an informative message directing the developer to use `*_url` instead.
The module with warnings is only mixed in when a controller returns false from the newly added `supports_relative_path?`.
Paired @sgrif & @schneems
Because it is more natural way to test substring inclusion. Also, in
this particular case it is much faster.
In general, using `Regexp.new str` for such kind of things is dangerous.
The string must be escaped, unless you know what you're doing. Example:
Regexp.new "\\" # HELLO WINDOWS
# RegexpError: too short escape sequence: /\/
The right way to do this is escape the string
Regexp.new Regexp.escape "\\"
# => /\\/
Here is the benchmark showing how faster `include?` call is.
```
require 'benchmark/ips'
Benchmark.ips do |x|
x.report('include?') { !"index".to_s.include? File::SEPARATOR }
x.report(' !~ ') { "index" !~ Regexp.new(File::SEPARATOR) }
end
__END__
Calculating -------------------------------------
include? 75754 i/100ms
!~ 21089 i/100ms
-------------------------------------------------
include? 3172882.3 (±4.5%) i/s - 15832586 in 5.000659s
!~ 322918.8 (±8.6%) i/s - 1602764 in 4.999509s
```
Extra `.to_s` call is needed to handle the case when `action_name` is
`nil`. If it is omitted, some tests fail.
We are going to deprecate only on Rails 5 to make easier plugin
maintainers support different Rails versions. Right now we are only
discouraging their usage.
This reverts commit 6c5f43bab8206747a8591435b2aa0ff7051ad3de.
Conflicts:
actionpack/CHANGELOG.md
This is a follow up to #15058.
This exception is regularly raised during development. This means it will enter
the user realm. We should provide an API page to show that this exception is public API.
/cc @schneems
This is an option for sending a raw content back to browser. Note that
this rendering option will unset the default content type and does not
include "Content-Type" header back in the response.
You should only use this option if you are expecting the "Content-Type"
header to not be set. More information on "Content-Type" header can be
found on RFC 2616, section 7.2.1.
Please see #12374 for more detail.
TLDR: always return an object that responds to the query methods from
request.format, and do not touch Mime::Type[] lookup to avoid bugs.
---
Long version:
The initial issue was about being able to do checks like
request.format.html? for request with an unknown format, where
request.format would be nil.
This is where the issue came from at first in #7837 and #8085
(merged in cba05887dc3b56a46a9fe2779b6b228880b49622), but the
implementation went down the path of adding this to the mime type
lookup logic.
This unfortunately introduced subtle bugs, for instance in the merged
commit a test related to send_file had to be changed to accomodate the
introduction of the NullType.
Later another bug was found in #13064, related to the content-type being
shown as #<Mime::NullType:...> for templates with localized extensions
but no format included. This one was fixed in #13133, merged in
43962d6ec50f918c9970bd3cd4b6ee5c7f7426ed.
Besides that, custom handlers were not receiving the proper template
formats anymore when passing through the rendering process, because of
the NullType addition. That was found while migrating an application
from 3.2 to 4.0 that uses the Markerb gem (a custom handler that
generates both text and html emails from a markdown template).
---
This changes the implementation moving away from returning this null
object from the mime lookup, and still fixes the initial issue where
request.format.zomg? would raise an exception for unknown formats due to
request.format being nil.
By default, variants in the templates will be picked up if a variant is set
and there's a match. The format will be:
app/views/projects/show.html.erb
app/views/projects/show.html+tablet.erb
app/views/projects/show.html+phone.erb
If request.variant = :tablet is set, we'll automatically be rendering the
html+tablet template.
In the controller, we can also tailer to the variants with this syntax:
class ProjectsController < ActionController::Base
def show
respond_to do |format|
format.html do |html|
@stars = @project.stars
html.tablet { @notifications = @project.notifications }
html.phone { @chat_heads = @project.chat_heads }
end
format.js
format.atom
end
end
end
The variant itself is nil by default, but can be set in before filters, like
so:
class ApplicationController < ActionController::Base
before_action do
if request.user_agent =~ /iPad/
request.variant = :tablet
end
end
end
This is modeled loosely on custom mime types, but it's specifically not
intended to be used together. If you're going to make a custom mime type,
you don't need a variant. Variants are for variations on a single mime
types.
