-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
Make api get upgrade-safe #17729
Conversation
(Standard links)
|
sql/test_data_second_domain.mysql
Outdated
@@ -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'; |
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.
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!)
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.
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.
test fails relate @colemanw |
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.
@totten @seamuslee001 It passes now :) |
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 |
Cool. I've followed up with #17823 |
Remove unnecessary try/catch per #17729
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.