Remove stringex as a dependency of core #2383
Conversation
core/lib/spree/app_configuration.rb
Outdated
| @@ -363,6 +363,15 @@ def payment_canceller | |||
| # Spree::CurrentStoreSelector | |||
| class_name_attribute :current_store_selector_class, default: 'Spree::StoreSelector::ByServerName' | |||
|
|
|||
|
|
|||
core/app/models/spree/taxon.rb
Outdated
| permalink_tail ||= name.to_url | ||
| self.permalink_part = permalink_tail | ||
| permalink_tail ||= Spree::Config.string_inflection_class.parameterize(name) | ||
| self.permalink_part = Spree::Config.string_inflection_class.dasherize(permalink_tail) |
There was a problem hiding this comment.
It's not clear to me why we are using two different methods here. Previously we only used one.
There was a problem hiding this comment.
stringex would "dasherize" everything as part of its to_url method. ActiveSupport::Inflector doesn't do that, so some specs related to this would fail (t2_child is expected to be convereted to t2-child, which ActiveSupport#parameterize alone does not do).
There was a problem hiding this comment.
I'd say delete the specs... but thats just me.
There was a problem hiding this comment.
We can either create a new class which calls parameterize then dasherize, or we can just use parameterize.
Since we're already changing behaviour here I think it is fine to just use parameterize and update the specs to pass.
core/lib/spree/app_configuration.rb
Outdated
| # @!attribute [rw] string_inflection_class | ||
| # @return [Class] a class that provides `#parameterize`, and `#dasherize` | ||
| # methods that both return Strings | ||
| class_name_attribute :string_inflection_class, default: 'ActiveSupport::Inflector' |
There was a problem hiding this comment.
This needs a name more clearly scoped to what it does. We (through rials) use ActiveSupport::Inflector heavily for other things and this name shouldn't imply that we are overriding that. It should be clear that this is for URL parameterization only.
The question here is do we intend to use this for. If we intend to only use this class from Taxon, it should be called taxon_parametizer_class (or maybe taxon_url_parametizer_class). If we intend to move everything else over, we could call it parametizer_class (or maybe url_parametizer_class).
It's worth noting that has some of its own logic and configuration for parameterization (though I believe it relies on
Inflector.parameterize). I'm not sure we would want to lose that logic, but it would be nice if we generated URLs consistently.
We can use this in `Taxon#set_permalink` to create valid url encoded strings.
We already have ActiveSupport as part of Solidus, and stringex doesn't do much else that ActiveSupport doesn't already do. We lose support for translating non-ascii characters, but `ActiveSupport#parameterize` does a pretty good job of replacing special characters for ones that can be used in a valid URL. This commit also updates some specs. `#parameterize` does not "dasherize" the string like stringex used to.
We can use ActiveSupport instead. `Spree::Config.taxon_url_parametizer_class` defaults to being `ActiveSupport::Inflector`. This can be customized.
We already have ActiveSupport as part of Solidus, and stringex doesn't
do much else that ActiveSupport doesn't already do. We lose support for
translating non-ascii characters, but
ActiveSupport#parameterizedoesa pretty good job of replacing special characters for ones that can be
used in a valid URL.
This PR also introduces a pluggable
taxon_url_parametizer_classthat defaults toActiveSupport::Inflector.