Fix the type of handlers options passed to render #2521

alpaca-tc · 2021-09-30T15:29:09Z

When calling #render without arguments, #_render_options infers default
options. Correctly, the type of handlers should be symbols, but now it is
strings.

In Rails7, if the type of handlers isn't symbols, action_view's template
resolver can't find the template. This is because the implementation of template search has been changed to match handlers exactly.

related changes: https://github.com/rails/rails/pull/42210/files#diff-b356f21d70a4c088415a9c4f315bbccbc979c2ddeda6b8cc5d76770084e5bc6bR31

NOTE: The actual type used by template resolver can be checked with the following command.

> ActionController::Base.new.lookup_context.handlers
=> [:raw, :erb, :html, :builder, :ruby, :jbuilder] # This is symbols, not strings

JonRowe · 2021-10-01T14:36:37Z

👋 Thanks for this! Our versioning scheme means it'd be a major version bump to support Rails 7, so I might not merge this to main. Do you have a scenario that would trigger this? As I'm surprised there was no change required to our scenarios for this.

alpaca-tc · 2021-10-01T16:01:02Z

so I might not merge this to main.

👌 😄

Do you have a scenario that would trigger this? As I'm surprised there was no change required to our scenarios for this.

Sorry I can't reproduce this bug in one file. Please check the following 🐕

$ rails --version #=> 7.0.0.alpha1
$ rails new test-app --minimal

...and applied changes. 
https://gist.github.com/alpaca-tc/9ce5c09bae477be96534338933627df8

I've also created a repository, but I'm going to delete it later.

$ git clone https://github.com/alpaca-tc/test-app
$ cd test-app
$ bundle install
$ bundle exec rspec

pirj · 2021-10-01T17:13:56Z

Probably something fails, but we've allowed failures for Rails master, and GitHub deceptively shows it as green, while it's actually red if you expand "Run script/build":

NameError:
  uninitialized constant Rack::Request::Env
# ./spec/spec_helper.rb:27:in `<top (required)>'

pirj · 2021-10-01T19:02:34Z

The problem with Rack::Request::Env is due to this rack/rack#1768 in rack master.
When we test against Rails ~~master~~ main, we also use the latest rack from git.

CI fix #2523:

🍏 specs for Rails main pass so far
🍏 dependencies for features resolve
🍏 sprockets complain about missing manifest.js

~~@alpaca-tc I'm off until Sunday, do you want to take over to push that other PR further?~~

pirj · 2021-10-01T19:56:38Z

Alright, here's the failing test we were looking for:

Failures:

  1) admin/accounts/index renders a list of admin/accounts
     Failure/Error: <%= render @admin_accounts %>

     ActionView::Template::Error:
       Missing partial admin/accounts/_account with {:locale=>[:en], :formats=>[:html, :text, :js, :css, :ics, :csv, :vcf, :vtt, :png, :jpeg, :gif, :bmp, :tiff, :svg, :mpeg, :mp3, :ogg, :m4a, :webm, :mp4, :otf, :ttf, :woff, :woff2, :xml, :rss, :atom, :yaml, :multipart_form, :url_encoded_form, :json, :pdf, :zip, :gzip], :variants=>[], :handlers=>[:raw, :erb, :html, :builder, :ruby, :coffee, :jbuilder]}.

       Searched in:
         * "/home/runner/work/rspec-rails/rspec-rails/tmp/example_app/app/views"
         * "/home/runner/work/rspec-rails/bundle/ruby/2.7.0/bundler/gems/rails-c2b083df913f/actiontext/app/views"
         * "/home/runner/work/rspec-rails/bundle/ruby/2.7.0/bundler/gems/rails-c2b083df913f/actionmailbox/app/views"
     # ./app/views/admin/accounts/index.html.erb:6:in `_app_views_admin_accounts_index_html_erb___1899867805406451531_30760'
     # ./spec/views/admin/accounts/index.html.erb_spec.rb:16:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # ActionView::MissingTemplate:
     #   Missing partial admin/accounts/_account with {:locale=>[:en], :formats=>[:html, :text, :js, :css, :ics, :csv, :vcf, :vtt, :png, :jpeg, :gif, :bmp, :tiff, :svg, :mpeg, :mp3, :ogg, :m4a, :webm, :mp4, :otf, :ttf, :woff, :woff2, :xml, :rss, :atom, :yaml, :multipart_form, :url_encoded_form, :json, :pdf, :zip, :gzip], :variants=>[], :handlers=>[:raw, :erb, :html, :builder, :ruby, :coffee, :jbuilder]}.
     #   
     #   Searched in:

~~@alpaca-tc I suggest you to rebase on top of fix-rails-7-build branch and see if your changes fixed the failure.~~
EDIT: fix-rails-7-build is merged, and I've rebased this branch on main.

pirj · 2021-10-02T19:19:06Z

Sadly enough, this fix doesn't fix the failing spec, something seems to still be missing.
Should we .to_sym match[:locale], too?

I've rebased and force-pushed this branch.

Can you try running cucumber locally, @alpaca-tc ?
To run against Rails' main:

rm Gemfile.lock
export RAILS_VERSION='main'
bundle
rm -rf tmp
cucumber

alpaca-tc · 2021-10-03T17:38:38Z

I see, it is a cucumber scenario 🥲

It seems that the content of tmp/example_app/app/views/admin/accounts/index.html.erb created by rails7 is different from that of the template created by rails6.
Moreover, the template created by rails7 contains the bug. render @admin_accounts doesn't work in every rails versions.

~~I am not familiar with the behavior of cucumber, how can I fix this?~~
It seems to be a bug in the rails scaffold. https://github.com/rails/rails/pull/41210/files#diff-f00cb6b8a5dac4a72299b7c0fc2962b5fc554a44e5452a8c0a7fb47b97080ad2R6

I'll try to fix this bug tomorrow. rails/rails#43365

pirj · 2021-11-22T19:34:47Z

Your Rails bugfix PR is merged, but there's now an inconsitency due to the recently removed ActionMailer::DeliveryJob #2531.

pirj · 2021-11-23T07:59:03Z

spec/rspec/rails/example/view_example_group_spec.rb

        end

        it "converts the filename without format into render options" do
          allow(view_spec).to receive(:_default_file_to_render) { "widgets/new.en.erb" }
          view_spec.render
-          expect(view_spec.received.first).to eq([{template: "widgets/new", locales: ['en'], handlers: ['erb']}, {}, nil])
+          expect(view_spec.received.first).to eq([{template: "widgets/new", locales: [:en], handlers: [:erb]}, {}, nil])


I would feel confident with merging this to main if we had a confirmation that with to_sym all supported Rails versions (from soft-supported 5.0 and 5.1) up to 6.1 still work fine.

Can you recall, what was the failing cucumber scenario for 7.0 without .to_sym exactly?

I can't recall but I think that calling render without arguments raises ActionView::MissingTemplate.

It seems that an exception is raised if the path described in the description contains handler, locales, etc. as shown below. Sorry I don't know that there is such a test in rspec-rails' cucumber.

RSpec.describe "root/index.html.erb", type: :view do # also xxx.html.slim, xxx.en.html.erb it { render } #=> raise ActionView::MissingTemplate end

My concern is that the change would break on an older version of Rails. As this is something that was not thought forward to be changed in the future, there might not be a test for it. Sorry for being extra cautious.

@pirj In that case would it be possible to release this as a new major version

@pirj So I just saw this discussion after making a similar commit to the PR I opened a few days ago. I'm a little confused as to what the concern is.

We know symbols work on Rails versions in CI because the relevant Cucumber feature (features/view_specs/view_spec.feature:124) passes for those versions. If symbols didn't work on a particular Rails version that feature would fail.

features/view_specs/view_spec.feature:124

Fair enough. For some reason, I failed to find this feature.

Wondering if it works equally fine with variant and locale.

For the reference, a discussion about format.to_sym.
This discussion has an argument "if they don't support symbols, it's a Rails bug, and people using older versions of Rails, they should stick to older versions of rspec-rails". We've started adhering to semver since then, and currently we officially support Rails 5.2 and soft-support 5.0 and 5.1 (please correct if I'm wrong with this again). From my perspective, it would be quite irresponsible to break the current rspec-rails 5.0.x for users of Rails 5.
Even though we claim that:

According to RSpec Rails new versioning strategy use:
rspec-rails 5.x for Rails 6.x.
rspec-rails 4.x for Rails from 5.x or 6.x.

and

5.0.0 / 2021-03-09

Drop support for Rails below 5.2.

We have a release strategy @JonRowe has described here that I've recently violated with the merge of that PR, hopefully without any breaking impact for the users.
For the future researchers, it might be surprising that Rails 7.0 compatibility support appears in rspec-rails 5.0.3/5.1.0 releases that target Rails 6.0 and 6.1.

I suggest branching rails-7-0-dev from main, and merging this PR and #2552 to it.
When we release 5.0.3 and 5.1.0, it can be merged back to main, and it will be time for rspec-rails 6.0.

@JonRowe Does this make sense?

For what it's worth, that approach sounds reasonable to me.

That said I'm not sure why there's a "soft support" for Rails 5 and 5.1. If you want to support them, why not just add them to CI? If they can't be added to CI, are they actually supported?

It's worth noting that PR #2552 + #2546 has only 2 code changes of very limited scope that could end users of spec-rails:

The symbol change for the handler argument ("erb" becomes :erb before being passed to ActionView::Base). This only becomes relevant in a particular type of view spec.

The changes to argument handling for Action Mailer matches (in my view this is the far more likely to break item between Rails versions)

On top of these changes, this PR symbolizes a few other values passed to ActionView::Base.

That's a very small, tightly scoped set of changes. So the potential blast radius and risk of impact on end users is very limited.

if you want to support them, why not just add them to CI

We don't want to support them, and we don't want to break them. The reason is to drop some burden, mostly branching in specs:

drop support for Rails < 5.2

drop support for Rails < 5.0

Actually, by looking at the former, there are two changes that presumably break Rails 5.0 support. But 5.1 may still work.

PR #2552 + #2546 has only 2 code changes of very limited scope

Right 👍
Please don't get me wrong with my rant above, I'm all for merging both ASAP and releasing this as 6.0.0-pre without bothering with 5.0.3 and 5.1.0.

I guess I just consider "soft support" very harmful because it adds ambiguity and discussion to almost any change, without any kind of authoritative answer. If it's in CI, then you know you didn't break it (or you broke it and need to fix it). Saving a few branches is in my view not worth the ambiguity on nearly every material PR. In my experience it sows up work (and open source work) substantially.

But I'm not a maintainer here, so I'll of course defer to the library maintainers.

When calling #render without arguments, #_render_options infers default options. Correctly, the type of handlers should be symbols, but now it is strings. In Rails7, if the type of handlers isn't symbols, action_view's template resolver can't find the template. This is because the implementation of template search has been changed to match handlers exactly. related: https://github.com/rails/rails/pull/42210/files#diff-b356f21d70a4c088415a9c4f315bbccbc979c2ddeda6b8cc5d76770084e5bc6bR31

…ated In Rails 3.2, passing formats or handlers to render :template and friends like `render :template => "foo.html.erb"` is deprecated. Instead, you can provide :handlers and :formats directly as options: ` render :template => "foo", :formats => [:html, :js], :handlers => :erb`.

alpaca-tc · 2022-01-12T09:41:04Z

Some commits(2a2c065 0f133af) are conflicted with #2552 , so I removed the commits and force-pushed.

pirj · 2022-01-20T10:29:50Z

Thank you, @alpaca-tc !

pirj mentioned this pull request Oct 1, 2021

Fix Rails 7 build #2523

Merged

pirj force-pushed the fix_handlers branch from fcd848d to 684e6e0 Compare October 2, 2021 18:52

pirj added the rails7 label Nov 16, 2021

pirj mentioned this pull request Nov 16, 2021

Remove explicit sprockets dependency for Rails 7 #2530

Merged

pirj force-pushed the fix_handlers branch from f51d4bb to 91075ff Compare November 22, 2021 18:50

pirj reviewed Nov 23, 2021

View reviewed changes

pirj mentioned this pull request Dec 7, 2021

Rails 7.0.0.rc1 view specs fail. #2539

Closed

pirj added this to the 6.0 milestone Dec 10, 2021

mitchellhenke mentioned this pull request Dec 29, 2021

Upgrade to Rails 7 18F/identity-idp#5762

Merged

alpaca-tc added 3 commits January 12, 2022 13:23

Fix the type of options passed to render

96a08e0

alpaca-tc force-pushed the fix_handlers branch 10 times, most recently from f4736f6 to 0f133af Compare January 12, 2022 08:51

alpaca-tc force-pushed the fix_handlers branch from 0f133af to 96a08e0 Compare January 12, 2022 09:39

petergoldstein mentioned this pull request Jan 12, 2022

Add Ruby 3.1 to CI and fix Rails 7 #2552

Closed

JonRowe mentioned this pull request Jan 18, 2022

RSpec Rails 6.0 Release Plan #2560

Closed

21 tasks

pirj merged commit 3ccd1f7 into rspec:main Jan 20, 2022

masarakki mentioned this pull request Mar 20, 2022

fix-finding-extension #2325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the type of handlers options passed to render #2521

Fix the type of handlers options passed to render #2521

alpaca-tc commented Sep 30, 2021 •

edited

JonRowe commented Oct 1, 2021

alpaca-tc commented Oct 1, 2021

pirj commented Oct 1, 2021 •

edited

pirj commented Oct 1, 2021 •

edited

pirj commented Oct 1, 2021 •

edited

pirj commented Oct 2, 2021 •

edited

alpaca-tc commented Oct 3, 2021 •

edited

pirj commented Nov 22, 2021

pirj Nov 23, 2021

alpaca-tc Nov 24, 2021

alpaca-tc Nov 24, 2021

pirj Nov 24, 2021

jcoyne Dec 10, 2021

petergoldstein Jan 12, 2022

pirj Jan 12, 2022

petergoldstein Jan 12, 2022

pirj Jan 12, 2022

petergoldstein Jan 12, 2022

alpaca-tc commented Jan 12, 2022

pirj commented Jan 20, 2022

Fix the type of handlers options passed to render #2521

Fix the type of handlers options passed to render #2521

Conversation

alpaca-tc commented Sep 30, 2021 • edited

JonRowe commented Oct 1, 2021

alpaca-tc commented Oct 1, 2021

pirj commented Oct 1, 2021 • edited

pirj commented Oct 1, 2021 • edited

pirj commented Oct 1, 2021 • edited

pirj commented Oct 2, 2021 • edited

alpaca-tc commented Oct 3, 2021 • edited

pirj commented Nov 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5.0.0 / 2021-03-09

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpaca-tc commented Jan 12, 2022

pirj commented Jan 20, 2022

alpaca-tc commented Sep 30, 2021 •

edited

pirj commented Oct 1, 2021 •

edited

pirj commented Oct 1, 2021 •

edited

pirj commented Oct 1, 2021 •

edited

pirj commented Oct 2, 2021 •

edited

alpaca-tc commented Oct 3, 2021 •

edited