Fix missing partial iteration

[meagar]

Summary

Fixes #27794

The iteration variable was being assigned to the correct key in the locals hash, but the name of the iteration variable (<partial_name>_iteration) was not included in the keys to find_template, meaning it was excluded from keys, a partial to be loaded without the <partial_name>_iteration variable defined as one of it's locals during initialization.

Other Information

I produced a simple POC against Rails 5.0.1, and created screenshots of the actual and expected behavior.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @chancancode (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

[eileencodes]

Thanks for working on this! Can you add a test so we don't have a regression in the future?

[eileencodes]

And this branch should be pointing at master, unless this is not broken on master?

[meagar]

@eileencodes Sure, tests incoming, familiarizing myself with the test suite.

Is the convention to fix these bugs on master and then merge into specific release branches? I've never contributed to Rails before.

[eileencodes]

No worries! The convention is to fix on master and merge to the appropriate branches. I tried changing the base for you but there's just too much that's different between master and 5-0-stable that it would be better to open a new PR pointing to master.

[meagar]

Fixed to merge into master, tests shortly.

[meagar]

I added a test, which necessitated adding two new fixture partials. I tried re-using the existing local_inspector partial, but there are two problems:

• I need two different partials to reproduce the issue, otherwise flow reaches collection_with_template which doesn't exhibit the same problem behavior as collection_without_template
• the local_inspector partial returns the contents of local_assigns, which doesn't demonstrate the problem I'm fixing

There is potentially a lower-level issue with ERB templates that my pull request doesn't address. A template gets a list of local names when built (as an array of strings) and then later when executed it gets a local_assigns hash which contains the values meant to be provided as locals.

See https://github.com/rails/rails/blob/master/actionview/lib/action_view/template.rb#L325

Only the variables previously set in @locals will actually be defined as local variables; the rest will be available in local_assigns. In this case, the variable <partial_name>_iteration was not set (because the name was omitted from the list of local names) , but local_assigns does contain a :<partial_name>_iteration key with the correct value.

I'm not sure if this is intentional behavior, but my fix is to simply create the template with the correct list of local names.

[meagar]

Not sure how to move forward with regards to the failing test in TravisCI. The test is passing for me locally, and I can't imagine how my changes could have induced a failure.

[meagar]

@eileencodes Not sure if there's something I should be doing to bump this along?

[eileencodes]

hey @meagar - sometimes tests fail randomly that have nothing to do with your branch so don't worry about that if it's not failing for you.

When we get around to reviewing your PR and merging we will. Hang tight. 🙂

[rafaelfranca]

Backported in 24c585d. Thanks!