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

Throw exception if site store isnt found in SiteBasedRequestResolver #2209

Merged

Conversation

pascalheidmann
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets #...

If you have a custom resolver with a priority lower than SiteBasedRequestResolver and are inside a request flagged as site request it will never come so far as it will return null instead of erroring

@CLAassistant
Copy link

CLAassistant commented Feb 10, 2023

CLA assistant check
All committers have signed the CLA.

@dpfaffenbauer dpfaffenbauer self-requested a review February 11, 2023 13:01
@dpfaffenbauer dpfaffenbauer self-assigned this Feb 11, 2023
@dpfaffenbauer
Copy link
Member

I am not really sure about potential BC breaks, maybe others suspect that the default store is returned when there is no store for the site

@pascalheidmann
Copy link
Contributor Author

@dpfaffenbauer returning the default store would be also a valid solution for me (which errors if none found). But at least it should not fail silently/break the resolver which expects a valid store and not null

@dpfaffenbauer
Copy link
Member

@pascalheidmann I think we should do it. At least within 3.x

@dpfaffenbauer dpfaffenbauer merged commit 4b04f76 into coreshop:master Feb 16, 2023
@dpfaffenbauer
Copy link
Member

thanks @pascalheidmann

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants