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

Allow redefining to_param delimiter using param_delimiter #49020

Merged
merged 2 commits into from Aug 23, 2023

Conversation

nvasilevski
Copy link
Contributor

@nvasilevski nvasilevski commented Aug 23, 2023

This commit allows customizing the delimiter used by to_param when to_key returns multiple value. This unblocks supporting more varieties of composite primary key types in Active Record.

This PR has a two commits:

  1. Active Model addition - adds param_delimiter attribute to configure the delimiter in Active Model
  2. Active Record change - tweaks to_param to support composite primary key and utilizes newly added param_delimiter along with defining AR default delimiter to _

It is possible to add composite primary key support to to_param without the delimiter abstraction by hardcoding certain value but it's really hard to choose one since we can't account for all possible values of the composite primary key values to avoid conflicts. And asking applications to override whole to_param seems inconvenient

I would be more than glad to split these changes into two separate PR and avoid squashing as I would definitely like to keep these commits separated

This commit allows customizing the delimiter used by `to_param` when
`to_key` returns multiple value. This unblocks supporting more varieties
of composite primary key types in Active Record.
@rafaelfranca rafaelfranca merged commit d70707d into rails:main Aug 23, 2023
4 checks passed
@rafaelfranca rafaelfranca deleted the allow-redefining-to-param-delimiter branch August 23, 2023 22:11
@etiennebarrie etiennebarrie mentioned this pull request Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants