-
Notifications
You must be signed in to change notification settings - Fork 824
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
FIX Don't make queries if no custom config is set #11591
Conversation
Note that this isn't really fixing a bug, but since it's a small change with very minimal risk I'll allow it to go in as a patch. |
if ($editorConfigCount === 0) { | ||
return 'cms'; | ||
} |
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.
Why do we default to "cms" in this scenario? If there's no available config maps what would we expect returning "cms" to do here?
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.
because that's the current default if no value is found ?
this method always return something, "cms" being the default.
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.
Is the default defined somewhere? A config?
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.
It's hardcoded currently
silverstripe-framework/src/Security/Member.php
Line 1721 in 9d1741c
return $currentName ? $currentName : 'cms'; |
i kept it the way it was :-)
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.
That makes sense - that part of code was hidden in the diff and I didn't think to expand to see further down.
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.
LGTM, thanks for submitting this. Nice tidy enhancement.
if ($editorConfigCount === 1) { | ||
return key($editorConfigMap); | ||
} | ||
|
||
foreach ($this->Groups() as $group) { |
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.
If we really wanted to be fancy, we could add a filter here `$this->Groups()->filter(['HtmlEditorConfig:not' => null]) - but the benefit of that would be super low so not worth holding up merging just for that. If you're keen on that I'd be happen to merge a separate PR for that change on its own.
Description
Fixes unecessary queries, see issue for context
Manual testing steps
Check that no extra query is made on a fresh install
Issues
#11590
Pull request checklist