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

Adds a specific ArgumentError when passing nil to dom_id. #48987

Merged
merged 1 commit into from Aug 22, 2023

Conversation

Austio
Copy link
Contributor

@Austio Austio commented Aug 20, 2023

Motivation / Background

I created this PR to make invalid arguments passed to dom_id raise a specific error when it receives an invalid argument.

For example with the following

dom_id(@something_non_existent)

Before this commit it would raise the following: NoMethodError: undefined method 'to_key' for nil:NilClass
After it raises ArumentError: dom_id must be passed a record_or_class as the first parameter, you passed 'nil'

Detail

This PR updates dom_id to validate it's argument and raise an ArgumentError whenever it is passed nil.
The current NoMethodError: undefined method 'to_key' for nil:NilClass error is relatively difficult to identify what is going on because it occurs when the record is cast to a key

The NoMethod error is raised when the argument that is passed to dom_id is sent to record_key_for_dom_id

def dom_id(record_or_class, prefix = nil)
  # nil is passed down the the private method  
  record_id = record_key_for_dom_id(record_or_class) unless record_or_class.is_a?(Class)
   ...
end

# this will raise the error which is harder to identify
def record_key_for_dom_id(record) # :doc:
  key = convert_to_model(record).to_key
  key ? key.join(JOIN) : key
end

Passing a nil argument can happen in many scenarios, but a specific example is misspelling an instance variable

# Set somewhere in your code path
@ivar = 'foo'

# somewhere else, like a view, the @ivar is misspelled
dom_id(@viar)

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

…appen if you do something like pass a non-existent ivar dom_id(@something_non_existant)

Before this would raise: `NoMethodError: undefined method `to_key' for nil:NilClass`
After it raises `ArumentError: dom_id must be passed a record_or_class as the first parameter, you passed 'nil'`
@byroot byroot merged commit 40d761c into rails:main Aug 22, 2023
9 of 10 checks passed
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