Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add runtime configuration class #11965

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

rafalcymerys
Copy link
Member

Currently, Spree::Config, Spree::Backend::Config etc. use Spree::Preferences::Configuration class as a base. The problem with Spree::Preferences::Configuration is that it persists every preference in the database, so that they can be e.g. changed via the UI in the admin panel.

This causes some major issues with the initialization process. As an example, Spree::Backend defines a preference called admin_path that's used to define the root for its routes. The value needs to be known at boot time, and cannot be dynamically changed. Similar case with Spree::Auth::Devise, where certain Devise features are enabled at boot time, based on configuration.

Because of that, we should introduce an additional type of Configuration - RuntimeConfiguration, that's not persisted and that should be only defined in an initializer. This will allow to run the configuration step before after_initialize, and will work regardless of whether the database connection is ready.

This PR implements a draft of such solution that could be later reused in other gems and extensions.

Copy link

viezly bot commented Nov 17, 2023

Changes preview:

Legend:

👀 Review pull request on Viezly

Copy link
Contributor

@mateus-po mateus-po left a comment

Choose a reason for hiding this comment

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

Looks great:))

Copy link
Contributor

@tomdonarski tomdonarski left a comment

Choose a reason for hiding this comment

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

Good call with being independent from db connection.
I'd need to have yet another sit on this class, as it's a bit hard to comprehend (the test helps to get the gist of it, so thank you for adding that).
On the other hand boot- and config-related things are kind of special ad not changed that often, so that extra bit of complexity is understandable.

@rafalcymerys rafalcymerys marked this pull request as ready for review November 24, 2023 14:31
@rafalcymerys rafalcymerys merged commit e5caabe into main Nov 27, 2023
12 checks passed
@delete-merged-branch delete-merged-branch bot deleted the feature/add-runtime-configuration-class branch November 27, 2023 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants