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

Use public_send in value_for_collection #33547

Merged
merged 5 commits into from Aug 22, 2018
Merged

Use public_send in value_for_collection #33547

merged 5 commits into from Aug 22, 2018

Conversation

@Ana06
Copy link
Contributor

@Ana06 Ana06 commented Aug 7, 2018

Deprecate use of private methods in view's helpers. We should start calling public_send in value_for_collection instead of send to void exposing private methods in view's helpers. :bowtie:

Fixes #33546

@rails-bot
Copy link

@rails-bot rails-bot commented Aug 7, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @georgeclaghorn (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.

Avoid exposing private methods in view's helpers.

Fixes rails#33546
@matthewd
Copy link
Member

@matthewd matthewd commented Aug 7, 2018

Hi!

I agree we should change this. (And the other send I spotted a few lines above.)

If it's worth fixing, it's worth keeping fixed, so we should add a test too.

My big question-mark, however, is whether we need a deprecation. Much as public_send is a far more sensible choice, it seems likely someone is currently, perhaps inadvertently, relying on the fact we call private methods. @rafaelfranca?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Aug 7, 2018

Agree. It is safer if we deprecate it.

@Ana06
Copy link
Contributor Author

@Ana06 Ana06 commented Aug 8, 2018

@matthewd

And the other send I spotted a few lines above.

👍

If it's worth fixing, it's worth keeping fixed, so we should add a test too.

I'll add it 😉

@matthewd @rafaelfranca regarding the deprecation, I guess you mean showing a message in case that the method used is private and not to deprecate the method itself, right?

@matthewd
Copy link
Member

@matthewd matthewd commented Aug 8, 2018

Right. I think we'd do it something like this:

begin
  item.public_send(value)
rescue NoMethodError
  if item.respond_to?(value, true) && !item.respond_to?(value)
    ActiveSupport::Deprecation "Hey don't do that"
    item.send(value)
  else
    raise
  end
end

Avoid exposing private methods in view's helpers. However, as
`extract_values_from_collection` is only called from
`options_from_collection_for_select` where `value_for_collection` is
previously called, this case was already covered. The change makes
anyway sense for consistency and in case the code changes in the
future.
@Ana06 Ana06 force-pushed the patch-1 branch 2 times, most recently from 1fb8b26 to ca77aab Aug 8, 2018
@Ana06
Copy link
Contributor Author

@Ana06 Ana06 commented Aug 8, 2018

I think at least 0c62e14 should be rebased into 87b6e6a, but I left it like that following @rails-bot instructions:

If any changes to this PR are deemed necessary, please add them as extra commits.

Instead of dropping it completely in case someone is relying (probably
inadvertenly) on it.
@Ana06 Ana06 force-pushed the patch-1 branch 3 times, most recently from 6adcd14 to def801a Aug 8, 2018
Test that using private methods in `options_from_collection_for_select`
is deprecated.

Make the unused `secret` paramether in the `Post` Struct private to use
it in the test.
@Ana06
Copy link
Contributor Author

@Ana06 Ana06 commented Aug 8, 2018

@matthewd @rafaelfranca I think the changes are addressed, please take a look. 🙏 Do we need to test other view's helpers or is it enough with testing options_from_collection_for_select? 🤔

@georgeclaghorn
Copy link
Contributor

@georgeclaghorn georgeclaghorn commented Aug 10, 2018

@rails-bot rails-bot assigned matthewd and unassigned georgeclaghorn Aug 10, 2018
@Ana06
Copy link
Contributor Author

@Ana06 Ana06 commented Aug 21, 2018

@matthewd something else to change? 🤔

@matthewd matthewd merged commit 0853cdf into rails:master Aug 22, 2018
1 of 2 checks passed
matthewd added a commit that referenced this issue Aug 22, 2018
Use public_send in value_for_collection
@matthewd
Copy link
Member

@matthewd matthewd commented Aug 22, 2018

Nope, this is great, thanks! ❤️

I made a few of tweaks in the merge commit, FYI: 047a893 (changelog formatting, and I added the name of the called private method to the deprecation message)

@Ana06
Copy link
Contributor Author

@Ana06 Ana06 commented Aug 22, 2018

@matthewd

I made a few of tweaks in the merge commit, FYI: 047a893 (changelog formatting, and I added the name of the called private method to the deprecation message)

I could have made the changes, now my name is not in the commit anymore 😭 😭 😭

@matthewd
Copy link
Member

@matthewd matthewd commented Aug 22, 2018

@Ana06 all your commits are in there: https://contributors.rubyonrails.org/contributors/ana-maria-martinez-gomez/commits (Oops.. if I'd seen there were so many I should've gotten you to squash. Oh well. 🤷🏻‍♂️)

@Ana06
Copy link
Contributor Author

@Ana06 Ana06 commented Aug 22, 2018

@matthewd

@Ana06 all your commits are in there: https://contributors.rubyonrails.org/contributors/ana-maria-martinez-gomez/commits (Oops.. if I'd seen there were so many I should've gotten you to squash. Oh well. 🤷🏻‍♂️)

you are right, sorry 🙈 I though you squashed the commits, because they needed to be squashed and I didn't see them in the commits history. It seems it is a different way to merge things (withour rebasing) and I didn't see them 👀

Thanks for merging it 😉

@matthewd
Copy link
Member

@matthewd matthewd commented Aug 22, 2018

No worries, thanks for the work! (It's an ordinary merge, but that means your commits are further down the list [currently on page 5 by my count], based on their commit date, even though they only just hit master.)

@Ana06
Copy link
Contributor Author

@Ana06 Ana06 commented Aug 28, 2018

@matthewd @rafaelfranca I spoke about this PR in my talk at EuRuKo (in front of around ~550 people), maybe you want to watch it: https://youtu.be/jUc8InwoA-E?t=2m54s 😉

The part where I speak about this is almost as the end, but I am not sure if it is possible to follow without watching the whole talk. 🤔 It is anyway not that long! only 25 minutes 😄

@Ana06 Ana06 deleted the patch-1 branch Apr 30, 2019
suketa added a commit to suketa/rails_sandbox that referenced this issue May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants