-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
Conversation
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.
36f9e56
to
b75432b
Compare
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.
b75432b
to
1456928
Compare
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. |
Thanks for this. Your fix looks good. |
I’m merging this in #130 with a minor fix for older versions of Ruby. |
Great, thanks! Oh, I forgot to update the changelog. |
No problem. I’ll sort the change log now. I’m going to do a patch release with this fix. |
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:
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:
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.