spohlenz / tinymce-rails

Integration of TinyMCE with the Rails asset pipeline
Other
815 stars 256 forks source link

allow use aliases for YAML files by default #297

Closed loqimean closed 1 year ago

loqimean commented 1 year ago

In ruby 3.1 (with Psycho 4) was breaking change that removes default aliases support, but the Rails team provides solution for this issue and unfortunately there wasn't it, but I add and of course tested it, there is the Stack Overflow response and issue:

https://stackoverflow.com/a/71192990/17425539

and there is the Rails team patch:

https://github.com/rails/rails/commit/179d0a1f474ada02e0030ac3bd062fc653765dbe

spohlenz commented 1 year ago

Have you been able to reproduce this as a problem in tinymce-rails? We use YAML.unsafe_load (since we assume tinymce.yml is trusted) rather than YAML.load as in the Rails patch, which from what I can tell supports aliases by default (https://github.com/ruby/psych/discussions/571#discussioncomment-3345427).

I've tried a few different Ruby/Psych versions and haven't been able to spot any issues.

loqimean commented 1 year ago

Have you been able to reproduce this as a problem in tinymce-rails? We use YAML.unsafe_load (since we assume tinymce.yml is trusted) rather than YAML.load as in the Rails patch, which from what I can tell supports aliases by default (ruby/psych#571 (comment)).

I've tried a few different Ruby/Psych versions and haven't been able to spot any issues.

So, look, too many times I tried to use aliases and after too many time found, that by default in NEW ryby 3.1 (which I use) as I said was used Psycho 4, which requires to specify aliases accepting.

So, now I must use fork of this gem with update, that was added to resolve this issue, not by me, but by Rails team and only after this change in my fork, I can finally use aliases, with nowadays version of this gem I CANNOT

spohlenz commented 1 year ago

Thank you @loqimean. Could you please confirm the exact versions of Ruby/Psych you are using?

3.1.3 :003 > RUBY_VERSION
 => "3.1.3" 
3.1.3 :004 > Psych::VERSION
 => "4.0.4" 
loqimean commented 1 year ago

Just rails 7.0.4 adn ruby 3.1.1 where was added Psycho 4, links are in issue body

loqimean commented 1 year ago
Psych::VERSION
3.1.1 :001 > RUBY_VERSION
 => "3.1.1" 
3.1.1 :002 > Psych::VERSION
 => "4.0.3" 
spohlenz commented 1 year ago

I'm a little confused by the behavior you're seeing. I actually think your code is falling through to the rescue ArgumentError block anyway. See the following example with .load vs .unsafe_load:

3.1.1 :009 > RUBY_VERSION
 => "3.1.1" 
3.1.1 :010 > Psych::VERSION
 => "4.0.3" 
config = <<-YAML
base: &base
  foo: bar

extended:
  <<: *base
  baz: 123
YAML

3.1.1 :020 > YAML.unsafe_load(config)
 => {"base"=>{"foo"=>"bar"}, "extended"=>{"foo"=>"bar", "baz"=>123}} 

3.1.1 :023 > YAML.unsafe_load(config, aliases: true)
/Users/sam/.rvm/rubies/ruby-3.1.1/lib/ruby/3.1.0/psych.rb:271:in `unsafe_load': unknown keyword: :aliases (ArgumentError)

3.1.1 :021 > YAML.safe_load(config)
/Users/sam/.rvm/rubies/ruby-3.1.1/lib/ruby/3.1.0/psych/visitors/to_ruby.rb:430:in `visit_Psych_Nodes_Alias': Unknown alias: base (Psych::BadAlias)

3.1.1 :022 > YAML.safe_load(config, aliases: true)
 => {"base"=>{"foo"=>"bar"}, "extended"=>{"foo"=>"bar", "baz"=>123}}

The existing config loading code in tinymce-rails uses YAML.unsafe_load so this should support aliases already and passing in the aliases: argument causes the ArgumentError.