Documentation of a class should be defined above the class definition,
not on the parent namespace.
This fixes the documentation of ActionView::Template::Renderable showing
up in ActionView::Template.
Most of the others are marked with `:nodoc:` anyway, so it shouldn't affect
their documentation.
Currently when opening the main framework pages there is no introduction
to the framework. Instead we only see a whole lot of modules and the
`gem_version` and `version` methods.
By including the READMEs using the `:include:` directive each frameworks
has a nice introduction.
For markdown READMEs we need to add the :markup: directive.
[ci-skip]
Co-authored-by: zzak <zzakscott@gmail.com>
In asset heavy views, the asset_path helper might be called a lot of
times. When checking the first character of the source element with []
we allocate a string each time just to check it against ?/.
By using String#start_with? we can avoid this step and go a bit faster.
This is the code I used to measure the change:
```
require "bundler/inline"
ROOT_STRING = '/'
TEST_PATH = "/some/path"
gemfile(true) do
source "https://rubygems.org"
gem "benchmark-ips"
end
Benchmark.ips do |x|
x.report("source[0]") do
TEST_PATH[0] != ROOT_STRING
end
x.report("source.start_with?") do
TEST_PATH.start_with?(ROOT_STRING)
end
x.compare!
end
```
Warming up --------------------------------------
source[0] 905.322k i/100ms
source.start_with? 1.541M i/100ms
Calculating -------------------------------------
source[0] 9.012M (± 0.7%) i/s - 45.266M in 5.022969s
source.start_with? 15.395M (± 0.4%) i/s - 77.030M in 5.003691s
Comparison:
source.start_with?: 15394807.0 i/s
source[0]: 9012304.9 i/s - 1.71x slower
This is a refactor of the `Registry` module added in https://github.com/rails/rails/pull/47347. This is an attempt to
minimize the namespace conflcits that will happen when users will have a top level `Registry` module which can cause
incorrect behavior
Replace ActionView::ViewPaths::Registry with ActionView::PathRegistry
* Remove Copyright years
* Basecamp is now 37signals... again
Co-authored-by: David Heinemeier Hansson <dhh@hey.com>
---------
Co-authored-by: David Heinemeier Hansson <dhh@hey.com>
When prepending or appending view paths, Rails will create an ActionView::Resolver
for each String or Pathname argument. Previously, these were not cached. If view
paths were added in a request context (which is a common thing in a multi-tenant
application), new resolvers were created on each request. This effectively
disabled template caching for the added view paths.
Further, under certain circumstances, it also created a memory leak.
This commit fixes both issues by turning dynamic view path strings into cached
resolvers.
Co-authored-by: Dominik Schöler <dominik.schoeler@makandra.de>
Co-authored-by: John Hawthorn <john@hawthorn.email>
Prior to this commit, chaining more than one `token_list` calls with a
[data-action][] attribute value would result in one too many HTML
escapes. Additional subsequent calls would compound the problem.
For example, the following calls would result in an invalid descriptor
that's escaped too many times to be parsed.
```ruby
first = "click->controller#action1"
second = "click->controller#action2"
third = "click->controller#action3"
fourth = "click->controller#action4"
value = token_list(first, token_list(second, token_list(third)))
CGI.unescape_html value.to_s
# => "click->controller#action1 click->controller#action2 click-&gt;controller#action3 click-&amp;gt;controller#action4"
```
By [CGI.unescape_html][] each `String` value before passing it to
[token_list][] (which re-escapes the value), we can preserve a lossless
concatenation process while also preserving the HTML safety.
After this commit, the previous example works as expected:
```ruby
first = "click->controller#action1"
second = "click->controller#action2"
third = "click->controller#action3"
fourth = "click->controller#action4"
value = token_list(first, token_list(second, token_list(third)))
CGI.unescape_html value.to_s
# => "click->controller#action1 click->controller#action2 click->controller#action3 click->controller#action4"
```
[unescaping]: https://ruby-doc.org/stdlib-2.5.3/libdoc/cgi/rdoc/CGI/Util.html#method-i-unescape_html
[token_list]:
https://edgeapi.rubyonrails.org/classes/ActionView/Helpers/TagHelper.html#method-i-token_list
[data-action]: https://stimulus.hotwired.dev/reference/actions
A recent change designed to avoid swapping references to the view context's
output buffer (https://github.com/rails/rails/pull/45731) added a #capture
method to the OutputBuffer class. The original CaptureHelper#capture method
however still swaps the buffer via CaptureHelper#with_output_buffer, meaning
the @output_buffer reference is still reassigned by form helpers, etc. This
change delegates capture behavior to @output_buffer so a single buffer is
used per request.
This PR removes the deprecation added in #47005 because it is
non-trivial for HAML and Slim to implement error highlighting for
templates. If a template engine doesn't support `translate_location`
then we can fallback to `spot` and the error will be displayed but we
won't be able to automatically underline the column.
Ruby 3.2 significantly changed how instance variables are store.
It now use shapes, and in short, it's important for performance
to define instance variables in a consistent order to limit the
amount of shapes.
Otherwise, the number of shapes will increase past a point where
MRI won't be able to cache instance variable access. The impact
is even more important when YJIT is enabled.
This PR is data driven. I dump the list of Shapes from Shopify's
monolith production environment, and Rails is very present among
the top offenders:
```
Shape Edges Report
-----------------------------------
770 @default_graphql_name
697 @own_fields
661 @to_non_null_type
555 @own_interface_type_memberships
472 @description
389 @errors
348 @oseid
316 @_view_runtime
310 @_db_runtime
292 @visibility
286 @shop
271 @attribute_method_patterns_cache
264 @namespace_for_serializer
254 @locking_column
254 @primary_key
253 @validation_context
244 @quoted_primary_key
238 @access_controls
234 @_trigger_destroy_callback
226 @_trigger_update_callback
224 @finder_needs_type_condition
215 @_committed_already_called
214 @api_type
203 @mutations_before_last_save
202 @access_controls_overrides
201 @options
198 @mutations_from_database
190 @_already_called
183 @name
179 @_request
176 @own_arguments
175 @_assigns
175 @virtual_path
174 @context
173 @_controller
173 @output_buffer
173 @view_flow
172 @_default_form_builder
169 @cache
159 @_touch_record
151 @attribute_names
151 @default_attributes
150 @columns_hash
149 @attribute_types
148 @columns
147 @marked_for_same_origin_verification
146 @schema_loaded
143 @_config
143 @type
141 @column_names
```
All the changes are of similar nature, the goal is to preset the instance
variable to nil when objects are allocated, or when classes are created.
For classes I leverage the `inherited` hook. If the patern becomes common enough
it might make sense to add a helper for this in `ActiveSupport::Concern`.
If other libraries like `haml` don't implement `translate_location` then
they won't get the error highlighting behavior that was implemented to
underline errors coming from templates.
This deprecation is to notify templating engines that this change is
necessary for them to get this highlighting.
We want to use error highlight with eval'd code, specifically ERB
templates.
Previously we could only get the information we needed by setting
`keep_script_lines` to true. In Ruby 3.2 and error_highlight we added
the ability to get this information without setting `keep_script_lines`.
This change implements that new behavior for Rails.
I removed the script line changes to support this in 3.1 because it is
not in any released version.
Ruby change: https://github.com/ruby/ruby/pull/6593
Erorr highlight change: https://github.com/ruby/error_highlight/pull/26
Co-authored-by: Aaron Patterson <tenderlove@ruby-lang.org>
Ref: ad39d6b
Ensure that all evals include file and line number to identify their
source.
Two of the evals reported by this cop were unneccesary and replaced with
non-eval alternatives: xml is set as a local variable in
Template::Handlers::Builder#call, and instance_eval isn't needed to get
the current binding.
There are additionally 27 offenses in test directories, but since it
seems less important to fix those they are currently ignored.
This is a continuation of #46706 to make sure we don't need to set an
instance variable to `@original_source` for the `compile` method to use.
We can't call `strict_locals!` after encode so we need to set it to a
local variable in `complile`. We changed the `strict_locals!` method to
check `NONE` instead of lazily defining instance variables which let us
simplify `strict_locals?` to return the value of `strict_locals!`. This
simplifies and clarifies the code.
Co-authored-by: Aaron Patterson tenderlove@ruby-lang.org
I do this a lot:
```erb
<%= select :post, :author, authors, required: true %>
```
It doesn't work; the `required` attribute is ignored! Instead, you need to do this:
```erb
<%= select :post, :author, authors, {}, required: true %>
```
It's hard to remember the right API, and it looks to me like a code smell. It looks even smellier when you end up with this:
```erb
<%= select :post, :author, authors, { include_blank: "Choose an option" }, { required: true } %>
```
Where this would be nicer, but again, the `required` attribute is ignored:
```erb
<%= select :post, :author, authors, include_blank: "Choose an option", required: true %>
```
This PR implements a special handling for `required`, `multiple`, and `size` HTML attributes so that these now do the same thing:
```erb
<%= select :post, :author, authors, include_blank: "Choose an option", required: true %>
<%= select :post, :author, authors, { include_blank: "Choose an option" }, { required: true } %>
```
ps. as proof I'm not the only person who makes this mistake, one of the tests in the Rails test suite was wrong! The test added in https://github.com/rails/rails/pull/40522 puts the `multiple` attribute in the wrong place and has the wrong assertion as as result. This PR includes a fix for the test.
Moves the part of `compile!` that compiles the template source into it's
own method. We need this for future work in improving exceptions for ERB
templates to pass to ErrorHighlight.
Co-authored-by: Aaron Patterson <tenderlove@ruby-lang.org>
This change makes date/time options (value, min, max) in `time_field`, `date_field`, `datetime_field`, `week_field`, `month_field` form helpers behave in a unified way.
Since engine initializers run later in the process, we need to run this
initializer earlier than the default.
This ensures they're all registered before the environments are loaded.
`deprecate_constant` will warn whenever the constant is referenced
instead of when the constant is a method receiver, which increases the
likelihood that the warning will be seen. Additionally,
`DeprecatedConstantProxy` prevents using the constant as a superclass,
such as in `class MyClass < SomeDeprecatedConstant`.
This commit adds `ActionView.deprecator` and replaces all usages of
`ActiveSupport::Deprecation.warn` in `actionview/lib` with
`ActionView.deprecator`. This commit also replaces a call to Ruby's
`Module#deprecate_constant` with Rails' `DeprecatedConstantProxy`, so
that its deprecation behavior can be configured using
`ActionView.deprecator`.
Additionally, this commit adds `ActionView.deprecator` to
`Rails.application.deprecators` so that it can be configured via
settings such as `config.active_support.report_deprecations`.
This commit also removes a few defunct `assert_deprecated` calls that
were not failing because they were nested in `assert_raises`, and the
raised error prevented checking the deprecation. (One was mistakenly
kept in d52d773946 when converting
`test_render_file_with_errors` to `test_render_template_with_errors`;
the other two were added in dd9991bac5 but
not removed when the deprecation was completed in
85ecf6e4098601222b604f7c1cbdcb4e49a6d1f0.)
Currently if you do this:
```ruby
check_box_tag "admin", "1", checked: false
```
It is treated [as truthy](19f9922523/actionview/lib/action_view/helpers/form_tag_helper.rb (L444)), and your checkbox is checked. This can be a bit surprising, particularly because the `FormHelper` version [does support](19f9922523/actionview/lib/action_view/helpers/form_helper.rb (L1285)) a keyword argument.
```ruby
f.check_box "admin", checked: false
```
So this PR updates `check_box_tag` and `radio_button_tag` to support `checked` as a positional or keyword argument, this way you can use the same API in both cases.
Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
jbuilder is doing some weird things such as instantiatign RenderedTemplate
with a `Hash` instance as a `body`.
So we can't forcibly cast to string in these places.
We recently let a few very easy to avoid warnings get merged.
The root cause is that locally the test suite doesn't run in
verbose mode unless you explictly pass `-w`.
On CI warnings are enabled, but there is no reason to look at the
build output unless something is failing. And even if one wanted
to do that, that would be particularly work intensive since warnings
may be specific to a Ruby version etc.
Because of this I believe we should:
- Always run the test suite with warnings enabled.
- Raise an error if a warning is unexpected.
We've been using this pattern for a long time at Shopify both in private
and public repositories.
We have access to the path from the backtrace location object. If we
use the path of the ERB as the key, then anytime the ERB changes it'll
just overwrite that template instance in the error handling hash
This commit maps the column information returned from ErrorHighlight in
to column information within the source ERB template. ErrorHighlight
only understands the compiled Ruby code, so this commit adds a small
translation layer that converts the values from ErrorHighlight in to the
right values for the ERB source template
We should get out of the business of parsing backtraces and only use
backtrace locations. Backtrace locations have the file and line number
information baked in, so we don't need to parse things anymore
This commit adds a SyntaxErrorProxy object to active support and wraps
syntax error exceptions with that proxy object. We want to enhance
syntax errors with information about the source location where they
actually happened (normally the backtrace doesn't contain such info).
Rather than mutating the original exception's backtrace, this wraps it
with a proxy object.
Eventually we will implement backtrace_locations on the proxy object so
that the exception handling middleware can be updated to _only_ deal
with backtrace_locations and never deal with raw `backtrace`
You no longer need to call `new` when passing a class to `dom_id`.
This makes `dom_id` behave like `dom_class` in this regard.
Apart from saving a few keystrokes, it prevents Ruby from needing to
instantiate a whole new object just to generate a string.
Before:
```ruby
dom_id(Post) # NoMethodError: undefined method `to_key' for Post:Class
```
After:
```ruby
dom_id(Post) # "new_post"
```
You can still call `dom_id(Post.new)`.
Previously we would mutate the PathSet in order to implement
{append,prepend}_view_path.
This removes the mutation methods we had and instead constructs a new
PathSet whenever we are modifying it. This should allow flexibility in
the classes holding a view_path to know that their view paths isn't
modified (for example, to allow caching).
When word_wrap's break_sequence contains non-rstrippable
characters, word_wrap fails to strip the extra break_sequence at the
end properly. Using chomp! helps cut exactly what was specified in the
argument.
This will either be nil or a non-blank string. The case that it is the
empty string is handled (and replaced) at compile. I'd also argue that
it's possible that the empty string could be a valid value that's
considered to be a strict local, we just happen not to use that value
(preferring the "nokey" syntax).
This commit allows template lookups and render calls which find a
"strict local" template to reuse that template even when provided with
different locals.
This also makes Template#locals return nil for strict local templates,
to avoid leaking the details of the first lookup. Almost nothing calls
this method, so this should not be a significant change.
Previously ripper tracker would misidentify the method name when called with a constant and assoc_hash.
This correctly finds the method name in those cases
The various LogSubscriber subclasses tend to subscribe to events
but then end up doing nothing if the log level is high enough.
But even if we end up not logging, we have to go through the
entire notification path, record timing etc.
By allowing subscribers to dynamically bail out early, we can
save a lot of work if all subscribers are silenced.
With the recent changes to OutputBuffer, calling `to_s` is extremely
expensive if the buffer later is concatenated to (if the buffer never
changes it should be relatively inexpensive, as Ruby will share memory).
Followup on https://github.com/rails/rails/pull/45745
`@rendered` is a much better variable for this kind of intermediate
result as that's what is used by DOM assertions.
Right now many helpers have to deal with two modes of operation to
capture view output.
The main one is to swap the `@output_buffer` variable with a new buffer.
But since some view implementations such as `builder` keep a reference
on the buffer they were initialized with, this doesn't always work.
So additionally, the various capturing helpers also record the buffer
length prior to executing the block, and then `slice!` the buffer back
to its original size.
This is wasteful and make the code rather unclear.
Now that `OutputBuffer` is a delegator, I'd like to refactor all this
so that:
- @output_buffer is no longer re-assigned
- A single OutputBuffer instance is used for the entire response rendering
- Instead capturing is done through `OutputBuffer#capture`
Once the above is achieved, it should allow us to enabled Erubi's
`:chain_appends` option and get some reduced template size and some
performance.
Not re-assigning `@output_buffer` will also allow template to access
the local variable instead of an instance variable, which is cheaper.
But more importantly, that should make the code easier to understand
and easier to be compatible with `StreamingBuffer`.
Ref: https://github.com/jeremyevans/erubi/pull/33
If the template is compiled with `frozen_string_literals: true`,
then explicitly freezing string is slightly wasteful as it will be
compiled as `opt_str_freeze` instead of a simple `putobject`.
The former has to check wether `String#freeze` was redefined every
time, which while fast is useless extra work.
The i18n gem (https://github.com/ruby-i18n/i18n) has been modified to `freeze`
locale setting values. This could cause the Date helper to not work correctly
under certain conditions. `dup` the configuration values fixes that problem.
Dynamic constant lookups (ie. x::SOME_CONST) are uncommon and slightly
slow operations (not _that_ slow). We know that the ::SET constant won't
change (this is a private API and it is called only with our own
classes) so we can switch this to an attr_reader for fast access on the
call to delgate_to.
This also removes the unused `type_klass` attr_accessor.
`ActionView::TestCase` would use an `ActiveSupport::SafeBuffer`
which behaves a bit differently. This never happens in production
so it's better to use a more accurate test.
It's only used by a handful of tests, but having two ways to
evaluate ERB makes it very confusing. The few tests using it can
go through ActionView::Template instead.
MRI has a lot of optimizations for string concatenation that
are only available when concatenating into a `String` instance.
Using a `String` subclass disable these optimizations.
The difference is even more important with YJIT.
So ideally we want the buffer not to be a String subclass.
Luckily, the Action View buffer is for internal use only, so
we can replace inheritance by composition without much work.
Benchmark:
```
ActionView::OutputBuffer: 147644.2 i/s
optimized buffer: 228001.4 i/s - 1.54x (± 0.00) faster
```
Source: https://gist.github.com/casperisfine/7199579a138e268fda71d6a91366af49
NB: That 50% faster figure is to be contextualized, it can radically change
from one template to the other, but is always faster.
It's possible for `ActionView::Helpers::FormTagHelper#field_name` calls
made by instances constructed through `fields` and `fields_for` helpers
to have an `object_name` argument that's `nil`. For example, the
following will raise an `undefined method `empty?' for nil:NilClass`
exception:
```erb
<%= fields do |f| %>
<%= f.field_name :body %>
<% end %>
```
To guard against those calls, replace the method's call to
`String#empty?` with `Object#blank?`, since `NilClass#empty?` is not
defined.
Because these strings contain no HTML elements and the basic entities
are escaped, they are safe to be included as-is as PCDATA in HTML
content. Tagging them as html-safe avoids double-escaping entities
when being concatenated to a SafeBuffer during rendering.
Fixes https://github.com/rails/rails-html-sanitizer/issues/124
Following the discussion here:
https://github.com/rails/rails/pull/44174/files#r785160819
Background: The `i18n` gem is relatively lax when it comes
to naming locales. It does not enforce any standard. Thus
it is possible to have e.g. per tenant locales (think
`en_tenant1`, `en_tenant2` etc.). This also worked for
translated templates up until rails 6.1.
Rails 7 changed the template lookup and enforced a naming
scheme for locales. This poses a problem for legacy apps
that use non-standard locale names.
This commit changes the way locale names are detected in
template file names. In addition to the previously used
regexp it also allows all known locales from
`I18n.available_locales`.
This makes it backwards compatible to rails 7.0
behavior while also allowing non-standard locale names.
Thanks to jvillarejo for the great idea.
Also introduce the usage of `Regexp.union`, a wonderful
suggestion by casperisfine.
Ensure models passed to `form_with` attempt to call `to_model`.
Now that `form_for` is implemented in terms of `form_with`, this commit
also removes the `convert_to_model` call from the `form_for` implementation.
To exercise this behavior, change existing `form_with` test coverage.
Prior to this change, a call to `form_with` made with a `model:` argument
that responds to `to_model` would not incorporate the instance's persistence
state into the form's HTTP verb. After this change, the persistence state
inferred from the `model:` argument's `to_model` call is incorporated into
the `<form>` element's `[method]` attribute.
This is a separate follow-up change proposed in [rails/rails#44328][].
The original change to restore old behavior _deliberately_ excluded
applying the same logic to `form_with`, since it would be a breaking
change from how `form_with` behaved previously.
This commit proposed making that breaking change.
[rails/rails#44328]: https://github.com/rails/rails/pull/44328#discussion_r808475585
https://github.com/rails/rails/pull/31250 optimised rendering a collection with `cached: true`, but also with `cached: proc {}`. The problem is the proc may reference associations that should be preloaded to avoid n+1s in generating the cache key.
For example:
```ruby
@posts = Post.preload(:author)
@view.render partial: "test/partial", collection: @post, cached: proc { |post| [post, post.author] }
```
This will n+1 on `post.author`.
To fix this, this PR will always preload the collection if there's a proc given for `cached`. `cached: true` will still not preload unless it renders, as it did in https://github.com/rails/rails/pull/31250.
Add the method ERB::Util.xml_name_escape to escape dangerous characters
in names of tags and names of attributes, following the specification of
XML.
Use that method in the tag helpers of ActionView::Helpers. Rename the option
:escape_attributes to :escape, to simplify by applying the option to the whole
tag.
Every time I write `config.cache_classes` I have to pause for a moment to make
sure I get it right. It makes you think.
On the other hand, if you read `config.enable_reloading = true`, does the
application reload? You do not need to spend 1 cycle of brain CPU to nod.
Because the indented code block follows an indented list item, RDoc
interprets the examples as a continuation of the list item prose,
instead of code. To distinguish the two, this commit moves the examples
to their own subsection with an intervening subheading.
Additionally, this commit applies a few other formatting tweaks.
Because the indented code block follows an indented list item, RDoc
interprets the examples as a continuation of the list item prose,
instead of code. To distinguish the two, this commit moves the examples
to their own subsection with an intervening subheading.
Additionally, this commit applies a few other formatting tweaks.
Follow up to https://github.com/rails/rails/pull/43112 and https://github.com/rails/rails/pull/44100
- `data-remote` is deprecated on links and buttons. Turbo doesn't need it since that is the default behaviour. You use `data-turbo=false` on elements that opt out of that, but I don't think that's in scope for Rails.
- `data-method` is deprecated on links. Turbo expects [data-turbo-method](https://turbo.hotwired.dev/handbook/drive#performing-visits-with-a-different-method).
Update actionview/lib/action_view/helpers/url_helper.rb
Co-authored-by: Hartley McGuire <skipkayhil@gmail.com>
Update actionview/lib/action_view/helpers/url_helper.rb
Co-authored-by: Hartley McGuire <skipkayhil@gmail.com>
Now it's possible to write
audio_tag(user.audio_file)
video_tag(user.video_file)
Instead of
audio_tag(polymorphic_path(user.audio_file))
video_tag(polymorphic_path(user.video_file))
image_tag already supported that, so this follows the same pattern.
These classes are relatively small, however they include lots of
modules as helpers. And if any of the included module hold constants
including it cause the global constant cache to be invalidated
which is really bad for performance.
So when eager loading is enabled we create all the possible classes
as part of the application boot.
RDoc will automatically format and link API references as long as they
are not already marked up as inline code.
This commit removes markup from various API references so that those
references will link to the relevant API docs.
"Overwrite" means "destructively replace", and is more suitable when,
for example, talking about writing data to a location.
"Override" means "supersede", and is more suitable when, for example,
talking about redifining methods in a subclass.
John Hawthorn