Skip to content

Commit

Permalink
Fix compatibility with Ruby 3.1' Psych version
Browse files Browse the repository at this point in the history 
The new Psych version required by Ruby 3.1 no longer allows the loading
of arbitrary classes into YAML by default. We switch to `#safe_load` and
enable the class we're currently using.

See:

ruby/psych@cb50aa8
ruby/psych@1764942

[skip ci]
  • Loading branch information
@waiting-for-dev
waiting-for-dev committed May 3, 2022
1 parent db88911 commit 008168c
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion core/app/models/spree/log_entry.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class LogEntry < Spree::Base
belongs_to :source, polymorphic: true, optional: true

def parsed_details
@details ||= YAML.load(details)
@details ||= YAML.safe_load(details, permitted_classes: [ActiveMerchant::Billing::Response])

This comment has been minimized.

Sign in to view

Copy link
@DanielePalombo

DanielePalombo May 12, 2022 •
edited
Loading

Contributor

Hi @waiting-for-dev ,
Is this backward with ruby < 3.1? I've got an error with this part using ruby 2.7.6

Psych::DisallowedClass in Spree::Admin::Payments#show

ActionView::Template::Error (Tried to load unspecified class: ActiveSupport::TimeWithZone):
    13:   </thead>
    14:   <tbody>
    15:   <% log_entries.each do |entry| %>
    16:     <tr class="log_entry <%= entry.parsed_details.success? ? 'success' : 'fail' %>">
    17:       <td>
    18:         <%= pretty_time(entry.created_at) %>
    19:       </td>

rbenv/versions/2.7.2/lib/ruby/2.7.0/psych/class_loader.rb:97:in `find'

This comment has been minimized.

Sign in to view

Copy link
@jarednorman

jarednorman May 12, 2022

Member

Found a similar issue over here that suggests we may need to use unsafe_load? sidekiq/sidekiq#5064

This comment has been minimized.

Sign in to view

Copy link
@waiting-for-dev

waiting-for-dev May 16, 2022

Author Contributor

unsafe_load is a new method on Psych, so using it would make it incompatible with older rubies. The issue arises when an unknown class is given to #safe_load. I'm not familiar with this part of the codebase, so I just added the one that made the test suite fail. Can those serialized classes be different depending on the user, or can we have a fixed allowed list that will work for all Solidus installations? (for instance, the issue here will be fixed if we explicitly add ActiveSupport::TimeWithZone to permited_classes:). cc @kennyadsl

This comment has been minimized.

Sign in to view

Copy link
@kennyadsl

kennyadsl May 16, 2022

Member

I don't know which classes can be serialized but I guess we should be as flexible as possible. Any customization would require overriding that method otherwise. I think we can add ActiveSupport::TimeWithZone and any other occurrence that we find in this period until the release but still allow people to change that array's values easily with some interface.

This comment has been minimized.

Sign in to view

Copy link
@waiting-for-dev

waiting-for-dev May 16, 2022

Author Contributor

It makes sense. We can add a preference for that. What do you think?

This comment has been minimized.

Sign in to view

Copy link
@kennyadsl

kennyadsl May 16, 2022

Member

Yes, even if, changing the preference in a store will make it very hard to get any other update we will release in core. What about an "environment" preference like this one? This will allow to append other values in extensions and store and still benefits from any update.

This comment has been minimized.

Sign in to view

Copy link
@waiting-for-dev

waiting-for-dev May 16, 2022

Author Contributor

Oh, I see. However, we could use a regular Spree::AppConfiguration.preference as long as we append instead of replacing a core and immutable list.

This comment has been minimized.

Sign in to view

Copy link
@kennyadsl

kennyadsl May 16, 2022

Member

Can't see why someone would want to remove a class from the array so 👍.

This comment has been minimized.

Sign in to view

Copy link
@waiting-for-dev

waiting-for-dev May 17, 2022

Author Contributor

Great! Please, see #4376.
end
end
end

0 comments on commit 008168c

Please sign in to comment.