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

Refactor: Centralise escaping and unescaping urls / URI.escape obsolete warning on Ruby 2.7+ #217

Conversation

syphax-bouazzouni
Copy link
Contributor

@syphax-bouazzouni syphax-bouazzouni commented Sep 8, 2022

Issues

  • URI.escape is obsolete on Ruby 2.7+
  • In some places, we use URI.escape, and on other CGI.escape
  • in application helpers, we have two methods that do the same escape(string) and encode_param(string)

Changes

This PR, fix those issues, by centralizing the escaping in only one function called escape(url) that uses CGI.escape. Then reuse it everywhere (same for unescaping).

@syphax-bouazzouni syphax-bouazzouni changed the title Refactor: Centralise escaping unescaping urls / URI.escape obsolete warning on Ruby 2.7+ Refactor: Centralise escaping and unescaping urls / URI.escape obsolete warning on Ruby 2.7+ Sep 8, 2022
@jvendetti
Copy link
Member

Thank you very much for this pull request and suggested refactor to resolve URI.escape obsoletion warnings. We discussed internally and chose to go with a different fix. Some of the warnings were resolved through usage of Rails path helpers (287788e), and others were resolved by removing defunct marginal notes views (7149982). Removal of the marginal notes views also allowed for deletion of the escape, encode_param, and to_param methods. We didn't see the need to continue wrapping CGI.escape in an escape method since it's still part of Ruby's standard library, and feels like an unnecessary level of indirection.

@jvendetti jvendetti closed this Jan 26, 2023
@syphax-bouazzouni
Copy link
Contributor Author

Hi, OK great.

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.

2 participants