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

Fix unintentionally calling to_a on the target #502

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

dorner
Copy link
Contributor

@dorner dorner commented Oct 13, 2023

The fix in #448 had some unintented consequences. We have a custom to_a method on one of our ActiveRecord objects which returns a set of different ActiveRecord objects. (Yes, I'm well aware this violates the Law of Least Surprise, but it was done ages ago and is now so baked into our code that extracting it would be a major undertaking.)

The current code "splats" the target of turbo_stream_action_tag to pass to the dom_id method. The problem is that it's assuming that the object is "array-like" - which the Array() method does not ensure. It uses to_ary or to_a to return an array, but doesn't indicate that the object itself is in fact an array.

This PR fixes this by using Array.wrap, ensuring we actually have an array, and splatting the array into the method rather than the original target (which would call to_a again).

I wasn't able to find a contribution guideline page, so please let me know if anything else needs to be done here (do I need to create an issue?)

Copy link
Contributor

@seanpdoyle seanpdoyle left a comment

Choose a reason for hiding this comment

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

Thank you! Great catch.

@dorner
Copy link
Contributor Author

dorner commented Nov 5, 2023

@seanpdoyle thanks for the approval - any idea when it can be merged?

@dorner
Copy link
Contributor Author

dorner commented Feb 19, 2024

@seanpdoyle bumping this :) Still not fixed in latest turbo-rails.

@seanpdoyle
Copy link
Contributor

@afcapel could you review this?

@brunoprietog
Copy link
Contributor

Thanks @dorner!

@brunoprietog brunoprietog merged commit 669ba6d into hotwired:main Jul 13, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants