-
Notifications
You must be signed in to change notification settings - Fork 66
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
Don't make query if not needed #558
Conversation
setByIDList will make a query even though there are no DBHTMLText for this DataObject Checking the presence of these fields allow to return early and prevent the needless query
@michalkleiner any other wishes for this PR ? |
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.
I think this looks fine. What does @GuySartorelli think?
Arguably this is an enhancement and shouldn't be targeting CMS 4 - but given the very low risk of regression it's probably fine. |
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.
Looks good, thanks
@GuySartorelli do you also apply this to the ss5 version or do i need to make a pr for that as well? |
@lekoala it'll get merged up, eventually. We're working on getting merge ups to be automated but for now they're just done ad hoc. If you need this immediately let me know and I'll make a point to do merge ups sooner rather than later. |
@GuySartorelli i'm in the upgrade process to ss5 for my main client and that should go to production at the end of the summer, so if it's in a month or so that works for me :-) |
setByIDList will make a query even though there are no DBHTMLText for this DataObject
Checking the presence of these fields allow to return early and prevent the needless query
Fixes #557