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-18464 use safe drop foreign key function to resolve upgrade error… #9679

Merged
merged 2 commits into from
Feb 9, 2017

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Jan 14, 2017

… and also drop Index with same name as foreign key being dropped if it exists
https://issues.civicrm.org/jira/browse/CRM-18464

@@ -119,6 +119,7 @@ public function setPostUpgradeMessage(&$postUpgradeMessage, $rev) {
* @param string $rev
*/
public function upgrade_4_7_alpha1($rev) {
$this->addTask('Drop action schedule mapping foreign key', 'dropActionScheudleMappingForeignKey');
Copy link
Member

Choose a reason for hiding this comment

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

Scheudle

*
* @return bool
*/
public static function dropActionScheudleMappingForeignKey(CRM_Queue_TaskContext $ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

Overall, +1 for more guarded schema changes.

@totten
Copy link
Member

totten commented Jan 24, 2017

So I did a test run of 9ef9434 roughly as follows:

cd sites/all/modules/civicrm
git checkout master && git scan up && ./bin/setup.sh -g

civibuild ut dmaster 4.6.11*
mysqldump civi > /tmp/upgrade.orig

git scan am https://github.com/civicrm/civicrm-core/pull/9679 --new
civibuild ut dmaster 4.6.11*
mysqldump civi > /tmp/upgrade.new

colordiff -u /tmp/upgrade.{orig,new}

The schema ends up slightly different -- i.e. the old code has FK_civicrm_action_schedule_mapping_id in the final ouptut, but the new code does not. Is that the intended effect?

CREATE TABLE `civicrm_action_schedule` (
  `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  ...
  KEY `FK_civicrm_action_schedule_mapping_id` (`mapping_id`),
  ...

Is that the intended effect? Does this mean the old code was misbehaving?

@seamuslee001
Copy link
Contributor Author

I think so i have a feeling it may have not 100% but that is strange, I would expect the foreign key to be dropped.

@totten
Copy link
Member

totten commented Jan 25, 2017

jenkins, test this please

@colemanw
Copy link
Member

colemanw commented Feb 6, 2017

@totten is this ready to merge?

@seamuslee001
Copy link
Contributor Author

@totten @colemanw I think i have fixed the difference in the code in that i didn't relise KEY was just meaning an index I think this is good to go

@totten
Copy link
Member

totten commented Feb 9, 2017

Confirmed that the new upgrade logic produces the same schema as the old upgrade logic (but with safer code).

@totten totten merged commit 9daa0c7 into civicrm:master Feb 9, 2017
@seamuslee001 seamuslee001 deleted the CRM-18464 branch February 9, 2017 01:28
monishdeb pushed a commit to monishdeb/civicrm-core that referenced this pull request May 2, 2017
CRM-18464 use safe drop foreign key function to resolve upgrade error…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants