-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Simplify current store selection #2041
Simplify current store selection #2041
Conversation
dc7e765
to
dd25531
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Maybe we could extract the previous behavior (mainly the header supporting one) into a second store finder class and still ship it with core, so existing stores can use the old one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be the best solution to me.
I didn't know multiple store urls could be set. If this feature is used can't we change the query using like
(via by_url
scope) and support multiline store urls?
Absolutely! Do you think it's reasonable for us to use this as the new default? It should be an improvement for most stores.
We could do that, and if we implement a "legacy" store selector class as @tvdeyen suggests, that is what it should do. That said, I'd rather remove that behaviour for this new implementation. The most glaring issue with the current implementation is that it will incorrectly match |
7009612
to
288d956
Compare
288d956
to
03b16d2
Compare
03b16d2
to
13fc5bf
Compare
Previously we selected the current store by pretty hacky means. We took the value of either the SPREE_STORE HTTP header or the domain name, and then selected a store either matching that code or had that value contained anywhere in its url attribute. Failing to find these it falls back to the default store. Using SPREE_STORE and finding by code is intended to be used with a reverse proxy like nginx, and is a good and powerful way to configure Solidus' multi domain. But it does require extra and very specific configuration, and isn't overly suitable as a default. Selecting by url which contains the (header or domain name) value is slow, surprising, and error-prone. This commit removes support for the SPREE_STORE header, and allows only one url per-store. Users needing this functionality can replicate it by implementing their own store selector class. This commit also allows the store selector to perform only a single query to find the current store.
13fc5bf
to
29aa494
Compare
Previously we selected the current store by pretty hacky means. We took the value of either the SPREE_STORE HTTP header or the domain name, and then selected a store either matching that code or had that value contained anywhere in its url attribute. Failing to find these it falls back to the default store.
Using SPREE_STORE and finding by code is intended to be used with a reverse proxy like nginx, and is a good and powerful way to configure Solidus' multi domain. But it does require extra and very specific
configuration, and isn't overly suitable as a default.
Selecting by url which contains the (header or domain name) value is slow, surprising, and error-prone (think having
example.com
andca.example.com
).This commit removes support for the SPREE_STORE header, and allows only one url per-store. Users needing this functionality can replicate it by implementing their own store selector class.
This commit also allows the store selector to perform only a single query to find the current store.
Before
After
This is a breaking change, it doesn't have to be, but I think the new behaviour is enough of an improvement that it's worth breaking (with a very clear notice in the CHANGELOG).
I'd like to hear from users running multi-domain stores:
SPREE_STORE
variable?url
attribute (spree_multi_domain used to recommend them newline-separated). If so we may want to add better support for this.Related to #1971, #2022
cc @kennyadsl