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

Allow specifying encoding of parameters by action #26092

Merged
merged 1 commit into from Aug 10, 2016

Conversation

Projects
None yet
7 participants
@kerrizor
Contributor

kerrizor commented Aug 9, 2016

At GitHub we need to handle parameter encodings that are not UTF-8. This
patch allows us to specify encodings per parameter per action.

"We know that this doesn't handle nested parameters, but we don't really need that right now." -- @tenderlove

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Aug 9, 2016

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

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

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Aug 9, 2016

Member

I'm slightly unsure about making this [only] action-specific; seems likely one might have a few well-known parameter names that are differently-encoded application wide. But I guess we can consider that later, if it starts to feel onerous.

Looks like we need a test to prove they are in fact action specific, though... AFAICS, the current tests would pass if we ignored action entirely. Ideally, I'd like to see a request where bar or baz doesn't change encoding (because the action doesn't match), and another showing that a second encoding definition for the same parameter name (but on a different action) also does the right thing.

Member

matthewd commented Aug 9, 2016

I'm slightly unsure about making this [only] action-specific; seems likely one might have a few well-known parameter names that are differently-encoded application wide. But I guess we can consider that later, if it starts to feel onerous.

Looks like we need a test to prove they are in fact action specific, though... AFAICS, the current tests would pass if we ignored action entirely. Ideally, I'd like to see a request where bar or baz doesn't change encoding (because the action doesn't match), and another showing that a second encoding definition for the same parameter name (but on a different action) also does the right thing.

@kerrizor

This comment has been minimized.

Show comment
Hide comment
@kerrizor

kerrizor Aug 9, 2016

Contributor

I'm slightly unsure about making this [only] action-specific; seems likely one might have a few well-known parameter names that are differently-encoded application wide. But I guess we can consider that later, if it starts to feel onerous.

Yeah, I had considered adding that, but it wasn't something we were using or needing ATM. Practically speaking, there were only 2-3 action/param combos that needed this.

Contributor

kerrizor commented Aug 9, 2016

I'm slightly unsure about making this [only] action-specific; seems likely one might have a few well-known parameter names that are differently-encoded application wide. But I guess we can consider that later, if it starts to feel onerous.

Yeah, I had considered adding that, but it wasn't something we were using or needing ATM. Practically speaking, there were only 2-3 action/param combos that needed this.

Allow specifying encoding of parameters by action
At GitHub we need to handle parameter encodings that are not UTF-8. This
patch allows us to specify encodings per parameter per action.
@kerrizor

This comment has been minimized.

Show comment
Hide comment
@kerrizor

kerrizor Aug 9, 2016

Contributor

@matthewd tests added!

Contributor

kerrizor commented Aug 9, 2016

@matthewd tests added!

@maclover7 maclover7 added the actionpack label Aug 9, 2016

@tenderlove tenderlove merged commit 79e27b9 into rails:master Aug 10, 2016

1 check passed

codeclimate Code Climate didn't find any new or fixed issues.
Details

kamipo added a commit to kamipo/rails that referenced this pull request Aug 11, 2016

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 12, 2016

Member

This is missing documentation. Can you send a new PR documenting this feature?

Member

rafaelfranca commented Aug 12, 2016

This is missing documentation. Can you send a new PR documenting this feature?

@kerrizor kerrizor deleted the kerrizor:kerrizor/force-param-encoding branch Aug 15, 2016

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Jan 10, 2017

Member

@rafaelfranca have we ever added the docs for this?

Member

arthurnn commented Jan 10, 2017

@rafaelfranca have we ever added the docs for this?

@matthewd

This comment has been minimized.

Show comment
Hide comment

voxik added a commit to voxik/rails-observers that referenced this pull request Jul 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment