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.

Sign up for GitHub

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

[Ref] Remove function parameter only used from test #20459

Merged
merged 1 commit into from
Jun 1, 2021
+16 −12
Merged

[Ref] Remove function parameter only used from test #20459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions CRM/Logging/Schema.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,10 +518,8 @@ private function _getColumnQuery($col, $createQuery) {

/**
* Fix schema differences.
*
* @param bool $rebuildTrigger
*/
public function fixSchemaDifferencesForAll($rebuildTrigger = FALSE) {
public function fixSchemaDifferencesForAll(): void {
$diffs = [];
$this->resetTableColumnsCache();

Expand All @@ -537,10 +535,6 @@ public function fixSchemaDifferencesForAll($rebuildTrigger = FALSE) {
foreach ($diffs as $table => $cols) {
$this->fixSchemaDifferencesFor($table, $cols);
}
if ($rebuildTrigger) {
// invoke the meta trigger creation call
CRM_Core_DAO::triggerRebuild(NULL, TRUE);
}
}

/**
Expand Down
20 changes: 15 additions & 5 deletions tests/phpunit/api/v3/LoggingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ protected function setUp(): void {

/**
* Clean up log tables.
*
* @throws \API_Exception
* @throws \CRM_Core_Exception
* @throws \CiviCRM_API3_Exception
*/
protected function tearDown(): void {
$this->quickCleanup(['civicrm_email', 'civicrm_address']);
Expand All @@ -41,8 +45,10 @@ protected function tearDown(): void {

/**
* Test that logging is successfully enabled and disabled.
*
* @throws \CRM_Core_Exception
*/
public function testEnableDisableLogging() {
public function testEnableDisableLogging(): void {
$this->assertEquals(0, $this->callAPISuccessGetValue('Setting', ['name' => 'logging']));
$this->assertLoggingEnabled(FALSE);

Expand Down Expand Up @@ -150,16 +156,20 @@ public function testUpdateLogTableHookINNODB() {

/**
* Check that if a field is added then the trigger is updated on refresh.
*
* @throws \CRM_Core_Exception
*/
public function testRebuildTriggerAfterSchemaChange() {
public function testRebuildTriggerAfterSchemaChange(): void {
$this->callAPISuccess('Setting', 'create', ['logging' => TRUE]);
$tables = ['civicrm_acl', 'civicrm_website'];
foreach ($tables as $table) {
CRM_Core_DAO::executeQuery("ALTER TABLE $table ADD column temp_col INT(10)");
}
unset(\Civi::$statics['CRM_Logging_Schema']);

$schema = new CRM_Logging_Schema();
$schema->fixSchemaDifferencesForAll(TRUE);
$schema->fixSchemaDifferencesForAll();
Civi::service('sql_triggers')->rebuild();
Copy link
Member

Choose a reason for hiding this comment

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

I think the first run of the test failed because it needs rebuild($tables=NULL, $force=TRUE) to be equivalent to the previous call to triggerRebuild($tables=NULL, $force=TRUE); If that works, then +1 for merging.

Alternatively - you might be able to swap the two lines with just a call to $schema->fixSchemaDifferences() or $schema->fixSchemaDifferences(TRUE), since that function basically does these two lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - I'm trying to get rid of that $force being passed in because I don't think we should be offering it as a parameter purely to support this test - might take a couple of rounds fight with jenkins to get there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - unsetting that static worked


foreach ($tables as $table) {
$this->assertTrue($this->checkColumnExistsInTable('log_' . $table, 'temp_col'), 'log_' . $table . ' has temp_col');
Expand All @@ -168,8 +178,8 @@ public function testRebuildTriggerAfterSchemaChange() {
$this->assertStringContainsString('temp_col', $dao->Statement);
}
}
CRM_Core_DAO::executeQuery("ALTER TABLE civicrm_acl DROP column temp_col");
CRM_Core_DAO::executeQuery("ALTER TABLE civicrm_website DROP column temp_col");
CRM_Core_DAO::executeQuery('ALTER TABLE civicrm_acl DROP column temp_col');
CRM_Core_DAO::executeQuery('ALTER TABLE civicrm_website DROP column temp_col');
}

/**
Expand Down