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

Make api get upgrade-safe #17729

Merged
merged 2 commits into from
Jul 13, 2020
Merged

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jul 1, 2020

Overview

By default, the api returns all available fields when doing a get. This can cause db errors when there are pending upgrades and a field exists in code but not yet in the db.

This mitigates the problem by only allowing the api to "see" fields that are appropriate to the current version of the db schema.

Before

Random crashes when db upgrades are pending.

After

Hopefully less crashy.

@civibot civibot bot added the master label Jul 1, 2020
@civibot
Copy link

civibot bot commented Jul 1, 2020

(Standard links)

@@ -963,4 +963,3 @@ INSERT INTO civicrm_navigation
VALUES
( @domainID, CONCAT('civicrm/report/instance/', @instanceID,'&reset=1'), 'Mailing Detail Report', 'Mailing Detail Report', 'administer CiviMail', 'OR', @reportlastID, '1', NULL, @instanceID+2 );
UPDATE civicrm_report_instance SET navigation_id = LAST_INSERT_ID() WHERE id = @instanceID;
UPDATE civicrm_domain SET version = '4.6.alpha1';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this file was full of trailing whitespace; the only real change is to remove this ugly hack which was causing all tests to run with a very very wrong domain version. Honestly I don't understand how our tests were passing at all with this problem (note that the purpose of this file is ostensibly to mock a second domain but this query clobbers the version of the main domain too. OOPS!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooof, that's fun. I suppose technically it didn't matter before. 🤷‍♂️

I see this patch changes the version from 4.2 to 4.6 I've posted an alternative #17734 which should avoid the need for periodic tweaks.

@seamuslee001
Copy link
Contributor

test fails relate @colemanw

@totten
Copy link
Member

totten commented Jul 2, 2020

Since the test failures seem relevant, this needs some kind of revision and isn't likely get into the RC. So I've extracted some smaller bits (#17734, #17735) in hopes that we can get those in sooner.

@colemanw
Copy link
Member Author

colemanw commented Jul 2, 2020

It's actually just one test class all with the same failure so shouln't be too hard to fix. I'll try in the morning.

Switches api v3 and v4 to use that method so they are upgrade-safe by default.
The settingTest class was being too aggressive about creating and deleting domains,
this teaches it to not delete pre-existing domains.
Also marks an old unused api function deprecated.
@colemanw
Copy link
Member Author

colemanw commented Jul 3, 2020

@totten @seamuslee001 It passes now :)

@eileenmcnaughton
Copy link
Contributor

OK this seems good now tests are passing - this change hits one zillion five hundred and two tests so if they pass I feel good with it

@eileenmcnaughton eileenmcnaughton merged commit 2aeddfe into civicrm:master Jul 13, 2020
@colemanw colemanw deleted the upgradeSafeApi branch July 13, 2020 23:59
colemanw added a commit to colemanw/civicrm-core that referenced this pull request Jul 14, 2020
colemanw added a commit to colemanw/civicrm-core that referenced this pull request Jul 14, 2020
@colemanw
Copy link
Member Author

Cool. I've followed up with #17823

seamuslee001 added a commit that referenced this pull request Jul 16, 2020
Remove unnecessary try/catch per #17729
christianwach pushed a commit to christianwach/civicrm-core that referenced this pull request Jul 18, 2020
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.

4 participants