Use #constantize for configurable class lists #1203
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhawthorn
force-pushed
the
class_constantizer
branch
from
a80ca68 to
035afd6
Compare
This class is responsible for holding as set of references to classes (by their name). This is done instead of just having the classes to avoid loading the classes too early as well as to allow code reloading of these classes.
This avoids loading the classes sooner than needed. This should fix issues where, for example, PaymentMethod is loaded before acts_as_list has its initializer run.
Previously promotion rules were added to the configuration after all initialization to avoid issues. Now we are able to add the class names at the normal time. This will also allow a store to remove a rule if desired.
jhawthorn
force-pushed
the
class_constantizer
branch
from
035afd6 to
e329a56
Compare
|
I think this is a great change in the right direction. It probably deserves a place in the Changelog. |
|
I caught the live show of this and am inherently 👍. Not just because it is @jhawthorn wandering around fixing problems I am having, but also because if its @jhawthorn wandering around actively fixing my mistakes. |
|
👍 |
|
Is this ready to merge? Does it close #1192 ? |
This was referenced Jun 2, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In solidus, we have a
Spree::Core::Environmentclass. This class is what's exposed asconfig.spreein initializers and holds mostly configuration on available classes:shipping_methods,payment_methods,promotion_rules, etc.This has caused some weird issues for quite a while since these classes are put into an array at compile time. If any of these classes are reloaded (as they will be in development) the configuration will hold an out of date version. #899 is an example of an issue arising from this.
This PR adds a
ClassConstantizer::Setcollection to hold these lists of classes. It works byto_sing anything being inserted into the list, andconstantizeing them when the list is iterated over. This should be backwards compatible for common ways to insert .This also solves issues caused by autoloading classes too early (#1192)
For the same reasons as this activerecord relations hold
class_nameinstead of just a class. Our UserClassHandle also serves a similar purpose.