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

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

Merged
merged 2 commits into from
Sep 19, 2017
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6f1a16
Do not try to write civicrm_menu.module_data if it doesn't exist yet
highfalutin Sep 12, 2017
c09129a
Store menu item fields other than module_data
highfalutin Sep 12, 2017
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
19 changes: 12 additions & 7 deletions CRM/Core/Menu.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,17 +317,22 @@ public static function store($truncate = TRUE) {

$menu->find(TRUE);

// Move unrecognized fields to $module_data.
$module_data = array();
foreach (array_keys($item) as $key) {
if (!isset($daoFields[$key])) {
$module_data[$key] = $item[$key];
unset($item[$key]);
if (!CRM_Core_Config::isUpgradeMode() ||
Copy link
Contributor

Choose a reason for hiding this comment

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

@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

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, 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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" 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make me one while you are at it?

CRM_Core_DAO::checkFieldExists('civicrm_menu', 'module_data', FALSE)
) {
// Move unrecognized fields to $module_data.
$module_data = array();
foreach (array_keys($item) as $key) {
if (!isset($daoFields[$key])) {
$module_data[$key] = $item[$key];
unset($item[$key]);
}
}

$menu->module_data = serialize($module_data);
}

$menu->copyValues($item);
$menu->module_data = serialize($module_data);

foreach (self::$_serializedElements as $element) {
if (!isset($item[$element]) ||
Expand Down