Skip to content
This repository was archived by the owner on Apr 14, 2023. It is now read-only.

Do not safeguard including of Spree::Preferences::Persistable #316

Merged

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Mar 4, 2022

The safeguard prevents this module to be included because the
class responds to preference (it is included in Spree::Base).

My guess is that this safeguard was included to avoid including
Spree::Preferences::Preferable twice (it is included by Spree::Preferences::Persistable)

But Ruby will take care of not including the same module twice.

This will not effect user of Solidus >= 3.0, but helps to get rid of a (rather large) deprecation warning in Solidus 2.11

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so sure about Ruby not including things twice:

[9] pry(main)> module Foo
[9] pry(main)*   def self.included(klass)
[9] pry(main)*     puts "INCLUDED"
[9] pry(main)*   end  
[9] pry(main)* end  
=> :included
[10] pry(main)> include Foo
INCLUDED
=> Object
[11] pry(main)> include Foo
INCLUDED
=> Object
[12] pry(main)> 

@tvdeyen
Copy link
Member Author

tvdeyen commented Mar 4, 2022

Not so sure about Ruby not including things twice:

[9] pry(main)> module Foo
[9] pry(main)*   def self.included(klass)
[9] pry(main)*     puts "INCLUDED"
[9] pry(main)*   end  
[9] pry(main)* end  
=> :included
[10] pry(main)> include Foo
INCLUDED
=> Object
[11] pry(main)> include Foo
INCLUDED
=> Object
[12] pry(main)> 

Ok, maybe I did not phrased it correctly. Ruby will not append the same methods to the same class twice. Also this is a ActiveSupport::Concern and this will take care of other potential side effects of the include.

The safeguard prevents this module to be included because the
class responds to preference (it is included in Spree::Base).

My guess is that this safeguard was included to avoid including
Spree::Preferences::Preferable twice (it is included by Spree::Preferences::Persistable)

But Ruby will take care of not including the same module twice.
@tvdeyen tvdeyen force-pushed the fix-configuration-persistable-include branch from 4eb5f39 to e18442e Compare March 4, 2022 17:01
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't know why this is here, I think we can remove it.

@@ -11,9 +11,7 @@ class Configuration < ::Spree::Base
messaging: { availables: %w[true false], default: 'false' }
}.freeze

unless respond_to?(:preference)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MinasMazar any chance you recall why we added this check here? 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kennyadsl I'm sorry, but I'm not aware right now. I think we can remove it.

@stale
Copy link

stale bot commented Nov 11, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 11, 2022
@kennyadsl kennyadsl added bug and removed wontfix labels Nov 11, 2022
@elia elia added this to the v1.2 milestone Dec 6, 2022
@elia elia merged commit bbb6fcb into solidusio:master Dec 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants