CRM-21568 - Move emptiness judgments from SettingsBag::setDb to InnoDBIndexer #11423
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…BIndexer The `SettingsBag::setDb()` function calls any `on_change` listeners. It originally used "dumb on change" behavior (where it calls the listeners without comparing values). CRM-19610 had an issue where the `InnoDBIndexer` was running a bit too often, so they tried to resolve it by making the `SettingsBag::setDb()` more clever. Unfortunately, that's been a bit bouncy, and the cleverness depends on one's particular interpretation of 0 vs '0' vs '' vs NULL vs FALSE. Before ------ All on-change listeners may be skipped if there's particular mix of emptiness in the old/new values. After ----- The on-change listeners always fire. However, the specific listener involved with CRM-19610 will now ignore some insignificant changes. Related discussion: civicrm#11417 ---------------------------------------- * CRM-21568: https://issues.civicrm.org/jira/browse/CRM-21568 * CRM-19610: https://issues.civicrm.org/jira/browse/CRM-19610
totten
force-pushed
the
master-onchange
branch
from
9f0441b
to
38e5457
Compare
|
I like this approach better @totten - but the way you tested it isn't addressing CRM-21568 if you didn't intentionally reduce the permissions of your MySQL user. Might want to ask Mattermost user "ben" to test this. I can try and test this next week, but since it was something I did for a random chat user, I might not be able to prioritize it.
|
|
I'm a bit worried about having this un-merged if it's going out on Wed - I would be inclined to merge this & through to master asap so we are not going out in 2 days with something not merged until the day |
|
My feeling is that this will fix the issue for the fts stuff and i would be in favor of merging it as it seems to be a much more stable fix than previous |
|
Ok - merging - all blame on @seamuslee001 because he encouraged me - but I think we want at least a day of 'rc' |
|
FWIW I asked Ben ("ben" on Mattermost) to test this against his environment (which exhibits the bugged behavior) and he reports that this fixes his problems with no noticeable side effects. |
|
great - thanks for the feedback |
sluc23
pushed a commit
to ixiam/civicrm-core
that referenced
this pull request
Jan 10, 2018
CRM-21568 - Move emptiness judgments from SettingsBag::setDb to InnoDBIndexer
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
The
SettingsBag::setDb()function calls anyon_changelisteners. Itoriginally used "dumb on change" behavior (where it calls the listeners
without comparing values). CRM-19610 had an issue where the
InnoDBIndexerwas running a bit too often, so they tried to resolve it by
making the
SettingsBag::setDb()more clever.Unfortunately, that's been a bit bouncy, and the cleverness depends on one's
particular interpretation of 0 vs '0' vs '' vs NULL vs FALSE.
Related discussion: #11417
Before
All on-change listeners may be skipped if there's particular mix of emptiness
in the old/new values.
After
The on-change listeners always fire. However, the specific listener
involved with CRM-19610 will now ignore some insignificant changes.
Comments
For
r-runtesting, I did the following:alternately updating values and reloading form.