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 to_param to maximize content #23636

Merged
merged 1 commit into from Jul 8, 2016

Conversation

Projects
None yet
5 participants
@rab
Contributor

rab commented Feb 12, 2016

To fix #23635

The documentation states that parameter values longer than 20 characters
will be truncated by words, but the example shows that a parameter based
on "David Heinemeier Hansson" (with id: 125) becomes "125-david" when
"David Heinemeier".length == 16 so why so short?

The answer lies in the use of the #truncate option omission: nil which
seems to have been intended to mean "nothing", but which actually causes
the default string "..." to be used. This causes #truncate to cleave
words until the "..." can be added and still remain within the requested
size of 20 characters.

The better option is omission: '' (which is probably what was originally
intended).

Furthermore, since the use of #parameterize will remove non-alphanumeric
characters, we can maximize the useful content of the output by calling
parameterize first and then giving truncate a separator: /-/ rather than
a space.

@rails-bot

This comment has been minimized.

rails-bot commented Feb 12, 2016

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

Please see the contribution instructions for more information.

@@ -100,7 +100,7 @@ def to_param(method_name = nil)
define_method :to_param do
if (default = super()) &&
(result = send(method_name).to_s).present? &&
(param = result.squish.truncate(20, separator: /\s/, omission: nil).parameterize).present?
(param = result.squish.parameterize.truncate(20, separator: /-/, omission: '')).present?

This comment has been minimized.

@vipulnsward

vipulnsward Feb 12, 2016

Member

I think it would be safer to keep parameterize towards the end to not cause any side effects from truncate affect parameterization.

This comment has been minimized.

@rab

rab Feb 12, 2016

Contributor

Please refer to my response near the test for this change (lines 39-44) below.

@vipulnsward

View changes

activerecord/test/cases/integration_test.rb Outdated
@@ -1,3 +1,4 @@
# coding: utf-8

This comment has been minimized.

@vipulnsward

vipulnsward Feb 12, 2016

Member

Is this needed?

This comment has been minimized.

@rab

rab Feb 12, 2016

Contributor

No, it no longer is w/ current rubies, but Emacs puts that in when it detects the need. I can remove it before merging.

def test_to_param_class_method_truncates_after_parameterize
firm = Firm.find(4)
firm.name = "Huey, Dewey, & Louie LLC"

This comment has been minimized.

@rab

rab Feb 12, 2016

Contributor

I don't understand your concern. The benefit to parameterizing prior to truncation is that characters that will not make it into the final result do not influence the truncation. In this case, the commas and the ampersand will not be used in the result, but that makes the length 4 characters more for no reason. In this way we are truncating the URL parameter rather than parameterizing the truncated attribute.

@rab

This comment has been minimized.

Contributor

rab commented Feb 15, 2016

The conflicts are with the CHANGELOG.md and will be resolved with a rebase/squash as soon as it is requested. Is there anything else that you'd like to see from me?

@rab rab force-pushed the rab:fix-to_param-truncation branch Jul 8, 2016

@rab

This comment has been minimized.

Contributor

rab commented Jul 8, 2016

Updated/rebased the fix to be against master now that 5.0.0 is released. The to_param code is unchanged.

@matthewd

This comment has been minimized.

Member

matthewd commented Jul 8, 2016

👍 please squash to a single commit

@matthewd matthewd assigned matthewd and unassigned arthurnn Jul 8, 2016

fix to_param to maximize content
The documentation states that parameter values longer than 20 characters
will be truncated by words, but the example shows that a parameter based
on "David Heinemeier Hansson" (with id: 125) becomes "125-david" when
"David Heinemeier".length == 16 so why so short?

The answer lies in the use of the #truncate option omission: nil which
seems to have been intended to mean "nothing", but which actually causes
the default string "..." to be used. This causes #truncate to cleave
words until the "..." can be added and still remain within the requested
size of 20 characters.

The better option is omission: '' (which is probably what was originally
intended).

Furthermore, since the use of #parameterize will remove non-alphanumeric
characters, we can maximize the useful content of the output by calling
parameterize first and then giving truncate a separator: /-/ rather than
a space.

@rab rab force-pushed the rab:fix-to_param-truncation branch to 3b49d79 Jul 8, 2016

@rab

This comment has been minimized.

Contributor

rab commented Jul 8, 2016

Squashed!

@matthewd matthewd merged commit 4e961ca into rails:master Jul 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthewd

This comment has been minimized.

Member

matthewd commented Jul 8, 2016

Thanks!

This will presumably cause some (URL-based) cache misses, for things that have learned to fit more detail, but that seems fine. /cc @jeremy, Defender of the Cache

@rab

This comment has been minimized.

Contributor

rab commented Jul 8, 2016

Presumably not any more than when the truncation began though.

Well, scratch that. It's been there since 4.1 and I only noticed when upgrading an app from 2.3.18 to 4.2.6 so the comment about cache misses is valid.

@rab rab deleted the rab:fix-to_param-truncation branch Jul 8, 2016

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