Tighten up the AV::Base constructor #35093
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tenderlove
force-pushed
the
av-base-constructor
branch
from
a8583f4
to
dacfb1b
Compare
The AV::Base constructor was too complicated, and this commit tightens up the parameters it will take. At runtime, AV::Base is most commonly constructed here: https://github.com/rails/rails/blob/94d54fa4ab641a0ddeb173409cb41cc5becc02a9/actionview/lib/action_view/rendering.rb#L72-L74 This provides an AV::Renderer instance, a hash of assignments, and a controller instance. Since this is the common case for construction, we should remove logic from the constructor that handles other cases. This commit introduces special constructors for those other cases. Interestingly, most code paths that construct AV::Base "strangely" are tests.
tenderlove
force-pushed
the
av-base-constructor
branch
from
dacfb1b
to
e17fe52
Compare
y-yagi
reviewed
Jan 31, 2019
| @_config = ActiveSupport::InheritableOptions.new | ||
|
|
||
| if context.is_a?(ActionView::Renderer) | ||
| @view_renderer = context | ||
| unless formats == NULL |
There was a problem hiding this comment.
If formats is NULL, should change the value to nil?
If this is left as NULL(Object.new), it will be incorrectly judged where the value is nil or not.
For example, if using web-console, NoMethodError will be raise in lookup context.
DEPRECATION WARNING: ActionView::Base instances should be constructed with a view renderer,
assigments, and a controller.
(called from new at /home/y-yagi/.rbenv/versions/2.6.0/lib/ruby/gems/2.6.0/gems/web-console-3.7.0/lib/web_console/template.rb:21)
NoMethodError: undefined method `delete' for #<Object:0x000055e2e6120880>
from /home/y-yagi/src/rails/master_y_yagi/rails/actionview/lib/action_view/lookup_context.rb:258:in `formats='
from /home/y-yagi/src/rails/master_y_yagi/rails/actionview/lib/action_view/base.rb:206:in `build_renderer'
from /home/y-yagi/src/rails/master_y_yagi/rails/actionview/lib/action_view/base.rb:243:in `initialize'
from /home/y-yagi/.rbenv/versions/2.6.0/lib/ruby/gems/2.6.0/gems/web-console-3.7.0/lib/web_console/template.rb:21:in `new'
from /home/y-yagi/.rbenv/versions/2.6.0/lib/ruby/gems/2.6.0/gems/web-console-3.7.0/lib/web_console/template.rb:21:in `render'
from /home/y-yagi/.rbenv/versions/2.6.0/lib/ruby/gems/2.6.0/gems/web-console-3.7.0/lib/web_console/middleware.rb:37:in `block in call'
from /home/y-yagi/.rbenv/versions/2.6.0/lib/ruby/gems/2.6.0/gems/web-console-3.7.0/lib/web_console/middleware.rb:20:in `catch'
from /home/y-yagi/.rbenv/versions/2.6.0/lib/ruby/gems/2.6.0/gems/web-console-3.7.0/lib/web_console/middleware.rb:20:in `call'
from /home/y-yagi/src/rails/master_y_yagi/rails/actionpack/lib/action_dispatch/middleware/show_exceptions.rb:33:in `call'
There was a problem hiding this comment.
@y-yagi I think so. Can you commit the change?
kamipo
added a commit
to kamipo/rails
that referenced
this pull request
Jan 31, 2019
I deprecated two unused attr_writers `visitor` and `indexes` at 8056fe0 and f4bc364 conservatively, since those are accidentaly exposed in the docs. https://api.rubyonrails.org/v5.2/classes/ActiveRecord/ConnectionAdapters/AbstractAdapter.html https://api.rubyonrails.org/v5.2/classes/ActiveRecord/ConnectionAdapters/TableDefinition.html But I've found that `view_renderer` attr_writer is removed without deprecation at rails#35093, that is also exposed in the doc. https://api.rubyonrails.org/v5.2/classes/ActionView/Base.html I'd like to also remove the deprecated attr_writers since I think that removing `visitor` and `indexes` attr_writers is as safe as removing `view_renderer` attr_writer.
koic
added a commit
to koic/buoys
that referenced
this pull request
Sep 7, 2019
Follow up of rails/rails#35093. This PR suppresses the following warning when using Rails 6.0. ```console % path/to/buoys % bundle exec rake (snip) DEPRECATION WARNING: ActionView::Base instances should be constructed with a lookup context, assignments, and a controller. (called from new at /Users/koic/src/github.com/muryoimpl/buoys/test/buoys_buoy_test.rb:7) ```
koic
added a commit
to koic/buoys
that referenced
this pull request
Sep 7, 2019
Follow up of rails/rails#35093. This PR suppresses the following warning when using Rails 6.0. ```console % path/to/buoys % bundle exec rake (snip) DEPRECATION WARNING: ActionView::Base instances should be constructed with a lookup context, assignments, and a controller. (called from new at /Users/koic/src/github.com/muryoimpl/buoys/test/buoys_buoy_test.rb:7) ```
muryoimpl
pushed a commit
to muryoimpl/buoys
that referenced
this pull request
Sep 8, 2019
Follow up of rails/rails#35093. This PR suppresses the following warning when using Rails 6.0. ```console % path/to/buoys % bundle exec rake (snip) DEPRECATION WARNING: ActionView::Base instances should be constructed with a lookup context, assignments, and a controller. (called from new at /Users/koic/src/github.com/muryoimpl/buoys/test/buoys_buoy_test.rb:7) ```
ghost
mentioned this pull request
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The AV::Base constructor was too complicated, and this commit tightens
up the parameters it will take. At runtime, AV::Base is most commonly
constructed here:
rails/actionview/lib/action_view/rendering.rb
Lines 72 to 74 in 94d54fa
This provides an AV::Renderer instance, a hash of assignments, and a
controller instance. Since this is the common case for construction, we
should remove logic from the constructor that handles other cases. This
commit introduces special constructors for those other cases.
Interestingly, most code paths that construct AV::Base "strangely" are
tests.