CRM-21174: Do not try to write civicrm_menu.module_data if it doesn't exist yet

[highfalutin]

Overview

Fix circular dependency during upgrade regarding introduction of module_data column to civicrm_menu.

Before

Upgrade via browser gives this message:

DB Error: no such field

After

Civi does not attempt to write to the civicrm_menu.module_data column during upgrade unless it already exists. The particular approach was recommended by @totten.

---

• CRM-21174: Circular dependency during upgrade: civicrm_menu.module_data

[seamuslee001]

@highfalutin i think this should be moved down and outside the if the reason is we still need to load in any new data into the other db fields before saving. @totten thoughts?

[highfalutin]

You're right. Should the foreach loop also be moved outside the if? This would unset unrecognized fields -- new behavior compared to previous releases.

[eileenmcnaughton]

I can & I think should edit this to be against the rc?

[seamuslee001]

I believe that is the sensible course of action

[eileenmcnaughton]

hmm - that didn't work!

[seamuslee001]

@highfalutin can you pls rebase against the branch 4.7.25-rc from core and only include relevant commits pls

[highfalutin]

@eileenmcnaughton I rebased against 4.7.25-rc and added a new commit to correct the error pointed out by @seamuslee001

[seamuslee001]

@highfalutin just thinking outloud i think that the only part that should be in if is the $menu->module_data and that should come after the copyValues

I suspect the patch should be something like

/*foreach statement here*/
$menu->copyValues($item);
if (!CRM_Core_Config::isUpgradeMode() ||
   CRM_Core_DAO::checkFieldExists('civicrm_menu', 'module_data', FALSE)
) {
  $menu->module_data = serialize($module_data);
}
else {
  unset($menu->module_data);
}

[highfalutin]

@seamuslee001 looks good, but, also thinking aloud, the only reason that we'd have $item['module_data'] would be if it were introduced from an extension's menu XML file or alterMenu hook. Should that be allowed, and if so, how should it be handled?

Your proposed patch implies "no, that's not allowed." Any module_data specified in XML or added by hook will be squashed or unset. That's fine with me -- I don't have an opinion on it but maybe @totten does?

[seamuslee001]

hmmm well i would have thought my patch is saying if we're in upgrade mode OR the field doesn't exist lets not try and write to module_data at all but if it is there we serialize the data. the reason being that the unset is contained in the else{} to that if statement but i think @totten needs to weigh in there

[seamuslee001]

pinging @totten this is still needing your feedback mate

[eileenmcnaughton]

@seamuslee001 I think I'm going to merge this & if we get @totten input it can be a follow up - I think getting a regression out of an rc is top priority & we should be getting this merged unless it regresses us - which I don't think it does.

[seamuslee001]

That seems like a sane strategy

[seamuslee001]

@highfalutin I have just been doing some testing on a PR with similar issues and i think we should change the || to be && as well because !CRM_Core_Config::isUpgradeMode() on its own seems to be causing some issues

[highfalutin]

OK, let's just also add a "TODO" to check whether the logic of this section is what we want. Starting with the current version (4.7.25), if we aren't in upgrade mode, the check for civicrm_menu.module_data should always come back true, because we create that column in this version. So that && really shouldn't be necessary. As @totten pointed out, the CRM_Core_DAO::checkFieldExists call has performance implications and it would be best to use it as sparingly as possible.

[highfalutin]

That said, regardless of whether we go with the current || or your proposed &&, I agree with @eileenmcnaughton that it would be best to merge this PR because it fixes a critical issue.

[seamuslee001]

@eileenmcnaughton @highfalutin @totten

so i created a little php script to do some testing after making sure that my database didn't have the module data field in it. this was the test script i came up with

<?php
civicrm_initialize();
 if (!CRM_Core_Config::isUpgradeMode() || CRM_Core_DAO::checkFieldExists('civicrm_menu', 'module_data', FALSE)) {
   print_r("If test passed even tho no module_data field upgrade only n check field\n");
 }
 if (!CRM_Core_Config::isUpgradeMode()) {
   print_r("If test passed even tho no module_data field upgrade only test\n");
 }
 if (CRM_Core_DAO::checkFieldExists('civicrm_menu', 'module_data', FALSE)) {
   print_r("If test passed even tho no module_data field check field only\n");
 }
 if (CRM_Core_Config::isUpgradeMode() || CRM_Core_DAO::checkFieldExists('civicrm_menu', 'module_data', FALSE)) {
   print_r("If test passed even tho no module_data field upgrade only with no bang n check field\n");
 }

the results were as follows

drush scr test.drush
If test passed even tho no module_data field upgrade only n check field
If test passed even tho no module_data field upgrade only test

note that i think because of the !isUppgradeMode() is creating a false true statement there

[highfalutin]

The results of your script look right to me: because you weren't in upgrade mode, the first two passed and the second two failed. I'm not sure what you mean by a "false true statement" 😉

[seamuslee001]

@highfalutin my point is that the database doesn't have the field module_data so in the first 2 tests it will be trying to write to the database and hence generating the error i would think but maybe that's actually ok and i'm just going round the twist.

[eileenmcnaughton]

Note merging as this is tangental to a bigger fix - keep twisting by all means

[highfalutin]

@seamuslee001 oh I see. Well my reasoning was this:

• if we're in version <4.7.25, module_data is missing, but this bit of code doesn't exist either so it's a moot point.
• if we're in version 4.7.25, module_data is only missing while upgrade mode is true -- and because upgrade is true, the if condition will fail and we won't try to write to module_data.
• after the upgrade is finished, we can safely assume module_data exists.

Does that make sense or do I need another cup of coffee? It may be the latter.

[eileenmcnaughton]

Can you make me one while you are at it?

[eileenmcnaughton]

Merging as agreed with @seamuslee001