Skip to content

Enable load symbol class - #473

Closed
riouruma wants to merge 6 commits into
hashie:masterfrom
blabo:feature/enable_load_symbol
Closed

Enable load symbol class#473
riouruma wants to merge 6 commits into
hashie:masterfrom
blabo:feature/enable_load_symbol

Conversation

@riouruma

@riouruma riouruma commented Mar 18, 2019 •
edited
Loading

Copy link
Copy Markdown

Problem

I got an error when rails server started.

from /Users/rails/project/vendor/bundle/ruby/2.6.0/bundler/gems/hashie/lib/hashie/mash.rb:110:in `load'
from /Users/rails/project/vendor/bundle/ruby/2.6.0/bundler/gems/hashie/lib/hashie/extensions/parsers/yaml_erb_parser.rb:21:in `perform'
from /Users/rails/project/vendor/bundle/ruby/2.6.0/bundler/gems/hashie/lib/hashie/extensions/parsers/yaml_erb_parser.rb:17:in `perform'
・・・
/Users/.rbenv/versions/2.6.2/lib/ruby/2.6.0/psych/class_loader.rb:97:in `find': Tried to load unspecified class: Symbol (Psych::DisallowedClass)

We need to allow Symbol class on YAML.safe_load .

Related issue

#458

@dblock

dblock commented Mar 18, 2019

Copy link
Copy Markdown
Member

This needs at least tests, please.

@riouruma

riouruma commented Mar 19, 2019

Copy link
Copy Markdown
Author

@dblock
Sorry. added the test.

@dblock

dblock commented Mar 19, 2019

Copy link
Copy Markdown
Member

Also CHANGELOG please, see bot note above.

@dblock

dblock commented Mar 19, 2019

Copy link
Copy Markdown
Member

Could you explain to us like 2 yo why this change is right? Should we be adding other classes? Exposing parameters to this? Something else?

@riouruma

riouruma commented Mar 20, 2019

Copy link
Copy Markdown
Author

https://docs.ruby-lang.org/en/2.6.0/Psych.html#method-c-safe_load
safe_load only allows the following classes by default, so I added Symbol Class.

  • TrueClass
  • FalseClass
  • NilClass
  • Numeric
  • String
  • Array
  • Hash

Should we be adding other classes?

As I needed it for my project, I can't think of any other classes I need now.

Exposing parameters to this?

Change to call a method instead of a direct params description.

@dblock

dblock commented Mar 20, 2019

Copy link
Copy Markdown
Member

Sorry to be a pest, I think we also need:

@dblock

dblock commented Mar 20, 2019

Copy link
Copy Markdown
Member

Also @riouruma thanks for hanging in here with my requests :) really appreciate your patience and hard work.

@riouruma

riouruma commented Mar 22, 2019

Copy link
Copy Markdown
Author

I added descriptions in README and UPGRADING.
Thanks, too :)

@dblock dblock mentioned this pull request Mar 22, 2019
Merged
@dblock

dblock commented Mar 22, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

@riouruma: I've replaced this with a different implementation in #474, which is backwards compatible and allows everything you're trying to do as well as further options if/when needed without monkey patching Hashie code. Please take a look and comment there.

@dblock

dblock commented Mar 22, 2019

Copy link
Copy Markdown
Member

Closing via #474

@dblock dblock closed this Mar 22, 2019
@tenderlove tenderlove mentioned this pull request May 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@riouruma @dblock