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

CRM-21568 - Move emptiness judgments from SettingsBag::setDb to InnoDBIndexer #11423

Merged
merged 1 commit into from
Dec 18, 2017

Conversation

totten
Copy link
Member

@totten totten commented Dec 16, 2017

Overview

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.

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-run testing, I did the following:

  • Switch local mysqld to v5.5. Rebuild. Navigate to "Search Preferences" and save (without making changes).
  • Switch local mysqld to v5.7. Rebuild. Navigate to "Search Preferences". Perform several cycles of
    alternately updating values and reloading form.

@totten
Copy link
Member Author

totten commented Dec 16, 2017

…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 totten changed the base branch from master to 4.7.29-rc December 16, 2017 20:50
@totten totten removed the master label Dec 16, 2017
@MegaphoneJon
Copy link
Contributor

MegaphoneJon commented Dec 16, 2017 via email

@eileenmcnaughton
Copy link
Contributor

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

@seamuslee001
Copy link
Contributor

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

@eileenmcnaughton
Copy link
Contributor

Ok - merging - all blame on @seamuslee001 because he encouraged me - but I think we want at least a day of 'rc'

@eileenmcnaughton eileenmcnaughton merged commit 6a0b4a5 into civicrm:4.7.29-rc Dec 18, 2017
@MegaphoneJon
Copy link
Contributor

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.

@eileenmcnaughton
Copy link
Contributor

great - thanks for the feedback

@totten totten deleted the master-onchange branch December 21, 2017 04:04
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants