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

Avoid passing {format:nil} from form_for/form_with to polymorphic_path #43766

Conversation

cryptoque
Copy link
Contributor

@cryptoque cryptoque commented Dec 2, 2021

Summary

In #43421 part of the functionalities of form_for was delegated to form_with, this could potentially cause some issues in that the following code in form_for:

if options[:url] != false
  options[:url] ||= if options.key?(:format)
    polymorphic_path(record, format: options.delete(:format))
  else
    polymorphic_path(record, {})
  end
end

is removed and replace with functionality inside form_with:

if url != false
  url ||= polymorphic_path(model, format: format)
end

This caused some noticeable issues in the latest rails upgrade for GitHub. As when format is not passed, polymorphic_path(model, {format: nil}) is called instead of the original polymorphic_path(model, {}) and

if options.empty?
  recipient.public_send(method, *args)
else
  recipient.public_send(method, *args, options)
end

and the extra options in recipient.public_send(method, *args, options) is potentially a breaking change for the underlying method.

This PR adds back the condition to pass {} if option is empty in form_with and added a test to make sure it doesn't break form_with.

Other Information

Thanks to @TooManyBees for help with debug this 🙇‍♀️

@rails-bot rails-bot bot added the actionview label Dec 2, 2021
@cryptoque cryptoque force-pushed the fix-regression-bug-form-for-polymorphic-path-options branch from 9a8f52d to f5cb993 Compare December 2, 2021 21:05
@cryptoque cryptoque changed the title Fix regress in form_for after form_for was delegated to form_with, pass {} arg to polymorphic_path when format is nil Fix issue in form_for after form_for was delegated to form_with, restore passing {} arg to polymorphic_path instead of {format:nil} when format is nil Dec 2, 2021
@cryptoque cryptoque marked this pull request as ready for review December 2, 2021 21:16
…to form_with, this could potentially cause some regress, as the condition to send polymorphic_path(record, {}) instead of polymorphic_path(record, format: options.delete(:format)) is removed and when format is not passed, this causes issues if the caller is not expecting an extra arg format:nil. This PR adds back the condition inside for with and add a test to make sure it doesn't break for_with
@cryptoque cryptoque force-pushed the fix-regression-bug-form-for-polymorphic-path-options branch from 8e5feee to c92ac83 Compare December 2, 2021 21:30
@@ -754,7 +754,11 @@ def form_with(model: nil, scope: nil, url: nil, format: nil, **options, &block)

if model
if url != false
url ||= polymorphic_path(model, format: format)
url ||= if format.nil?
Copy link
Member

Choose a reason for hiding this comment

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

Should this behave differently if called as form_with(format: nil) instead of form_with() (without a format?). It looks like the old code in was using options.key? and options.delete

IMO it seems fine that format: nil is the same as unspecified, but if that has a difference to polymorphic_path maybe it is significant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO it seems fine that format: nil is the same as unspecified, but if that has a difference to polymorphic_path maybe it is significant.

Personally I think it should be fine given that the only real usage of the format:nil within polymorphic_path (via method polymorphic_method) is it being passed on as part of options to the method:

if options.empty?
   recipient.public_send(method, *args)
else
    recipient.public_send(method, *args, options)
end

I think most likely the method itself can handle the difference between format:nil vs format not specified, as format:nil isn't a meaningful thing to pass on... and there is likely a default value if a method is expecting a format.

@jhawthorn jhawthorn changed the title Fix issue in form_for after form_for was delegated to form_with, restore passing {} arg to polymorphic_path instead of {format:nil} when format is nil Avoid passing {format:nil} from form_for/form_with to polymorphic_path Dec 3, 2021
@jhawthorn jhawthorn merged commit 3103e63 into rails:main Dec 3, 2021
@@ -754,7 +754,11 @@ def form_with(model: nil, scope: nil, url: nil, format: nil, **options, &block)

if model
if url != false
url ||= polymorphic_path(model, format: format)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was curious to see if we could implement this as polymorphic_path(model, { format: format }.compact).

But then I noticed that I can't get the test to fail if I change the code back to the version before this PR. @cryptoque can you double-check the test to see if it needs tweaking?

3.0.2 ~/code/rails/actionview main *= bin/test test/template/form_helper_test.rb -n /new_form_for_without_format/
Running 305 tests in parallel using 8 processes
Run options: -n /new_form_for_without_format/ --seed 41714

# Running:

.

Finished in 0.204844s, 4.8818 runs/s, 4.8818 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaspth that is good point, I will modify the test on Monday.

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