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

Implement field_id in terms of the FormBuilder's namespace: option #43408

Merged

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Oct 8, 2021

When constructing the field's [id] attribute, the current
FormBuilder#field_id implementation (introduced in 59ca21c) ignores the
namespace: option.

This commit incorporates any namespace by prepending it to the
@object_name.

@rails-bot rails-bot bot added the actionview label Oct 8, 2021
@seanpdoyle seanpdoyle force-pushed the action-view-form-builder-field-id-options branch from 140ee21 to 5d8a042 Compare October 9, 2021 01:08
@seanpdoyle seanpdoyle force-pushed the action-view-form-builder-field-id-options branch from 5d8a042 to bac65e6 Compare October 29, 2021 13:59
Copy link
Member

@jonathanhefner jonathanhefner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it would be good to include this. 👍

actionview/lib/action_view/helpers/form_helper.rb Outdated Show resolved Hide resolved
@seanpdoyle
Copy link
Contributor Author

seanpdoyle commented Nov 10, 2021

@jonathanhefner I've since discovered that during the Tags::Base class call to add_default_name_and_id relies on a private tag_id:

def add_default_name_and_id(options)
index = name_and_id_index(options)
options["name"] = options.fetch("name") { tag_name(options["multiple"], index) }
if generate_ids?
options["id"] = options.fetch("id") { tag_id(index) }
if namespace = options.delete("namespace")
options["id"] = options["id"] ? "#{namespace}_#{options['id']}" : namespace
end
end
end

That tag_id method invokes @template_object.field_id instead of delegating to the form instance:

def tag_id(index = nil)
@template_object.field_id(@object_name, @method_name, index: index)
end

This means that after these changes, user-land calls to form.field_id will incorporate any namespace: option passed in. However, since the internals skip the FormBuilder instance and call field_id on the @template_object directly, any FormBuilder options are inaccessible at that point, so the built-in [id] generation code will not be affected by these changes.

@jonathanhefner
Copy link
Member

This means that after these changes, user-land calls to form.field_id will incorporate any namespace: option passed in. However, since the internals skip the FormBuilder instance and call field_id on the @template_object directly, any FormBuilder options are inaccessible at that point, so the built-in [id] generation code will not be affected by these changes.

I'm not as familiar with those internals, but it sounds like you are saying this change should be pushed down into @template.field_id, and add_default_name_and_id + tag_id should be modified to thread options["namespace"] to it. Is that accurate?

@seanpdoyle
Copy link
Contributor Author

@jonathanhefner maybe! I'll push on that a bit.

@seanpdoyle seanpdoyle force-pushed the action-view-form-builder-field-id-options branch from 9994078 to a653f9f Compare November 17, 2021 03:14
@seanpdoyle seanpdoyle force-pushed the action-view-form-builder-field-id-options branch 5 times, most recently from faa0ba7 to 46453d2 Compare November 25, 2021 20:32
@seanpdoyle seanpdoyle force-pushed the action-view-form-builder-field-id-options branch 2 times, most recently from 1e044b6 to 02a761e Compare December 3, 2021 16:13
@seanpdoyle seanpdoyle changed the title FormBuilder#field_id: prepend namespace: option Add support for namespace: option to FormBuilder#field_id Dec 7, 2021
@seanpdoyle seanpdoyle force-pushed the action-view-form-builder-field-id-options branch 2 times, most recently from 57cf085 to 739a75f Compare December 9, 2021 00:48
@seanpdoyle
Copy link
Contributor Author

@rafaelfranca since field_id was introduced as part of 7.0.0, would this be a good change to consider for the next release candidate?

@seanpdoyle seanpdoyle changed the title Add support for namespace: option to FormBuilder#field_id Extend FormBuilder#field_id to bake-in support for namespace: option Dec 12, 2021
@seanpdoyle seanpdoyle changed the title Extend FormBuilder#field_id to bake-in support for namespace: option Implement field_id in terms of the FormBuilder's namespace: option Dec 12, 2021
@seanpdoyle seanpdoyle force-pushed the action-view-form-builder-field-id-options branch from 739a75f to a73ca86 Compare December 13, 2021 17:51
When constructing the field's `[id]` attribute, the current
`FormBuilder#field_id` implementation (introduced in [59ca21c][])
ignores the `namespace:` option.

This commit incorporates any namespace by prepending it to the
`@object_name`.

[59ca21c]: rails@59ca21c

Re-use template.field_id
---

Thread options[:namespace] down through the FormBuilder instance to the
`Tags::Base#tag_id` and `#add_default_name_and_id` methods
@seanpdoyle seanpdoyle force-pushed the action-view-form-builder-field-id-options branch from a73ca86 to 0b8dde0 Compare December 14, 2021 23:00
@rafaelfranca rafaelfranca merged commit fc3094d into rails:main Dec 14, 2021
rafaelfranca added a commit that referenced this pull request Dec 14, 2021
…eld-id-options

Implement `field_id` in terms of the FormBuilder's `namespace:` option
@seanpdoyle seanpdoyle deleted the action-view-form-builder-field-id-options branch December 14, 2021 23:25
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

3 participants