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

Improve compatibility with frozen string literal #549

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Throne3d
Copy link

@Throne3d Throne3d commented Dec 23, 2024

When ActionView::Template.frozen_string_literal is set to true (e.g. via Rails.config.action_view.frozen_string_literal), we get errors while rendering exception_notifier templates inside PP.pp on Ruby 3.3.6:

   ActionView::Template::Error: can't modify frozen String: ""

  /app/vendor/ruby-3.3.6/lib/ruby/3.3.0/prettyprint.rb:184:in `text'
  /app/vendor/ruby-3.3.6/lib/ruby/3.3.0/prettyprint.rb:252:in `group'
  /app/vendor/ruby-3.3.6/lib/ruby/3.3.0/pp.rb:201:in `pp'
  /app/vendor/ruby-3.3.6/lib/ruby/3.3.0/pp.rb:97:in `block in pp'
  /app/vendor/ruby-3.3.6/lib/ruby/3.3.0/pp.rb:158:in `guard_inspect_key'
  /app/vendor/ruby-3.3.6/lib/ruby/3.3.0/pp.rb:97:in `pp'
  /app/vendor/bundle/ruby/3.3.0/gems/exception_notification-4.5.0/lib/exception_notifier/views/exception_notifier/_session.text.erb:2:in `_vendor_bundle_ruby_______gems_exception_notification_______lib_exception_notifier_views_exception_notifier__session_text_erb__2544461215938986440_61040'

The emails still come through, but the session component is replaced with this exception instead. I've seen other libraries avoid this for strings that need to be mutable by replacing "" with +"", to ensure it's a copy of the literal and therefore mutable.

I'm not entirely sure this entirely solves the problem - I wasn't able to properly run the exception_notification tests locally - but it should help out! In my application, this fixes the session section, but I haven't tested data thoroughly.

I understand Ruby 3.4 will make this configuration the default, so this may become more pressing shortly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant