-
Notifications
You must be signed in to change notification settings - Fork 59
Fix #9: Substitution context replaces ? with false.
#11
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
Conversation
|
Is |
|
I think this regex decides it: https://github.com/rails/rails-deprecated_sanitizer/blob/master/lib/rails/deprecated_sanitizer/html-scanner/html/selector.rb#L540 Here's the full documentation on it: https://github.com/rails/rails-deprecated_sanitizer/blob/master/lib/rails/deprecated_sanitizer/html-scanner/html/selector.rb#L188-L199 |
|
Actually it says that the selector have to start with
|
|
Right. I'm starting to doubt if this would have passed on the old sanitizer. |
|
Try it on the old one. |
|
I can't get it to run because of more dependency issues... |
|
Test in a older version of Rails.
|
|
Yep, it works on an earlier Rails version. |
|
I know why this happens now. The substitution happens before checking what kind of equality checking we're doing. The substitution then swallows the extra arguments: https://github.com/rails/rails-dom-testing/blob/master/lib/rails/dom/testing/assertions/selector_assertions/html_selector.rb#L62 I'm not exactly what the correct way to handle this is. |
|
Maybe the fix would be as simple as only substitute values when they're argument is either a string or a regex. |
- Reorder methods so public interface is more clear - Make methods private - Don't allow substitute to be changed
Fix #9: Substitution context replaces `?` with false.
|
❤️ 💚 💙 💛 💜 |
Fixes #9.
Possible solution: scope substitutions, so only the question marks within
:match()get substituted.