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

Menu mixin problem on older version of civi #257

Closed
demeritcowboy opened this issue Aug 10, 2022 · 9 comments
Closed

Menu mixin problem on older version of civi #257

demeritcowboy opened this issue Aug 10, 2022 · 9 comments

Comments

@demeritcowboy
Copy link
Contributor

This is on 5.44. There's an argument to upgrade and I can't reproduce the problem on master so I don't know how much time I'll spend tracking it down but what happens is that if you clear cache, the menu route disappears (visiting the url in the browser goes to the civi dashboard) and you need to visit /civicrm/menu/rebuild?reset=1 to get it to come back.

The extension also uses hook_navigation_menu, but might not be relevant.

In info.xml it has <format>22.05.2</format> and

<mixins>
    <mixin>mgd-php@1.0.0</mixin>
    <mixin>setting-php@1.0.0</mixin>
    <mixin>menu-xml@1.0.0</mixin>
  </mixins>

In xml/Menu:

<item>
    <path>civicrm/foo/bar</path>
    <page_callback>CRM_Foo_Form_Bar</page_callback>
    <title>My Settings</title>
    <access_arguments>administer CiviCRM</access_arguments>
  </item>
function foo_civicrm_navigationMenu(&$menu) {
  _foo_civix_insert_navigation_menu($menu, 'Administer/CiviContribute', [
    'label' => E::ts('My Settings'),
    'name' => 'foo_my_settings',
    'url' => 'civicrm/foo/bar?reset=1',
    'permission' => 'administer CiviCRM',
    'operator' => 'OR',
    'separator' => 0,
  ]);
  _foo_civix_navigationMenu($menu);
}
@ErikHommel
Copy link

I have the same issue: in older versions of CiviCRM my menu items disappear whenever I do a clear cache. And unlike above the civicrm/menu/rebuild?reset=1 does not bring it back

@jaapjansma
Copy link
Contributor

I have the same issue and discovered the cuase in the boilerplate code generated by civix.

in polyfill.php a check is done whether the mixin function has already run. This is the case when you have multiple extensions enabled in CiviCRM.

Change polyfill.php from

foreach ($mixins as $mixin) {
    // If there's trickery about installs/uninstalls/resets, then we may need to register a second time.
    if (!isset(\Civi::$statics[__FUNCTION__][$mixin])) {
      \Civi::$statics[__FUCTION__][$mixin] = 1;
      $func = $_CIVIX_MIXIN_POLYFILL[$mixin];
      $func($mixInfo, $bootCache);
    }
  }

to

foreach ($mixins as $mixin) {
    // If there's trickery about installs/uninstalls/resets, then we may need to register a second time.
    if (!isset(\Civi::$statics[$longName][$mixin])) {
      \Civi::$statics[$longName][$mixin] = 1;
      $func = $_CIVIX_MIXIN_POLYFILL[$mixin];
      $func($mixInfo, $bootCache);
    }
  }

I will submit a patch for civix soon.

@totten or @colemanw does this make sense to you?

@colemanw
Copy link
Contributor

Good detective work @jaapjansma - looks like the before version would cause multiple extensions to interfere with each other.

@jaapjansma
Copy link
Contributor

Yes indeed it does. I did the discovery on a Civi 5.37 version.

@jaapjansma
Copy link
Contributor

Ah and I see that I need to update the code in core and then update civix

@jaapjansma
Copy link
Contributor

I have created a PR: civicrm/civicrm-core#25179

@colemanw are you able to review that PR?

After the PR is ok and merged we can update CiviX to load the fixed boilerplate.

totten added a commit that referenced this issue Dec 16, 2022
totten added a commit to civicrm/civicrm-core that referenced this issue Dec 16, 2022
totten added a commit that referenced this issue Dec 16, 2022
Apply update polyfill. Add test coverage for generating multiple extensions. (#257)
@totten
Copy link
Owner

totten commented Dec 16, 2022

Well done @jaapjansma! The update works for me. Applied updates to both civicrm-core and civix.

@totten totten closed this as completed Dec 16, 2022
@jaapjansma
Copy link
Contributor

Thanks @totten

@jaapjansma
Copy link
Contributor

Would it be wise to release new version of civix and inform the community through mattermost that an update of civix is advisable to do? So that extension developers get the new polyfill as soon as they work on their extensions with civix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants