Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix rendering explicit template in 1.1 #125

Closed
wants to merge 2 commits into from

Conversation

Burgestrand
Copy link
Contributor

@Burgestrand Burgestrand commented Nov 27, 2023

See #121 for reported issue.

To get CI to pass I first needed to fix that, which is why 1799dca is part of this PR. See #122 for that in isolation.

Original issue report as follows:

Hi! This report is still early, because I haven't figured out the why yet, but I have figured out the what.

Ruby 3.2.1, Rails 7.1.2.

The change in b0cc8bc broke this code:

def template 
  render(
    template: "orders/layouts/single_room",
    locals: { view: self, dato: }
  )
end

orders/layouts/single_room is an ERB file, but I'm not sure that matters because it never gets that far in the backtrace anyway.

With this backtrace:

     ActionView::Template::Error:
       'nil' is not an ActiveModel-compatible object. It must implement :to_partial_path.
     # /Users/kim/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/actionview-7.1.2/lib/action_view/renderer/abstract_renderer.rb:82:in `partial_path'
     # /Users/kim/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/actionview-7.1.2/lib/action_view/renderer/object_renderer.rb:20:in `render_object_derive_partial'
     # /Users/kim/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/actionview-7.1.2/lib/action_view/renderer/renderer.rb:96:in `render_partial_to_object'
     # /Users/kim/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/actionview-7.1.2/lib/action_view/renderer/renderer.rb:55:in `render_partial'
     # /Users/kim/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/actionview-7.1.2/lib/action_view/helpers/rendering_helper.rb:35:in `block in render'
     # /Users/kim/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/actionview-7.1.2/lib/action_view/base.rb:291:in `in_rendering_context'
     # /Users/kim/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/actionview-7.1.2/lib/action_view/helpers/rendering_helper.rb:33:in `render'
     # /Users/kim/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/phlex-rails-1.1.0/lib/phlex/rails/sgml/overrides.rb:26:in `render'

So far I've only verified that reverting that change only is sufficient for the code to not raise. I tested this by modifying the single line in the installed phlex-rails gem on my machine and re-running my tests.

Order in Gemfile is well-defined. This is sufficient to define the Rails version constant, which is required to be present before rspec-rails or loading will crash with a missing constant.
@Burgestrand
Copy link
Contributor Author

I believe I found a suitable fix, and so this is ready for review.

The PR that introduced this problem is #113. I do not know if the change I made just now brings that problem back.

This call style/parameter is documented in e.g: https://guides.rubyonrails.org/layouts_and_rendering.html#rendering-an-action-s-template-from-another-controller

In b0cc8bc the code was changed from _only passing a block to render if one was originally given_ to _always passing a block to render regardless_. This is enough to break the call to render in some situations.

This change only passes a block to render if one was actually given to begin with.
@Burgestrand
Copy link
Contributor Author

I found a more focused test approach, and added a regression test for #113 too. Turns out my initial fix did indeed break that, but now with my recent change (force-pushed, I didn't think that piece of history was worth preserving) this passes both use-cases.

With that I'm done with this until you've had a look.

@joeldrapper
Copy link
Collaborator

Thanks for this. Your fix looks good.

@joeldrapper
Copy link
Collaborator

joeldrapper commented Nov 27, 2023

I’m merging this in #130 with a minor fix for older versions of Ruby.

@Burgestrand
Copy link
Contributor Author

Great, thanks! Oh, I forgot to update the changelog.

@Burgestrand Burgestrand deleted the kbs/fix-render-template branch November 27, 2023 13:49
@joeldrapper
Copy link
Collaborator

No problem. I’ll sort the change log now. I’m going to do a patch release with this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants