solidusio / solidus

🛒 Solidus, the open-source eCommerce framework for industry trailblazers.
https://solidus.io
Other
5.02k stars 1.29k forks source link

Context for a proc given as a preference default changed from v3.0 to v3.1 #4720

Open waiting-for-dev opened 1 year ago

waiting-for-dev commented 1 year ago

Before v3.1, the proc given as the default value in a preference had its context bound to the initialized instance. That allowed, for example, referencing other preferences. From v3.1., the context is bound to the class.

Solidus Version: v3.2, v3.1, v3.0

To Reproduce

On v3.0 you can do the following:

Spree::Calculator::FlatRate.preference(:foo, :string, default: -> { preferred_currency })
a = Spree::Calculator::FlatRate.new
a.preferred_foo #=> "USD"

However, on Solidus v3.1:

Spree::Calculator::FlatRate.preference(:foo, :string, default: -> { preferred_currency })
a = Spree::Calculator::FlatRate.new
# undefined local variable or method `preferred_currency' for main:Object (NameError)

Current behavior The proc is bound to the class context, so a NameError is raised because the generated preferred_ methods are not found.

Expected behavior It should keep working as in v3.0.

Additional context First reported on Slack: https://solidusio.slack.com/archives/C0JBKDF35/p1668417688273419

Roddoric commented 1 year ago

I did a git bisect with the example above and found the commit introducing the behavior change https://github.com/solidusio/solidus/commit/b897eccb6cdd8235d782275e9df069d95474cd6b

I'll try yo look into it has I need it fixed to update our Solidus version

waiting-for-dev commented 1 year ago

I also suspected of that commit, but looking to the diff I don't see how it could break that 🤷 Thanks for looking into it, I'm curious about what's the source of the problem here 🙂

Roman2K commented 1 year ago

The culprit does indeed seem to be the commit found by @Roddoric.

It changes the context of execution of the default block when defining the default getter method:

Before that commit, the default block was passed as method body, so its execution context was the "preferable" instance:

define_method preference_default_getter_method(name), &default

After that commit, the default block is merely called from another method body, so its execution context is wherever it was created at (before being passed to preference(...)):

define_method preference_default_getter_method(name) do
  default.call(*context_for_default)
end

I see 2 solutions (not tested :nerd_face:):

1. instance_exec

define_method preference_default_getter_method(name) do
  instance_exec(*context_for_default, &default)
end

2. define_method

Way more verbose:

define_method preference_default_getter_method(name) do
  send(preference_original_default_getter_method(name), *context_for_default)
end

define_method preference_original_default_getter_method(name), &default

# ...

def preference_original_default_getter_method(name)
  "preferred_#{name}_original_default".to_sym
end
waiting-for-dev commented 1 year ago

🤦 Good caught! Feel free to submit a PR fixing it. I think 1 would be ok for now.