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

Fixes partial path resolvings for jbuilder partials. #220

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pgruener
Copy link

For me it was a hard journey to discover this error, especially reasoned by the condition if Rails.env.development?, which lets the error only occur in production environment. And I was looking for some trouble (TypeError: no implicit conversion of nil into Integer) in the affected jbuilder template, which was totally wrong.

I am sorry, that I can't afford the time to write tests for that and I understand, if that's why the PR won't be merged. But as I investigated several hours in finding this issue, I (at least) wanted to point this error out.

So if you didn't ever plan that this method is called with a path with another class-instance than String, you could easily approve it. Otherwise I hope this PR helps somebody as reference, as we implemented that as a monkey patch for now.

Let me know if I should clarify the situation more that I reached with my comment.

Fixes partial path resolvings for jbuilder partials
@jagthedrummer
Copy link
Contributor

Hey @pgruener, this looks like a small enough change, but I don't really understand why we need it. What would be some reproduction steps to see the error that you're trying to prevent? (I see the comment in the code you changed, but I don't quite get it. Is there a particular combination of gems that was giving you trouble?)

@pgruener
Copy link
Author

Hey @pgruener, this looks like a small enough change, but I don't really understand why we need it. What would be some reproduction steps to see the error that you're trying to prevent? (I see the comment in the code you changed, but I don't quite get it. Is there a particular combination of gems that was giving you trouble?)

Hi @jagthedrummer unfortunately there is so much time past, that I actually don't remember which gem caused the error. My fault that I wasn't precise enough.
But yes, you got it in general, that there was a gem, which called json.partial!('show', record: record). And this kind of partial rendering (by permitting the record as hash) causes the error.

And my opinion was, that as long as this is not a forbidden pattern, it should not crash the framework.

So actually you should be able to reproduce it, by making this call by yourself in an own ressource template.

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.

None yet

2 participants